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

deps: V8: backport 0aa622e12893 #49419

Merged
merged 1 commit into from
Sep 11, 2023
Merged

deps: V8: backport 0aa622e12893 #49419

merged 1 commit into from
Sep 11, 2023

Conversation

joyeecheung
Copy link
Member

Original commit message:

[api] allow v8::Data as internal field

Previously only v8::Value can be stored as internal fields.
In some cases, however, it's necessary for the embedder to
tie the lifetime of a v8::Data with the lifetime of a
JS object, and that v8::Data may not be a v8::Value, as
it can be something returned from the V8 API. One way to
keep the v8::Data alive may be to use a v8::Persistent<v8::Data>
but that can easily lead to leaks.

This patch changes v8::Object::GetInternalField() and
v8::Object::SetInernalField() to accept v8::Data instead of just
v8::Value, so that v8::Data can kept alive by a JS object in
a way that the GC can be aware of to address this problem.
This is a breaking change for embedders
using v8::Object::GetInternalField() as it changes the return
type. Since most v8::Value subtypes only support direct casts
from v8::Value but not v8::Data, calls like

object->GetInternalField(index).As<v8::External>()

needs to be updated to cast the value to v8::Value first:

object->GetInternalField(index).As<v8::Value>().As<v8::External>()

Bug: v8:14120
Change-Id: I731c958d1756b9d5ee4a3e78813416cd60d1b7ca
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4707972
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/main@{#89718}

Refs: v8/v8@0aa622e

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added 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. labels Aug 31, 2023
@joyeecheung joyeecheung mentioned this pull request Aug 31, 2023
Closed
@joyeecheung joyeecheung added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Aug 31, 2023
@joyeecheung
Copy link
Member Author

For v20.x this needs a manual non-ABI-breaking backport (probably by doing a workaround to add a similarly-named method with the signature, instead of just breaking it).

@joyeecheung
Copy link
Member Author

Uh-oh, this was just reverted in the upstream. I'll investigate the crash that caused the revert. Marking it as blocked for now..

@joyeecheung joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Aug 31, 2023
@joyeecheung
Copy link
Member Author

Reland opened in the upstream: https://chromium-review.googlesource.com/c/v8/v8/+/4834471

@joyeecheung
deps: V8: backport 93b1a74cbc9b
d08e689 
Original commit message:

    Reland "[api] allow v8::Data as internal field"

    This is a reland of commit 0aa622e12893e9921c01a34ce9507b544e599c4a

    The original patch tried to run a test that calls exit() in the
    fatal error handler in parallel, which would not work. This marked
    the test with TEST() to avoid running it in parallel.

    Original change's description:
    > [api] allow v8::Data as internal field
    >
    > Previously only v8::Value can be stored as internal fields.
    > In some cases, however, it's necessary for the embedder to
    > tie the lifetime of a v8::Data with the lifetime of a
    > JS object, and that v8::Data may not be a v8::Value, as
    > it can be something returned from the V8 API. One way to
    > keep the v8::Data alive may be to use a v8::Persistent<v8::Data>
    > but that can easily lead to leaks.
    >
    > This patch changes v8::Object::GetInternalField() and
    > v8::Object::SetInernalField() to accept v8::Data instead of just
    > v8::Value, so that v8::Data can kept alive by a JS object in
    > a way that the GC can be aware of to address this problem.
    > This is a breaking change for embedders
    > using v8::Object::GetInternalField() as it changes the return
    > type. Since most v8::Value subtypes only support direct casts
    > from v8::Value but not v8::Data, calls like
    >
    > object->GetInternalField(index).As<v8::External>()
    >
    > needs to be updated to cast the value to v8::Value first:
    >
    > object->GetInternalField(index).As<v8::Value>().As<v8::External>()
    >
    > Bug: v8:14120
    > Change-Id: I731c958d1756b9d5ee4a3e78813416cd60d1b7ca
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4707972
    > Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    > Commit-Queue: Joyee Cheung <joyee@igalia.com>
    > Cr-Commit-Position: refs/heads/main@{#89718}

    Bug: v8:14120
    Change-Id: I3e45d09b5c300d5eefc73e380ef21ac2bd61760c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4834471
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#89824}

Refs: v8/v8@93b1a74
@joyeecheung
Copy link
Member Author

Updated to backport the reland instead - it's been in V8 main branch for a few days, should be safe to merge back now.

@joyeecheung joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed blocked PRs that are blocked by other issues or PRs. labels Sep 8, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/5547/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5547/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5547/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5548/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/5548/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5548/

@github-actions github-actions bot mentioned this pull request Sep 9, 2023
Open
24 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Sep 10, 2023
Open
19 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@anonrig anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 10, 2023
@github-actions github-actions bot mentioned this pull request Sep 11, 2023
Open
18 tasks
@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 11, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 11, 2023
@nodejs-github-bot nodejs-github-bot merged commit f970087 into nodejs:main Sep 11, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in f970087

@joyeecheung joyeecheung mentioned this pull request Sep 11, 2023
Open
This was referenced Sep 12, 2023
Open
Open
@joyeecheung joyeecheung added the dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. label Sep 13, 2023
@joyeecheung joyeecheung mentioned this pull request Sep 15, 2023
Closed
joyeecheung added a commit to joyeecheung/node that referenced this pull request Sep 26, 2023
@joyeecheung
deps: V8: backport 93b1a74cbc9b
6ced2ae 
Original commit message:

    Reland "[api] allow v8::Data as internal field"

    This is a reland of commit 0aa622e12893e9921c01a34ce9507b544e599c4a

    The original patch tried to run a test that calls exit() in the
    fatal error handler in parallel, which would not work. This marked
    the test with TEST() to avoid running it in parallel.

    Original change's description:
    > [api] allow v8::Data as internal field
    >
    > Previously only v8::Value can be stored as internal fields.
    > In some cases, however, it's necessary for the embedder to
    > tie the lifetime of a v8::Data with the lifetime of a
    > JS object, and that v8::Data may not be a v8::Value, as
    > it can be something returned from the V8 API. One way to
    > keep the v8::Data alive may be to use a v8::Persistent<v8::Data>
    > but that can easily lead to leaks.
    >
    > This patch changes v8::Object::GetInternalField() and
    > v8::Object::SetInernalField() to accept v8::Data instead of just
    > v8::Value, so that v8::Data can kept alive by a JS object in
    > a way that the GC can be aware of to address this problem.
    > This is a breaking change for embedders
    > using v8::Object::GetInternalField() as it changes the return
    > type. Since most v8::Value subtypes only support direct casts
    > from v8::Value but not v8::Data, calls like
    >
    > object->GetInternalField(index).As<v8::External>()
    >
    > needs to be updated to cast the value to v8::Value first:
    >
    > object->GetInternalField(index).As<v8::Value>().As<v8::External>()
    >
    > Bug: v8:14120
    > Change-Id: I731c958d1756b9d5ee4a3e78813416cd60d1b7ca
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4707972
    > Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    > Commit-Queue: Joyee Cheung <joyee@igalia.com>
    > Cr-Commit-Position: refs/heads/main@{#89718}

    Bug: v8:14120
    Change-Id: I3e45d09b5c300d5eefc73e380ef21ac2bd61760c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4834471
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#89824}

Refs: v8/v8@93b1a74
PR-URL: nodejs#49419
Refs: v8/v8@0aa622e
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@joyeecheung joyeecheung mentioned this pull request Sep 26, 2023
Closed
@RafaelGSS RafaelGSS mentioned this pull request Oct 9, 2023
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@joyeecheung @alexfernandez
deps: V8: backport 93b1a74cbc9b
57bdd23 
Original commit message:

    Reland "[api] allow v8::Data as internal field"

    This is a reland of commit 0aa622e12893e9921c01a34ce9507b544e599c4a

    The original patch tried to run a test that calls exit() in the
    fatal error handler in parallel, which would not work. This marked
    the test with TEST() to avoid running it in parallel.

    Original change's description:
    > [api] allow v8::Data as internal field
    >
    > Previously only v8::Value can be stored as internal fields.
    > In some cases, however, it's necessary for the embedder to
    > tie the lifetime of a v8::Data with the lifetime of a
    > JS object, and that v8::Data may not be a v8::Value, as
    > it can be something returned from the V8 API. One way to
    > keep the v8::Data alive may be to use a v8::Persistent<v8::Data>
    > but that can easily lead to leaks.
    >
    > This patch changes v8::Object::GetInternalField() and
    > v8::Object::SetInernalField() to accept v8::Data instead of just
    > v8::Value, so that v8::Data can kept alive by a JS object in
    > a way that the GC can be aware of to address this problem.
    > This is a breaking change for embedders
    > using v8::Object::GetInternalField() as it changes the return
    > type. Since most v8::Value subtypes only support direct casts
    > from v8::Value but not v8::Data, calls like
    >
    > object->GetInternalField(index).As<v8::External>()
    >
    > needs to be updated to cast the value to v8::Value first:
    >
    > object->GetInternalField(index).As<v8::Value>().As<v8::External>()
    >
    > Bug: v8:14120
    > Change-Id: I731c958d1756b9d5ee4a3e78813416cd60d1b7ca
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4707972
    > Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    > Commit-Queue: Joyee Cheung <joyee@igalia.com>
    > Cr-Commit-Position: refs/heads/main@{#89718}

    Bug: v8:14120
    Change-Id: I3e45d09b5c300d5eefc73e380ef21ac2bd61760c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4834471
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#89824}

Refs: v8/v8@93b1a74
PR-URL: nodejs#49419
Refs: v8/v8@0aa622e
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gengjiawen gengjiawen gengjiawen approved these changes

@anonrig anonrig anonrig approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. 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.
4 participants
@joyeecheung @nodejs-github-bot @anonrig @gengjiawen