Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: cherry-pick 3c8195d from V8 upstream #16897

Closed
wants to merge 1 commit into from

Conversation

fhinkel
Copy link
Member

@fhinkel fhinkel commented Nov 9, 2017

Original commit message:

[map] Fix map constructor to correctly throw.

We need to throw before rethrowing, otherwise the exception does
not trigger a debugger event and is not reported if uncaught.

R=gsathya@chromium.org, jgruber@chromium.org

Bug: v8:7047
Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
Reviewed-on: https://chromium-review.googlesource.com/758372
Reviewed-by: Jakob Gruber jgruber@chromium.org
Reviewed-by: Sathya Gunasekaran gsathya@chromium.org
Commit-Queue: Yang Guo yangguo@chromium.org
Cr-Commit-Position: refs/heads/master@{#49237}

Fixes: #16856

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

deps

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Nov 9, 2017
@fhinkel fhinkel force-pushed the nov/backport-map-error-fix branch 2 times, most recently from a681714 to be55877 Compare November 9, 2017 05:17
@targos
Copy link
Member

targos commented Nov 10, 2017

Can this one be merged to 6.2 and 6.3 on V8 side?

@fhinkel
Copy link
Member Author

fhinkel commented Nov 11, 2017

@targos I openend a merge request: https://bugs.chromium.org/p/v8/issues/detail?id=7071

🤔 When I opened this PR, I assumed we can't merge to stable (62) upstream. But it's a correctness fix, so we should merge upstream. Waiting for feedback from V8 PMs on Monday.

Original commit message:

  [map] Fix map constructor to correctly throw.

  We need to throw before rethrowing, otherwise the exception does
  not trigger a debugger event and is not reported if uncaught.

  R=gsathya@chromium.org, jgruber@chromium.org

  Bug: v8:7047
  Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
  Reviewed-on: https://chromium-review.googlesource.com/758372
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#49237}

Fixes: nodejs#16856
@bricss bricss mentioned this pull request Nov 13, 2017
@fhinkel
Copy link
Member Author

fhinkel commented Nov 14, 2017

This is not a critical fix so we cant back-merge it to 6.2 upstream. Landed in e7f30db.

@fhinkel fhinkel closed this Nov 14, 2017
fhinkel added a commit that referenced this pull request Nov 14, 2017
Original commit message:

  [map] Fix map constructor to correctly throw.

  We need to throw before rethrowing, otherwise the exception does
  not trigger a debugger event and is not reported if uncaught.

  R=gsathya@chromium.org, jgruber@chromium.org

  Bug: v8:7047
  Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
  Reviewed-on: https://chromium-review.googlesource.com/758372
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#49237}

PR-URL: #16897
Fixes: #16856
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
evanlucas pushed a commit that referenced this pull request Nov 14, 2017
Original commit message:

  [map] Fix map constructor to correctly throw.

  We need to throw before rethrowing, otherwise the exception does
  not trigger a debugger event and is not reported if uncaught.

  R=gsathya@chromium.org, jgruber@chromium.org

  Bug: v8:7047
  Change-Id: I7ce0253883a21d6059e4e0ed0fc56dc55a0dcba6
  Reviewed-on: https://chromium-review.googlesource.com/758372
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#49237}

PR-URL: #16897
Fixes: #16856
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos
Copy link
Member

targos commented Nov 23, 2017

This was landed without a CI run and the new test is failing on master: https://ci.nodejs.org/job/node-test-commit-v8-linux/1072/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new Map(wrong_iterable) swallow TypeError in file scripts
7 participants