★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/30708Nouvelle recherche | Portail wanayoo
Skip to content

/ node

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

deps: V8: cherry-pick ca5b0ec #30708

wants to merge 2 commits into from

Conversation

@addaleax
Copy link
Member

addaleax commented Nov 28, 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: https://github.com//issues/30498
Refs: https://github.com//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
@addaleax
deps: V8: cherry-pick ca5b0ec
Verified
This commit was signed with a verified signature.
addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
a8c5b51 
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: v8/v8@ca5b0ec
Fixes: #30498
Fixes: #30648
@addaleax
Revert "test: skip test-domain-error-types in debug mode temporariliy"
Verified
This commit was signed with a verified signature.
addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
c4f3cdf 
This reverts commit 6d022c1.
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

nodejs-github-bot commented Nov 28, 2019 •
edited by Trott

CI: https://ci.nodejs.org/job/node-test-pull-request/27151/ ✅
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2662/
@addaleax addaleax added the fast-track label Nov 28, 2019
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Nov 28, 2019

👍 this comment to approve fast-tracking, this fixes a flaky test in CI that has been very unstable this week.
@devsnek
devsnek approved these changes Nov 28, 2019
View changes
@cjihrig
cjihrig approved these changes Nov 29, 2019
View changes
@gengjiawen
gengjiawen approved these changes Nov 29, 2019
View changes
@Trott
Trott approved these changes Nov 29, 2019
View changes
@targos
targos approved these changes Nov 29, 2019
View changes
@gireeshpunathil
gireeshpunathil approved these changes Nov 29, 2019
View changes
@gireeshpunathil

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented Nov 29, 2019

new v8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2663/
@targos

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented Nov 29, 2019

ah, ok. so landing...
gireeshpunathil added a commit that referenced this pull request Nov 29, 2019
@addaleax @gireeshpunathil
deps: V8: cherry-pick ca5b0ec
29b5432 
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 added a commit that referenced this pull request Nov 29, 2019
@addaleax @gireeshpunathil
test: revert 6d022c1
Loading status checks…
58850f6 
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

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented Nov 29, 2019

landed as 58850f6 and 29b5432
@addaleax addaleax deleted the addaleax:heap-synth-module branch Nov 29, 2019
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Nov 29, 2019 •
edited

@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

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented Nov 29, 2019

@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

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Nov 29, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented Nov 29, 2019

thanks; between this conversation I guessed so (out-dated tooling).
addaleax added a commit that referenced this pull request Nov 30, 2019
@addaleax
deps: V8: cherry-pick ca5b0ec
Verified
This commit was signed with a verified signature.
addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
aa01ebd 
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
@addaleax
test: revert 6d022c1
Verified
This commit was signed with a verified signature.
addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
83861fb 
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
Merged
MylesBorins added a commit that referenced this pull request Jan 12, 2020
@addaleax @MylesBorins
test: revert 6d022c1
Verified
This commit was signed with a verified signature.
MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
eb4b932 
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 added a commit that referenced this pull request Jan 12, 2020
@addaleax @MylesBorins
deps: V8: cherry-pick ca5b0ec
Verified
This commit was signed with a verified signature.
MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
fcc4bf9 
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>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs added a commit that referenced this pull request Feb 6, 2020
@addaleax @BethGriggs
test: revert 6d022c1
Verified
This commit was signed with a verified signature.
BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
e0eb670 
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 added a commit that referenced this pull request Feb 6, 2020
@addaleax @BethGriggs
deps: V8: cherry-pick ca5b0ec
Verified
This commit was signed with a verified signature.
BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
6bee687 
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
Merged
saitonakamura pushed a commit to saitonakamura/node that referenced this pull request Apr 7, 2020
@addaleax Michael Bashurov
deps: V8: cherry-pick ca5b0ec
d87b026 
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: https://github.com/v8/v8/commit/ \
ca5b0ec2722d2af4551c01ca78921fa16a26ae72
Fixes: nodejs#30498
Fixes: nodejs#30648

PR-URL: nodejs#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>
saitonakamura pushed a commit to saitonakamura/node that referenced this pull request Apr 7, 2020
@addaleax Michael Bashurov
test: revert 6d022c1
68d976f 
Revert "test: skip test-domain-error-types in debug mode temporariliy"
This reverts commit 6d022c1.

PR-URL: nodejs#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott

@targos targos

@cjihrig cjihrig

@gengjiawen gengjiawen

@devsnek devsnek

@gireeshpunathil gireeshpunathil

Assignees
No one assigned
Labels
V8 Engine build fast-track
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

investigate flaky test_make_callback/test-async-hooks-gcable in CI
investigate flaky test-domain-error-types in CI
8 participants
@addaleax @nodejs-github-bot @gireeshpunathil @targos @Trott @cjihrig @gengjiawen @devsnek
You can’t perform that action at this time.