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: V8: cherry-pick ca5b0ec #30708

Closed
wants to merge 2 commits into from
Closed

Conversation

addaleax
Copy link
Member

Original commit message:

[heap] Ensure SyntheticModule is initialized before next allocation

Ensure that all fields of `SyntheticModule` are set before creating
the exports hash table for it, because the latter may trigger
garbage collection, leading to crashes.

This has been causing failures in the Node.js CI over the last weeks,
after making the creating of synthetic modules part of Node’s
startup sequence.

(I am generally not very familiar with this part of the V8
code and there might be a better way, or possibly a way to add a
reliable regression test, that I am not aware of.)

Refs: https://github.com/nodejs/node/issues/30498
Refs: https://github.com/nodejs/node/issues/30648
Change-Id: I32da4b7bd888c6ec1421f34f5bd52e7bad154c1e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1939752
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65247}

Refs: v8/v8@ca5b0ec
Fixes: #30498
Fixes: #30648

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

Original commit message:

    [heap] Ensure SyntheticModule is initialized before next allocation

    Ensure that all fields of `SyntheticModule` are set before creating
    the exports hash table for it, because the latter may trigger
    garbage collection, leading to crashes.

    This has been causing failures in the Node.js CI over the last weeks,
    after making the creating of synthetic modules part of Node’s
    startup sequence.

    (I am generally not very familiar with this part of the V8
    code and there might be a better way, or possibly a way to add a
    reliable regression test, that I am not aware of.)

    Refs: nodejs#30498
    Refs: nodejs#30648
    Change-Id: I32da4b7bd888c6ec1421f34f5bd52e7bad154c1e
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1939752
    Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#65247}

Refs: v8/v8@ca5b0ec
Fixes: nodejs#30498
Fixes: nodejs#30648
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Nov 28, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 28, 2019

@addaleax addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 28, 2019
@addaleax
Copy link
Member Author

👍 this comment to approve fast-tracking, this fixes a flaky test in CI that has been very unstable this week.

@gireeshpunathil
Copy link
Member

@targos
Copy link
Member

targos commented Nov 29, 2019

@gireeshpunathil V8 CI is as good as it can be. There are two permanent red jobs because of #30152.

@gireeshpunathil
Copy link
Member

ah, ok. so landing...

gireeshpunathil pushed a commit that referenced this pull request Nov 29, 2019
Original commit message:

[heap] Ensure SyntheticModule is initialized before next allocation

Ensure that all fields of `SyntheticModule` are set before creating
the exports hash table for it, because the latter may trigger
garbage collection, leading to crashes.

This has been causing failures in the Node.js CI over the last weeks,
after making the creating of synthetic modules part of Node’s
startup sequence.

(I am generally not very familiar with this part of the V8
code and there might be a better way, or possibly a way to add a
reliable regression test, that I am not aware of.)

Refs: #30498
Refs: #30648
Change-Id: I32da4b7bd888c6ec1421f34f5bd52e7bad154c1e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1939752
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65247}

Refs: https://github.com/v8/v8/commit/ \
ca5b0ec2722d2af4551c01ca78921fa16a26ae72
Fixes: #30498
Fixes: #30648

PR-URL: #30708
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
gireeshpunathil pushed a commit that referenced this pull request Nov 29, 2019
Revert "test: skip test-domain-error-types in debug mode temporariliy"
This reverts commit 6d022c1.

PR-URL: #30708
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@gireeshpunathil
Copy link
Member

landed as 58850f6 and 29b5432

@addaleax addaleax deleted the heap-synth-module branch November 29, 2019 10:22
@addaleax
Copy link
Member Author

addaleax commented Nov 29, 2019

@gireeshpunathil Just so you know, the commit message for the second commit was just fine – our tooling has special handling for the standard git revert format, so it would have highlighted that the commit was a revert but otherwise have handled it correctly.

Also, I think some indentation was lost for the first one?

@gireeshpunathil
Copy link
Member

@addaleax - this is what I got with the core-validate-commit . So I just adjusted things such that it does not complain!

did not know that the tooling will take care of those stuff. Is there anything needs to do now, or is it just ok?

#git rev-list upstream/master...HEAD | xargs core-validate-commit
  ✖  1a46b7ccd7b786a990b80413792db66b1bbc1aad
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✔  0:0      metadata is at end of message             metadata-end
     ✔  0:0      reviewers are valid                       reviewers
     ✔  0:0      valid subsystems                          subsystem
     ✖  0:50     Title must be <= 50 columns.              title-length
  ✖  dcc885f9b3c269bc0f78c288898214b7f90465f2
     ✔  26:7     Valid fixes url                           fixes-url
     ✔  27:7     Valid fixes url                           fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✖  9:72     Line should be <= 72 columns.             line-length
     ✖  20:72    Line should be <= 72 columns.             line-length
     ✖  25:72    Line should be <= 72 columns.             line-length
     ✔  0:0      metadata is at end of message             metadata-end
     ✔  0:0      reviewers are valid                       reviewers
     ✔  0:0      valid subsystems                          subsystem
     ✔  0:0      Title is <= 50 columns.                   title-length

@addaleax
Copy link
Member Author

I think it’s fine, I just wanted to let you know :)

That being said, I think your local version of core-validate-commit might be out of date? I think all of these issues have been fixed by now.

@gireeshpunathil
Copy link
Member

thanks; between this conversation I guessed so (out-dated tooling).

addaleax added a commit that referenced this pull request Nov 30, 2019
Original commit message:

[heap] Ensure SyntheticModule is initialized before next allocation

Ensure that all fields of `SyntheticModule` are set before creating
the exports hash table for it, because the latter may trigger
garbage collection, leading to crashes.

This has been causing failures in the Node.js CI over the last weeks,
after making the creating of synthetic modules part of Node’s
startup sequence.

(I am generally not very familiar with this part of the V8
code and there might be a better way, or possibly a way to add a
reliable regression test, that I am not aware of.)

Refs: #30498
Refs: #30648
Change-Id: I32da4b7bd888c6ec1421f34f5bd52e7bad154c1e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1939752
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65247}

Refs: https://github.com/v8/v8/commit/ \
ca5b0ec2722d2af4551c01ca78921fa16a26ae72
Fixes: #30498
Fixes: #30648

PR-URL: #30708
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Nov 30, 2019
Revert "test: skip test-domain-error-types in debug mode temporariliy"
This reverts commit 6d022c1.

PR-URL: #30708
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
Revert "test: skip test-domain-error-types in debug mode temporariliy"
This reverts commit 6d022c1.

PR-URL: #30708
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
Original commit message:

[heap] Ensure SyntheticModule is initialized before next allocation

Ensure that all fields of `SyntheticModule` are set before creating
the exports hash table for it, because the latter may trigger
garbage collection, leading to crashes.

This has been causing failures in the Node.js CI over the last weeks,
after making the creating of synthetic modules part of Node’s
startup sequence.

(I am generally not very familiar with this part of the V8
code and there might be a better way, or possibly a way to add a
reliable regression test, that I am not aware of.)

Refs: #30498
Refs: #30648
Change-Id: I32da4b7bd888c6ec1421f34f5bd52e7bad154c1e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1939752
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65247}

Refs: https://github.com/v8/v8/commit/ \
ca5b0ec2722d2af4551c01ca78921fa16a26ae72
Fixes: #30498
Fixes: #30648

PR-URL: #30708
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Revert "test: skip test-domain-error-types in debug mode temporariliy"
This reverts commit 6d022c1.

PR-URL: #30708
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Original commit message:

[heap] Ensure SyntheticModule is initialized before next allocation

Ensure that all fields of `SyntheticModule` are set before creating
the exports hash table for it, because the latter may trigger
garbage collection, leading to crashes.

This has been causing failures in the Node.js CI over the last weeks,
after making the creating of synthetic modules part of Node’s
startup sequence.

(I am generally not very familiar with this part of the V8
code and there might be a better way, or possibly a way to add a
reliable regression test, that I am not aware of.)

Refs: #30498
Refs: #30648
Change-Id: I32da4b7bd888c6ec1421f34f5bd52e7bad154c1e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1939752
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65247}

Refs: https://github.com/v8/v8/commit/ \
ca5b0ec2722d2af4551c01ca78921fa16a26ae72
Fixes: #30498
Fixes: #30648

PR-URL: #30708
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
8 participants