deps: cherry-pick f4a2b7f3 from V8 upstream. #16053

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 ofrobots, targos and fhinkel 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: cherry-pick f4a2b7f3 from V8 upstream. #16053

deps: cherry-pick f4a2b7f3 from V8 upstream. #16053

erinishimoticha commented Oct 6, 2017 •

edited

Loading

erinishimoticha commented Oct 6, 2017

MylesBorins left a comment

MylesBorins commented Oct 6, 2017

gibfahn commented Oct 6, 2017

MylesBorins commented Oct 6, 2017

gibfahn commented Oct 6, 2017

ofrobots left a comment

MylesBorins commented Oct 6, 2017

MylesBorins commented Oct 6, 2017

ofrobots commented Oct 7, 2017

erinishimoticha commented Oct 7, 2017

MylesBorins commented Oct 7, 2017

ofrobots commented Oct 7, 2017

erinishimoticha commented Oct 7, 2017

MylesBorins commented Oct 7, 2017

deps: cherry-pick f4a2b7f3 from V8 upstream. #16053

deps: cherry-pick f4a2b7f3 from V8 upstream. #16053

Conversation

erinishimoticha commented Oct 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

erinishimoticha commented Oct 6, 2017

MylesBorins left a comment

Choose a reason for hiding this comment

MylesBorins commented Oct 6, 2017

gibfahn commented Oct 6, 2017

MylesBorins commented Oct 6, 2017

gibfahn commented Oct 6, 2017

ofrobots left a comment

Choose a reason for hiding this comment

MylesBorins commented Oct 6, 2017

MylesBorins commented Oct 6, 2017

ofrobots commented Oct 7, 2017

erinishimoticha commented Oct 7, 2017

MylesBorins commented Oct 7, 2017

ofrobots commented Oct 7, 2017

erinishimoticha commented Oct 7, 2017

MylesBorins commented Oct 7, 2017

erinishimoticha commented Oct 6, 2017 •

edited

Loading