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

v8: backport 22116dd6c884c026225e56dd8e442a660193e729 #21992

Closed
wants to merge 1 commit into from

Conversation

laverdet
Copy link
Contributor

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
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
Contributor

mscdex commented Jul 27, 2018

/cc @nodejs/v8-update

@laverdet
Copy link
Contributor Author

Thanks, I went ahead and updated the PR.

@targos
Copy link
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
Contributor Author

@targos alright, I've made those changes.

Copy link
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
Member

targos commented Aug 1, 2018

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
Member

targos commented Sep 3, 2018

@targos
Copy link
Member

targos commented Sep 3, 2018

The V8 test fails at compile time:

Edit: sorry, wrong PR.

@targos
Copy link
Member

targos commented Sep 4, 2018

@targos
Copy link
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
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
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 6, 2018
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]>
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.

7 participants