FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v14.x] deps: V8: cherry-pick 81181a8ad80a by thomasmichaelwallace · Pull Request #39187 · nodejs/node · GitHub

/ node Public

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

Merged
richardlau merged 1 commit into
nodejs:v14.x-stagingfrom
thomasmichaelwallace:v14.x-staging
Jul 15, 2021
ConversationCommits (1)ChecksFiles changed
Merged

[v14.x] deps: V8: cherry-pick 81181a8ad80a#39187
richardlau merged 1 commit into
nodejs:v14.x-stagingfrom
thomasmichaelwallace:v14.x-staging

Conversation

thomasmichaelwallace commented Jun 28, 2021

Copy link
Copy Markdown

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 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 commented Jun 29, 2021

Copy link
Copy Markdown
Member

Thanks, it's perfect!

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

mcollina approved these changes Jun 30, 2021
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 30, 2021
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 commented Jun 30, 2021 •
edited by targos
Loading

Copy link
Copy Markdown
Collaborator

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

thomasmichaelwallace commented Jul 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Author

~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 commented Jul 2, 2021

Copy link
Copy Markdown
Collaborator

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

targos commented Jul 15, 2021

Copy link
Copy Markdown
Member

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

This comment has been minimized.

Sign in to view

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

Copy link
Copy Markdown
Collaborator

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 commented Jul 15, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jul 15, 2021

Copy link
Copy Markdown
Collaborator

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

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 merged commit 848cf46 into nodejs:v14.x-staging Jul 15, 2021

richardlau commented Jul 15, 2021

Copy link
Copy Markdown
Member

Landed in 848cf46.

richardlau pushed a commit that referenced this pull request Jul 20, 2021
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 mentioned this pull request Jul 20, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
richardlau richardlau approved these changes
gireeshpunathil gireeshpunathil approved these changes
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

Back | FazBrowse Home | New Git URL