deps: cherry-pick f4a2b7f3 from V8 upstream. by erinishimoticha · Pull Request #16053 · nodejs/node

erinishimoticha · 2017-10-06T20:53:03Z

Also requested backport to v8 6.1 and 6.2.

Bug: https://bugs.chromium.org/p/v8/issues/detail?id=6902
Merge request for 6.1: https://chromium-review.googlesource.com/c/v8/v8/+/706213
Merge request for 6.2: https://chromium-review.googlesource.com/c/v8/v8/+/706199

Original commit message:

should ignore asyncTask* with null

In V8Debugger code we don't expect task_id == null, e.g.
asyncTaskStartedForStepping will trigger debug break on null as task_id.
Let's filter task_id == null out.

This issue is originally filed in Node.js:
https://github.com/nodejs/node/issues/15464

R=dgozman@chromium.org

Bug: none
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Icc9f96105b3c91ee1b102d545a7817f7ee93394c
Reviewed-on: https://chromium-review.googlesource.com/695808
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48265}

Fixes #15464

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

V8

erinishimoticha · 2017-10-06T20:53:19Z

cc: @MylesBorins

MylesBorins

LGTM

MylesBorins · 2017-10-06T20:55:45Z

I would like to land this asap so I can roll it into the 8.7.0-rc

ci: https://ci.nodejs.org/job/node-test-pull-request/10432/

gibfahn · 2017-10-06T20:59:15Z

Could the commit message include a link to the V8 commit (i.e. v8/v8@f4a2b7f)?

MylesBorins · 2017-10-06T21:00:25Z

@gibfahn in the past we haven't done that... and it isn't outlined in our V8 guide

might make sense to update the guide?

gibfahn · 2017-10-06T21:01:26Z

Yeah definitely. Just took me a second to find it, so thought it'd be nice to have in case anyone else had the same problem.

ofrobots

LGTM. I was going add a note for posterity that the v8-version is not being bumped because V8 6.1 is still stable upstream, but I see that a new patch includes a version bump.

nit: Could we undo the version bump? There is still a small chance that upstream might merge a patch giving us two conflicting version numbers between upstream and us.

MylesBorins · 2017-10-06T21:07:31Z

@ofrobots my bad... for some reason i thought 6.1 was already done.

We should request this patch be backported to 6.1 && 6.2

@erinspice might be interested in helping with this

Use `Commit:` for the V8 commit, and `PR-URL:` for the Node PR URL. Refs: nodejs#16053 (comment)

Original commit message: should ignore asyncTask* with null In V8Debugger code we don't expect task_id == null, e.g. asyncTaskStartedForStepping will trigger debug break on null as task_id. Let's filter task_id == null out. This issue is originally filed in Node.js: nodejs#15464 R=dgozman@chromium.org Bug: none Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: Icc9f96105b3c91ee1b102d545a7817f7ee93394c Reviewed-on: https://chromium-review.googlesource.com/695808 Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#48265} Fixes nodejs#15464

MylesBorins · 2017-10-06T23:40:08Z

ci: https://ci.nodejs.org/job/node-test-pull-request/10439/
ci-V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/951/

ofrobots · 2017-10-07T05:20:22Z

Yep, it would be good to request a merge to 6.1 and 6.2 upstream. I can open this request.

erinishimoticha · 2017-10-07T05:54:41Z

Yep, I put up the merge requests to 6.2 and 6.1 this morning.

MylesBorins · 2017-10-07T17:55:48Z

One more attempt at CI: https://ci.nodejs.org/job/node-test-pull-request/10458/
V8 is green

ofrobots · 2017-10-07T18:20:33Z

@erinspice awesome! For posterity, can you put a link to the V8 bug you opened for the merge? I can make sure it makes progress on the upstream side.

erinishimoticha · 2017-10-07T18:54:34Z

@ofrobots Yes!

Bug: https://bugs.chromium.org/p/v8/issues/detail?id=6902
Merge request for 6.1: https://chromium-review.googlesource.com/c/v8/v8/+/706213
Merge request for 6.2: https://chromium-review.googlesource.com/c/v8/v8/+/706199

Adding this to this PR summary, too.

MylesBorins · 2017-10-07T21:03:38Z

landed in be2a5b3

Original commit message: should ignore asyncTask* with null In V8Debugger code we don't expect task_id == null, e.g. asyncTaskStartedForStepping will trigger debug break on null as task_id. Let's filter task_id == null out. This issue is originally filed in Node.js: #15464 R=dgozman@chromium.org Bug: none Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: Icc9f96105b3c91ee1b102d545a7817f7ee93394c Reviewed-on: https://chromium-review.googlesource.com/695808 Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Cr-Commit-Position: refs/heads/master@{#48265} Fixes #15464 PR-URL: #16053 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Original commit message: should ignore asyncTask* with null In V8Debugger code we don't expect task_id == null, e.g. asyncTaskStartedForStepping will trigger debug break on null as task_id. Let's filter task_id == null out. This issue is originally filed in Node.js: nodejs/node#15464 R=dgozman@chromium.org Bug: none Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: Icc9f96105b3c91ee1b102d545a7817f7ee93394c Reviewed-on: https://chromium-review.googlesource.com/695808 Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Cr-Commit-Position: refs/heads/master@{#48265} Fixes nodejs/node#15464 PR-URL: nodejs/node#16053 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Use `Commit:` for the V8 commit, and `PR-URL:` for the Node PR URL. Refs: #16053 (comment) PR-URL: #16054 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Use `Commit:` for the V8 commit, and `PR-URL:` for the Node PR URL. Refs: nodejs/node#16053 (comment) PR-URL: nodejs/node#16054 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Use `Commit:` for the V8 commit, and `PR-URL:` for the Node PR URL. Refs: #16053 (comment) PR-URL: #16054 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Oct 6, 2017

MylesBorins approved these changes Oct 6, 2017

View reviewed changes

MylesBorins requested review from fhinkel, ofrobots and targos October 6, 2017 20:54

addaleax approved these changes Oct 6, 2017

View reviewed changes

gibfahn approved these changes Oct 6, 2017

View reviewed changes

erinishimoticha force-pushed the issue-15464 branch from 36bda95 to 7bd16fa Compare October 6, 2017 20:59

ofrobots approved these changes Oct 6, 2017

View reviewed changes

BridgeAR approved these changes Oct 6, 2017

View reviewed changes

This was referenced Oct 6, 2017

doc: include V8 commit URL in V8 backport guide #16054

Closed

build: add V8 embedder version string #15785

Closed

gibfahn added a commit to gibfahn/node that referenced this pull request Oct 6, 2017

doc: include V8 commit URL in V8 backport guide

4f6d1b0

Use `Commit:` for the V8 commit, and `PR-URL:` for the Node PR URL. Refs: nodejs#16053 (comment)

erinishimoticha force-pushed the issue-15464 branch from 7bd16fa to e466e36 Compare October 6, 2017 21:24

targos approved these changes Oct 6, 2017

View reviewed changes

TimothyGu approved these changes Oct 7, 2017

View reviewed changes

cjihrig approved these changes Oct 7, 2017

View reviewed changes

MylesBorins mentioned this pull request Oct 7, 2017

inspector: not pass zero as task_id #15740

Closed

2 tasks

MylesBorins closed this Oct 7, 2017

MylesBorins mentioned this pull request Oct 7, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

joyeecheung mentioned this pull request Feb 20, 2025

Debugging v8.5 - always pauses in async_hooks.js on promise rejection #15464

Closed

Uh oh!

Comments

Conversation

erinishimoticha commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

erinishimoticha commented Oct 6, 2017

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Oct 6, 2017

Uh oh!

gibfahn commented Oct 6, 2017

Uh oh!

MylesBorins commented Oct 6, 2017

Uh oh!

gibfahn commented Oct 6, 2017

Uh oh!

ofrobots left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Oct 6, 2017

Uh oh!

MylesBorins commented Oct 6, 2017

Uh oh!

ofrobots commented Oct 7, 2017

Uh oh!

erinishimoticha commented Oct 7, 2017

Uh oh!

MylesBorins commented Oct 7, 2017

Uh oh!

ofrobots commented Oct 7, 2017

Uh oh!

erinishimoticha commented Oct 7, 2017

Uh oh!

MylesBorins commented Oct 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

erinishimoticha commented Oct 6, 2017 •

edited

Loading