Skip to content

v8: backport 22116dd6c884c026225e56dd8e442a660193e729#21992

Closed
laverdet wants to merge 1 commit intonodejs:masterfrom
laverdet:v10.x-staging
Closed

v8: backport 22116dd6c884c026225e56dd8e442a660193e729#21992
laverdet wants to merge 1 commit intonodejs:masterfrom
laverdet:v10.x-staging

Conversation

@laverdet
Copy link
Copy Markdown
Contributor

@laverdet laverdet commented Jul 27, 2018

This is a backport for v8 commit 22116dd6 which fixes v8 issue #7857. The issue does not affect core nodejs but does affect my native npm module isolated-vm. Essentially v8 will segfault if you try to create a startup snapshot of an isolate that contains a closure.

The snapshot crash as it pertains to isolated-vm was originally reported on superfly/fly#101.

The bug was introduced in v8 commit 6bd1d3c2, landed in v8 version 6.7.247, which made its way onto nodejs v10.2.0.

The fix landed in v8 version 6.9.186 will probably never see the light of day on the v10x branch of nodejs, which leads me to this PR :)

The patch applied cleanly with no conflicts.

Refs: https://github.com/v8/v8/commit/22116dd6c884c026225e56dd8e442a660193e729

Original commit message:

    [snapshot] fix resetting function code.

    Unconditionally setting the JSFunction code to that of the SFI
    may skip initializing the feedback vector.

    [email protected]

    Bug: v8:7857
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I65d4bf32493be4cade2eaf3d665d44f93e80f809
    Reviewed-on: https://chromium-review.googlesource.com/1107618
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#53881}
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added v10.x v8 engine Issues and PRs related to the V8 dependency. labels Jul 27, 2018
@mscdex
Copy link
Copy Markdown
Contributor

mscdex commented Jul 27, 2018

I believe the v8_embedder_string variable in common.gypi needs to be incremented any time we float patches on top of V8.

@mscdex
Copy link
Copy Markdown
Contributor

mscdex commented Jul 27, 2018

/cc @nodejs/v8-update

@laverdet
Copy link
Copy Markdown
Contributor Author

laverdet commented Jul 27, 2018

Thanks, I went ahead and updated the PR.

@targos
Copy link
Copy Markdown
Member

targos commented Jul 29, 2018

@laverdet Could you please retarget this PR to the master branch (and change the embedder string accordingly)? We are still at V8 6.8 there and that version is going to be backported to v10.x.

Also, the first line of the commit message is a bit too long and should start with "deps".
May I suggest: deps: cherry-pick 22116dd from upstream V8? (we use "backport" when manual changes were necessary because of conflicts, otherwise it's "cherry-pick").

@laverdet laverdet changed the base branch from v10.x-staging to master July 29, 2018 16:43
@laverdet
Copy link
Copy Markdown
Contributor Author

laverdet commented Jul 29, 2018

@targos alright, I've made those changes.

targos
targos approved these changes Aug 1, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@targos targos removed the v10.x label Aug 1, 2018
@targos
Copy link
Copy Markdown
Member

targos commented Aug 1, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16115/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1560/

@laverdet @targos
deps: cherry-pick 22116dd from upstream V8
e23ea6f 
Refs: v8/v8@22116dd

Original commit message:

    [snapshot] fix resetting function code.

    Unconditionally setting the JSFunction code to that of the SFI
    may skip initializing the feedback vector.

    [email protected]

    Bug: v8:7857
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I65d4bf32493be4cade2eaf3d665d44f93e80f809
    Reviewed-on: https://chromium-review.googlesource.com/1107618
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#53881}
@targos
Copy link
Copy Markdown
Member

targos commented Sep 3, 2018

I rebased the PR on top of master.
New CI: https://ci.nodejs.org/job/node-test-pull-request/16992/
V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/1648/

@targos
Copy link
Copy Markdown
Member

targos commented Sep 3, 2018 •
edited
Loading

The V8 test fails at compile time:

Edit: sorry, wrong PR.

@targos
Copy link
Copy Markdown
Member

targos commented Sep 4, 2018 •
edited
Loading

Fixed V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1654/
Edit: rebuild: https://ci.nodejs.org/job/node-test-commit-v8-linux/1659/

@targos targos mentioned this pull request Sep 4, 2018
Closed
@targos
Copy link
Copy Markdown
Member

targos commented Sep 4, 2018

Landed in 0d3da39

@targos targos closed this Sep 4, 2018
targos pushed a commit that referenced this pull request Sep 4, 2018
@laverdet @targos
deps: cherry-pick 22116dd from upstream V8
0d3da39 
Refs: v8/v8@22116dd

Original commit message:

    [snapshot] fix resetting function code.

    Unconditionally setting the JSFunction code to that of the SFI
    may skip initializing the feedback vector.

    [email protected]

    Bug: v8:7857
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I65d4bf32493be4cade2eaf3d665d44f93e80f809
    Reviewed-on: https://chromium-review.googlesource.com/1107618
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#53881}

PR-URL: #21992
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
targos pushed a commit that referenced this pull request Sep 5, 2018
@laverdet @targos
deps: cherry-pick 22116dd from upstream V8
212e6bb 
Refs: v8/v8@22116dd

Original commit message:

    [snapshot] fix resetting function code.

    Unconditionally setting the JSFunction code to that of the SFI
    may skip initializing the feedback vector.

    [email protected]

    Bug: v8:7857
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I65d4bf32493be4cade2eaf3d665d44f93e80f809
    Reviewed-on: https://chromium-review.googlesource.com/1107618
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#53881}

PR-URL: #21992
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@targos targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
@laverdet @targos
deps: cherry-pick 22116dd from upstream V8
037906b 
Refs: v8/v8@22116dd

Original commit message:

    [snapshot] fix resetting function code.

    Unconditionally setting the JSFunction code to that of the SFI
    may skip initializing the feedback vector.

    [email protected]

    Bug: v8:7857
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I65d4bf32493be4cade2eaf3d665d44f93e80f809
    Reviewed-on: https://chromium-review.googlesource.com/1107618
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Leszek Swirski <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#53881}

PR-URL: #21992
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@beevelop beevelop mentioned this pull request Sep 6, 2018
Closed
@beevelop beevelop mentioned this pull request Sep 6, 2018
Closed
@laverdet laverdet deleted the v10.x-staging branch May 6, 2019 17:10
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request May 16, 2022
Open
This was referenced May 16, 2022
Open
Open
@kissofire kissofire mentioned this pull request May 16, 2022
Open
@wedataintelligence wedataintelligence mentioned this pull request May 16, 2022
Open
@ron-a20 ron-a20 mentioned this pull request May 16, 2022
Open
@molllyn1 molllyn1 mentioned this pull request May 17, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fhinkel fhinkel fhinkel approved these changes

@targos targos targos approved these changes

+2 more reviewers

@TimothyGu TimothyGu TimothyGu approved these changes

@devsnek devsnek devsnek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@laverdet @mscdex @targos @fhinkel @TimothyGu @devsnek @nodejs-github-bot