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.

Sign up for GitHub

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

Jump to bottom

[v14.x] deps: V8: cherry-pick 81181a8ad80a #39187

Merged
merged 1 commit into from
Jul 15, 2021
Merged

[v14.x] deps: V8: cherry-pick 81181a8ad80a #39187

merged 1 commit into from
Jul 15, 2021

Conversation

thomasmichaelwallace
Copy link

Original commit message:

[JSON] Fix GC issue in BuildJsonObject
We must ensure that the sweeper is not running or has already swept
mutable_double_buffer. Otherwise the GC can add it to the free list.

Bug: v8:11837
Change-Id: Ifd9cf15f1c94f664fd6489c70bb38b59730cdd78
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2928181
Commit-Queue: Victor Gomes <victorgomes@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74859}

Refs: v8/v8@81181a8

Fixes: #37553

Sorry if I've got this wrong; I've tried to follow the v8 patch contribution guidelines, but this is my first time and C++ is not my best language 😄

Some additional context:

  • this is an v8 fix for a bug causing failure on AWS lambda (which tracks node 14)
  • therefore I think it's a good candidate for a backport
  • this fix was backported in v8 from master to v9.2.230.13 (https://chromium-review.googlesource.com/c/v8/v8/+/2988414)
  • if I understand correctly, this means the fix hasn't hit node 16, but it will be picked up in time because node 16 is tracking v8 9.2.X
  • the node 14 v8 code has diverged enough that I had to apply this patch manually (which was easy, given how small it is!)

@github-actions github-actions bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v14.x v8 engine Issues and PRs related to the V8 dependency. labels Jun 28, 2021
@targos
Copy link
Member

targos commented Jun 29, 2021

Thanks, it's perfect!

I opened #39196 for master/v16.x. It includes this commit.

mcollina
mcollina approved these changes Jun 30, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 30, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 30, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 30, 2021 •
edited by targos
Loading

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

@thomasmichaelwallace
Copy link
Author

thomasmichaelwallace commented Jul 1, 2021 •
edited
Loading

~I'm not really sure how to interpret these CI results; if I need to do anything, please let me know 😄 ~

CI errors fixed by #39244.

I've synced this PR (v8_embedder_string required a merge), but I don't know (may not be able to?) start the V8 CI (@targos ?).

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38820/

@targos
Copy link
Member

targos commented Jul 15, 2021

@richardlau is going to prepare the next v14.x release

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 15, 2021 •
edited by richardlau
Loading

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

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/39076/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/39080/

@thomasmichaelwallace @richardlau
deps: V8: cherry-pick 81181a8ad80a
848cf46 
Original commit message:

    [JSON] Fix GC issue in BuildJsonObject
    We must ensure that the sweeper is not running or has already swept
    mutable_double_buffer. Otherwise the GC can add it to the free list.

    Bug: v8:11837
    Change-Id: Ifd9cf15f1c94f664fd6489c70bb38b59730cdd78
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2928181
    Commit-Queue: Victor Gomes <victorgomes@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#74859}

Refs: v8/v8@81181a8

PR-URL: nodejs#39187
Fixes: nodejs#37553
Refs: v8/v8@81181a8
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@richardlau richardlau merged commit 848cf46 into nodejs:v14.x-staging Jul 15, 2021
@richardlau
Copy link
Member

Landed in 848cf46.

richardlau pushed a commit that referenced this pull request Jul 20, 2021
@thomasmichaelwallace @richardlau
deps: V8: cherry-pick 81181a8ad80a
4213e97 
Original commit message:

    [JSON] Fix GC issue in BuildJsonObject
    We must ensure that the sweeper is not running or has already swept
    mutable_double_buffer. Otherwise the GC can add it to the free list.

    Bug: v8:11837
    Change-Id: Ifd9cf15f1c94f664fd6489c70bb38b59730cdd78
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2928181
    Commit-Queue: Victor Gomes <victorgomes@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#74859}

Refs: v8/v8@81181a8

PR-URL: #39187
Fixes: #37553
Refs: v8/v8@81181a8
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@richardlau richardlau mentioned this pull request Jul 20, 2021
Merged
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@thomasmichaelwallace @foxxyz
deps: V8: cherry-pick 81181a8ad80a
fe9d00c 
Original commit message:

    [JSON] Fix GC issue in BuildJsonObject
    We must ensure that the sweeper is not running or has already swept
    mutable_double_buffer. Otherwise the GC can add it to the free list.

    Bug: v8:11837
    Change-Id: Ifd9cf15f1c94f664fd6489c70bb38b59730cdd78
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2928181
    Commit-Queue: Victor Gomes <victorgomes@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#74859}

Refs: v8/v8@81181a8

PR-URL: nodejs#39187
Fixes: nodejs#37553
Refs: v8/v8@81181a8
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@richardlau richardlau richardlau approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@targos targos Awaiting requested review from targos

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. 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.
6 participants
@thomasmichaelwallace @targos @nodejs-github-bot @richardlau @mcollina @gireeshpunathil