GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — src: fix cppgc incompatibility in v8 by codebytere · Pull Request #43521 · nodejs/node

Conversation

codebytere commented Jun 21, 2022

Copy link

Copy Markdown

Member

This fixes a crash that happens sporadically when Node is used in the same V8 Isolate as Blink.

Example Stacktrace

#
# Fatal error in ../../v8/src/objects/js-objects-inl.h, line 306
# Debug check failed: static_cast<unsigned>(index) < static_cast<unsigned>(GetEmbedderFieldCount()) (1 vs. 1).
#
#
#
#FailureMessage Object: 0x7ffee46fd1c0
0   Electron Framework                  0x00000001181c78d9 base::debug::CollectStackTrace(void**, unsigned long) + 9
1   Electron Framework                  0x00000001180ea633 base::debug::StackTrace::StackTrace() + 19
2   Electron Framework                  0x000000011a04decd gin::(anonymous namespace)::PrintStackTrace() + 45
3   Electron Framework                  0x0000000119a9b416 V8_Fatal(char const*, int, char const*, ...) + 326
4   Electron Framework                  0x0000000119a9aeb5 v8::base::(anonymous namespace)::DefaultDcheckHandler(char const*, int, char const*) + 21
5   Electron Framework                  0x000000011530763f v8::internal::JSObject::GetEmbedderFieldOffset(int) + 207
6   Electron Framework                  0x00000001155f68e6 v8::internal::LocalEmbedderHeapTracer::EmbedderWriteBarrier(v8::internal::Heap*, v8::internal::JSObject) + 150
7   Electron Framework                  0x00000001152cd34f v8::Object::SetAlignedPointerInInternalField(int, void*) + 639
8   Electron Framework                  0x000000011d18df35 node::BaseObject::BaseObject(node::Environment*, v8::Local<v8::Object>) + 101
9   Electron Framework                  0x000000011d347b6e node::crypto::DiffieHellman::DiffieHellman(node::Environment*, v8::Local<v8::Object>) + 14
10  Electron Framework                  0x000000011d348413 node::crypto::DiffieHellman::New(v8::FunctionCallbackInfo<v8::Value> const&) + 147
[...]

This crash is happening because the V8 isolate in this scenario has cppgc enabled. When cppgc is enabled, V8 assumes that the first embedder field is a "type" pointer, the first 16 bits of which are the embedder ID. At the moment, Node.js does not adhere to this requirement. Mostly, this worked by accident. If the first field in the BaseObject was a pointer to a bit of memory that happened to contain the two-byte little-endian value 0x0001, however, V8 would take that to mean that the object was a Blink object1, and attempt to read the pointer in the second embedder slot, which would result in a CHECK.

This change adds an "embedder id" pointer as the first embedder field in all Node-managed objects. This ensures that cppgc will always skip over Node objects.

Upstreamed from our existing patch.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with eyes emoji

All reactions

👀 1 reaction

codebytere requested review from joyeecheung and targos

June 21, 2022 12:14

nodejs-github-bot added c++

Issues and PRs that require attention from people who are familiar with C++.

needs-ci PRs that need a full CI run. labels

Jun 21, 2022

targos commented Jun 21, 2022

Copy link

Copy Markdown

Member

@nodejs/cpp-reviewers

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung reviewed

Jun 21, 2022

View reviewed changes

Comment thread

src/base_object.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/base_object-inl.h Outdated

joyeecheung Jun 21, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Would it make sense to add our ID to https://source.chromium.org/chromium/chromium/src/+/main:gin/public/gin_embedders.h;l=18-23;drc=5a758a97032f0b656c3c36a3497560762495501a ? I imagine there probably should be some kind of directory of V8 embedders in case they step on each other's toes, though not sure if gin is the right place for that directory..

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell reacted with thumbs up emoji

All reactions

👍 1 reaction

joyeecheung commented Jun 21, 2022

Copy link

Copy Markdown

Member

By the way I like this change :) Helps making the checks in the snapshot (de/)serialization callbacks more robust (right now they are just checking whatever in BaseObject::kSlot behaves like a BaseObject, which is quite unreliable)

codebytere and Qard reacted with heart emoji

All reactions

❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

codebytere force-pushed the fix-isolate-overlap-crash branch from d6103ef to c3efda3 Compare

June 21, 2022 20:34

RaisinTen reviewed

Jun 26, 2022

View reviewed changes

Comment thread

src/base_object-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

jasnell approved these changes

Jun 27, 2022

View reviewed changes

codebytere force-pushed the fix-isolate-overlap-crash branch from c3efda3 to a54c7dd Compare

June 28, 2022 10:18

RaisinTen reviewed

Jun 28, 2022

View reviewed changes

RaisinTen left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Not sure why the CHECK in

node/src/env-inl.h

Line 441 in bc5f9e1

CHECK(result.second);

is failing. 🤔

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/base_object-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/base_object-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/base_object.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/base_object-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/base_object-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung commented Jun 30, 2022

Copy link

Copy Markdown

Member

The failed check indicates that the binding being added was already added, which shouldn't have happened? You can try printing T::type_name above the check to figure out which binding it is (they are supposed to be unique for each binding)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

codebytere force-pushed the fix-isolate-overlap-crash branch 2 times, most recently from 312149f to 096447c Compare

June 30, 2022 07:44

RaisinTen approved these changes

Jun 30, 2022

View reviewed changes

RaisinTen left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM % formatting fixes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

codebytere commented Jun 30, 2022

Copy link

Copy Markdown

Member Author

@joyeecheung it looks like node::process::BindingDatais getting logged out 🤔 ?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung commented Jul 12, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

@codebytere I figured it out - the SerializeNodeContextInternalFields() and DeserializeNodeInternalFields() were assuming that they are only ever called on BaseObject::kSlot, which was fine previously, but now that there is another field before that we need to rewrite these two callbacks to work with the new layout. Can I push the fix directly to the PR branch?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

codebytere commented Jul 14, 2022

Copy link

Copy Markdown

Member Author

@joyeecheung sure - go for it :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung force-pushed the fix-isolate-overlap-crash branch from 7997f5f to d49cb4c Compare

July 14, 2022 09:19

joyeecheung commented Jul 14, 2022

Copy link

Copy Markdown

Member

I've also rebased the branch to resolve the merge conflicts

codebytere reacted with heart emoji

All reactions

❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Jul 14, 2022

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung commented Jul 14, 2022

Copy link

Copy Markdown

Member

For some reason, Windows is still testing the code before the patch...retrying the CI

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Jul 14, 2022

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Jul 14, 2022

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot mentioned this pull request

Jul 15, 2022

CI Reliability 2022-07-15 nodejs/reliability#323

Open

24 tasks

nodejs-github-bot commented Jul 15, 2022

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot mentioned this pull request

Jul 19, 2022

CI Reliability 2022-07-19 nodejs/reliability#325

Open

19 tasks

nodejs-github-bot commented Jul 19, 2022

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung commented Jul 21, 2022

Copy link

Copy Markdown

Member

@jasnell @RaisinTen Can you take a look at the patch again as it has changed quite a bit now to work with the snapshot? Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

guangwong pushed a commit to noslate-project/node that referenced this pull request

Jan 3, 2023


          src: fix cppgc incompatibility in v8

d96c4eb

This patch updates the layout of the BaseObjects to make sure
that the first embedder field of them is a "type" pointer, the
first 16 bits of which are the Node.js embedder ID, so that
cppgc will always skip over them. In addition we now use this
field to determine if the native object should be interpreted
as a Node.js embedder object in the serialization and deserialization
callbacks for the startup snapshot to improve the reliability.

Co-authored-by: Joyee Cheung <[email protected]>
PR-URL: nodejs/node#43521
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

guangwong pushed a commit to noslate-project/node that referenced this pull request

Jan 3, 2023


          src: fix cppgc incompatibility in v8

2980e5a

This patch updates the layout of the BaseObjects to make sure
that the first embedder field of them is a "type" pointer, the
first 16 bits of which are the Node.js embedder ID, so that
cppgc will always skip over them. In addition we now use this
field to determine if the native object should be interpreted
as a Node.js embedder object in the serialization and deserialization
callbacks for the startup snapshot to improve the reliability.

Co-authored-by: Joyee Cheung <[email protected]>
PR-URL: nodejs/node#43521
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

danielleadams pushed a commit that referenced this pull request

Jan 3, 2023


          src: fix cppgc incompatibility in v8

f071028

This patch updates the layout of the BaseObjects to make sure
that the first embedder field of them is a "type" pointer, the
first 16 bits of which are the Node.js embedder ID, so that
cppgc will always skip over them. In addition we now use this
field to determine if the native object should be interpreted
as a Node.js embedder object in the serialization and deserialization
callbacks for the startup snapshot to improve the reliability.

Co-authored-by: Joyee Cheung <[email protected]>
PR-URL: #43521
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

codebytere added a commit to electron/electron that referenced this pull request

Jan 9, 2023


          src: fix cppgc incompatibility in v8

abb599f

nodejs/node#43521

codebytere added a commit to electron/electron that referenced this pull request

Jan 9, 2023


          src: fix cppgc incompatibility in v8

1a05ca6

nodejs/node#43521

codebytere added a commit to electron/electron that referenced this pull request

Jan 11, 2023


          chore: bump node to v18.13.0 (main) (#36818)

1d9a4ab

* chore: bump node in DEPS to v18.13.0

* child_process: validate arguments for null bytes

nodejs/node#44782

* bootstrap: merge main thread and worker thread initializations

nodejs/node#44869

* module: ensure relative requires work from deleted directories

nodejs/node#42384

* src: add support for externally shared js builtins

nodejs/node#44000

* lib: disambiguate `native module` to `binding`

nodejs/node#45673

* test: convert test-debugger-pid to async/await

nodejs/node#45179

* deps: upgrade to libuv 1.44.2

nodejs/node#42340

* src: fix cppgc incompatibility in v8

nodejs/node#43521

* src: use qualified `std::move` call in node_http2

nodejs/node#45555

* build: fix env.h for cpp20

nodejs/node#45516

* test: remove experimental-wasm-threads flag

nodejs/node#45074

* src: iwyu in cleanup_queue.cc

nodejs/node#44983

* src: add missing include for `std::all_of`

nodejs/node#45541

* deps: update ICU to 72.1

nodejs/node#45068

* chore: fixup patch indices

* chore: remove errant semicolons

- nodejs/node#44179
- nodejs/node#44193

* src: add support for externally shared js builtins

nodejs/node#44376

* chore: add missing GN filenames

* deps: update nghttp2 to 1.51.0

nodejs/node#45537

* chore: disable more Node.js snapshot tests

The Snapshot feature is currently disabled

* chore: disable ICU timezone tests

Node.js uses a different version of ICU than Electron so they
will often be out of sync.

* chore: disable threadpool event tracing test

Event tracing is not enabled in embedded Node.js

* chore: fixup patch indices

* chore: comments from review

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>

joyeecheung mentioned this pull request

Jan 27, 2023

src,tools: initialize cppgc #45704

Merged

khalwa pushed a commit to solarwindscloud/electron that referenced this pull request

Feb 22, 2023


          chore: bump node to v18.13.0 (main) (electron#36818)

cd481d5

* chore: bump node in DEPS to v18.13.0

* child_process: validate arguments for null bytes

nodejs/node#44782

* bootstrap: merge main thread and worker thread initializations

nodejs/node#44869

* module: ensure relative requires work from deleted directories

nodejs/node#42384

* src: add support for externally shared js builtins

nodejs/node#44000

* lib: disambiguate `native module` to `binding`

nodejs/node#45673

* test: convert test-debugger-pid to async/await

nodejs/node#45179

* deps: upgrade to libuv 1.44.2

nodejs/node#42340

* src: fix cppgc incompatibility in v8

nodejs/node#43521

* src: use qualified `std::move` call in node_http2

nodejs/node#45555

* build: fix env.h for cpp20

nodejs/node#45516

* test: remove experimental-wasm-threads flag

nodejs/node#45074

* src: iwyu in cleanup_queue.cc

nodejs/node#44983

* src: add missing include for `std::all_of`

nodejs/node#45541

* deps: update ICU to 72.1

nodejs/node#45068

* chore: fixup patch indices

* chore: remove errant semicolons

- nodejs/node#44179
- nodejs/node#44193

* src: add support for externally shared js builtins

nodejs/node#44376

* chore: add missing GN filenames

* deps: update nghttp2 to 1.51.0

nodejs/node#45537

* chore: disable more Node.js snapshot tests

The Snapshot feature is currently disabled

* chore: disable ICU timezone tests

Node.js uses a different version of ICU than Electron so they
will often be out of sync.

* chore: disable threadpool event tracing test

Event tracing is not enabled in embedded Node.js

* chore: fixup patch indices

* chore: comments from review

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>

gecko19 pushed a commit to brightsign/electron that referenced this pull request

Feb 28, 2023


          chore: bump node to v18.13.0 (main) (electron#36818)

730c38e

* chore: bump node in DEPS to v18.13.0

* child_process: validate arguments for null bytes

nodejs/node#44782

* bootstrap: merge main thread and worker thread initializations

nodejs/node#44869

* module: ensure relative requires work from deleted directories

nodejs/node#42384

* src: add support for externally shared js builtins

nodejs/node#44000

* lib: disambiguate `native module` to `binding`

nodejs/node#45673

* test: convert test-debugger-pid to async/await

nodejs/node#45179

* deps: upgrade to libuv 1.44.2

nodejs/node#42340

* src: fix cppgc incompatibility in v8

nodejs/node#43521

* src: use qualified `std::move` call in node_http2

nodejs/node#45555

* build: fix env.h for cpp20

nodejs/node#45516

* test: remove experimental-wasm-threads flag

nodejs/node#45074

* src: iwyu in cleanup_queue.cc

nodejs/node#44983

* src: add missing include for `std::all_of`

nodejs/node#45541

* deps: update ICU to 72.1

nodejs/node#45068

* chore: fixup patch indices

* chore: remove errant semicolons

- nodejs/node#44179
- nodejs/node#44193

* src: add support for externally shared js builtins

nodejs/node#44376

* chore: add missing GN filenames

* deps: update nghttp2 to 1.51.0

nodejs/node#45537

* chore: disable more Node.js snapshot tests

The Snapshot feature is currently disabled

* chore: disable ICU timezone tests

Node.js uses a different version of ICU than Electron so they
will often be out of sync.

* chore: disable threadpool event tracing test

Event tracing is not enabled in embedded Node.js

* chore: fixup patch indices

* chore: comments from review

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>

gecko19 pushed a commit to brightsign/electron that referenced this pull request

Mar 15, 2023


          chore: bump node to v18.13.0 (main) (electron#36818)

23a535e

* chore: bump node in DEPS to v18.13.0

* child_process: validate arguments for null bytes

nodejs/node#44782

* bootstrap: merge main thread and worker thread initializations

nodejs/node#44869

* module: ensure relative requires work from deleted directories

nodejs/node#42384

* src: add support for externally shared js builtins

nodejs/node#44000

* lib: disambiguate `native module` to `binding`

nodejs/node#45673

* test: convert test-debugger-pid to async/await

nodejs/node#45179

* deps: upgrade to libuv 1.44.2

nodejs/node#42340

* src: fix cppgc incompatibility in v8

nodejs/node#43521

* src: use qualified `std::move` call in node_http2

nodejs/node#45555

* build: fix env.h for cpp20

nodejs/node#45516

* test: remove experimental-wasm-threads flag

nodejs/node#45074

* src: iwyu in cleanup_queue.cc

nodejs/node#44983

* src: add missing include for `std::all_of`

nodejs/node#45541

* deps: update ICU to 72.1

nodejs/node#45068

* chore: fixup patch indices

* chore: remove errant semicolons

- nodejs/node#44179
- nodejs/node#44193

* src: add support for externally shared js builtins

nodejs/node#44376

* chore: add missing GN filenames

* deps: update nghttp2 to 1.51.0

nodejs/node#45537

* chore: disable more Node.js snapshot tests

The Snapshot feature is currently disabled

* chore: disable ICU timezone tests

Node.js uses a different version of ICU than Electron so they
will often be out of sync.

* chore: disable threadpool event tracing test

Event tracing is not enabled in embedded Node.js

* chore: fixup patch indices

* chore: comments from review

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>

gecko19 pushed a commit to brightsign/electron that referenced this pull request

Mar 15, 2023


          chore: bump node to v18.13.0 (main) (electron#36818)

c6ae563

* chore: bump node in DEPS to v18.13.0

* child_process: validate arguments for null bytes

nodejs/node#44782

* bootstrap: merge main thread and worker thread initializations

nodejs/node#44869

* module: ensure relative requires work from deleted directories

nodejs/node#42384

* src: add support for externally shared js builtins

nodejs/node#44000

* lib: disambiguate `native module` to `binding`

nodejs/node#45673

* test: convert test-debugger-pid to async/await

nodejs/node#45179

* deps: upgrade to libuv 1.44.2

nodejs/node#42340

* src: fix cppgc incompatibility in v8

nodejs/node#43521

* src: use qualified `std::move` call in node_http2

nodejs/node#45555

* build: fix env.h for cpp20

nodejs/node#45516

* test: remove experimental-wasm-threads flag

nodejs/node#45074

* src: iwyu in cleanup_queue.cc

nodejs/node#44983

* src: add missing include for `std::all_of`

nodejs/node#45541

* deps: update ICU to 72.1

nodejs/node#45068

* chore: fixup patch indices

* chore: remove errant semicolons

- nodejs/node#44179
- nodejs/node#44193

* src: add support for externally shared js builtins

nodejs/node#44376

* chore: add missing GN filenames

* deps: update nghttp2 to 1.51.0

nodejs/node#45537

* chore: disable more Node.js snapshot tests

The Snapshot feature is currently disabled

* chore: disable ICU timezone tests

Node.js uses a different version of ICU than Electron so they
will often be out of sync.

* chore: disable threadpool event tracing test

Event tracing is not enabled in embedded Node.js

* chore: fixup patch indices

* chore: comments from review

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>

pull Bot pushed a commit to jamlee-t/v8 that referenced this pull request

Jun 27, 2023


          [cppgc] expose wrapper descriptor on CppHeap

This makes it possible for embedders to:

1. Avoid creating wrapper objects that happen to have a layout that
  leads V8 to consider the object cppgc-managed while it's not.
  Refs: nodejs/node#43521
2. Create cppgc-managed wrapper objects when they do not own the
   CppHeap. Refs: nodejs/node#45704

Bug: v8:13960
Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833
Reviewed-by: Michael Lippautz <[email protected]>
Commit-Queue: Michael Lippautz <[email protected]>
Cr-Commit-Position: refs/heads/main@{#88490}

joyeecheung added a commit to joyeecheung/node that referenced this pull request

Jun 30, 2023


          deps: V8: cherry-pick 93275031284c

d35c6ca

Original commit message:

    [cppgc] expose wrapper descriptor on CppHeap

    This makes it possible for embedders to:

    1. Avoid creating wrapper objects that happen to have a layout that
      leads V8 to consider the object cppgc-managed while it's not.
      Refs: nodejs#43521
    2. Create cppgc-managed wrapper objects when they do not own the
       CppHeap. Refs: nodejs#45704

    Bug: v8:13960
    Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#88490}

Refs: v8/v8@9327503

joyeecheung added a commit to joyeecheung/node that referenced this pull request

Jul 5, 2023


          deps: V8: cherry-pick 93275031284c

a8820d5

Original commit message:

    [cppgc] expose wrapper descriptor on CppHeap

    This makes it possible for embedders to:

    1. Avoid creating wrapper objects that happen to have a layout that
      leads V8 to consider the object cppgc-managed while it's not.
      Refs: nodejs#43521
    2. Create cppgc-managed wrapper objects when they do not own the
       CppHeap. Refs: nodejs#45704

    Bug: v8:13960
    Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#88490}

Refs: v8/v8@9327503

joyeecheung added a commit to joyeecheung/node that referenced this pull request

Jul 20, 2023


          deps: V8: cherry-pick 93275031284c

30573b2

Original commit message:

    [cppgc] expose wrapper descriptor on CppHeap

    This makes it possible for embedders to:

    1. Avoid creating wrapper objects that happen to have a layout that
      leads V8 to consider the object cppgc-managed while it's not.
      Refs: nodejs#43521
    2. Create cppgc-managed wrapper objects when they do not own the
       CppHeap. Refs: nodejs#45704

    Bug: v8:13960
    Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#88490}

Refs: v8/v8@9327503

joyeecheung added a commit that referenced this pull request

Jul 21, 2023


          deps: V8: cherry-pick 93275031284c

44027fb

Original commit message:

    [cppgc] expose wrapper descriptor on CppHeap

    This makes it possible for embedders to:

    1. Avoid creating wrapper objects that happen to have a layout that
      leads V8 to consider the object cppgc-managed while it's not.
      Refs: #43521
    2. Create cppgc-managed wrapper objects when they do not own the
       CppHeap. Refs: #45704

    Bug: v8:13960
    Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#88490}

Refs: v8/v8@9327503
PR-URL: #48660
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>

targos pushed a commit to targos/node that referenced this pull request

Jul 31, 2023


          deps: V8: cherry-pick 93275031284c

70639b7

Original commit message:

    [cppgc] expose wrapper descriptor on CppHeap

    This makes it possible for embedders to:

    1. Avoid creating wrapper objects that happen to have a layout that
      leads V8 to consider the object cppgc-managed while it's not.
      Refs: nodejs#43521
    2. Create cppgc-managed wrapper objects when they do not own the
       CppHeap. Refs: nodejs#45704

    Bug: v8:13960
    Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#88490}

Refs: v8/v8@9327503
PR-URL: nodejs#48660
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>

Ceres6 pushed a commit to Ceres6/node that referenced this pull request

Aug 14, 2023


          deps: V8: cherry-pick 93275031284c

06a0bb1

Original commit message:

    [cppgc] expose wrapper descriptor on CppHeap

    This makes it possible for embedders to:

    1. Avoid creating wrapper objects that happen to have a layout that
      leads V8 to consider the object cppgc-managed while it's not.
      Refs: nodejs#43521
    2. Create cppgc-managed wrapper objects when they do not own the
       CppHeap. Refs: nodejs#45704

    Bug: v8:13960
    Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#88490}

Refs: v8/v8@9327503
PR-URL: nodejs#48660
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>

Ceres6 pushed a commit to Ceres6/node that referenced this pull request

Aug 14, 2023


          deps: V8: cherry-pick 93275031284c

0b3a087

Original commit message:

    [cppgc] expose wrapper descriptor on CppHeap

    This makes it possible for embedders to:

    1. Avoid creating wrapper objects that happen to have a layout that
      leads V8 to consider the object cppgc-managed while it's not.
      Refs: nodejs#43521
    2. Create cppgc-managed wrapper objects when they do not own the
       CppHeap. Refs: nodejs#45704

    Bug: v8:13960
    Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#88490}

Refs: v8/v8@9327503
PR-URL: nodejs#48660
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>

joyeecheung added a commit to joyeecheung/node that referenced this pull request

Aug 15, 2023


          deps: V8: cherry-pick 93275031284c

aa1a676

Original commit message:

    [cppgc] expose wrapper descriptor on CppHeap

    This makes it possible for embedders to:

    1. Avoid creating wrapper objects that happen to have a layout that
      leads V8 to consider the object cppgc-managed while it's not.
      Refs: nodejs#43521
    2. Create cppgc-managed wrapper objects when they do not own the
       CppHeap. Refs: nodejs#45704

    Bug: v8:13960
    Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#88490}

Refs: v8/v8@9327503
PR-URL: nodejs#48660
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>

joyeecheung added a commit to joyeecheung/node that referenced this pull request

Aug 17, 2023


          deps: V8: cherry-pick 93275031284c

eba74b7

Original commit message:

    [cppgc] expose wrapper descriptor on CppHeap

    This makes it possible for embedders to:

    1. Avoid creating wrapper objects that happen to have a layout that
      leads V8 to consider the object cppgc-managed while it's not.
      Refs: nodejs#43521
    2. Create cppgc-managed wrapper objects when they do not own the
       CppHeap. Refs: nodejs#45704

    Bug: v8:13960
    Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#88490}

Refs: v8/v8@9327503
PR-URL: nodejs#48660
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>

Reviewers

joyeecheung

joyeecheung left review comments

jasnell

jasnell approved these changes

RaisinTen

RaisinTen approved these changes

targos

Awaiting requested review from targos

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++

Issues and PRs that require attention from people who are familiar with C++.

commit-queue-failed

PRs whose Commit Queue landing failed and need manual intervention before retrying.

commit-queue-squash

PRs the Commit Queue should land as one squashed commit.

needs-ci

PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

codebytere commented Jun 21, 2022

Uh oh!

targos commented Jun 21, 2022

Uh oh!

Uh oh!

joyeecheung Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jun 21, 2022

Uh oh!

Uh oh!

RaisinTen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joyeecheung commented Jun 30, 2022

Uh oh!

RaisinTen left a comment

Choose a reason for hiding this comment

Uh oh!

codebytere commented Jun 30, 2022

Uh oh!

joyeecheung commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codebytere commented Jul 14, 2022

Uh oh!

joyeecheung commented Jul 14, 2022

Uh oh!

nodejs-github-bot commented Jul 14, 2022

Uh oh!

joyeecheung commented Jul 14, 2022

Uh oh!

nodejs-github-bot commented Jul 14, 2022

Uh oh!

nodejs-github-bot commented Jul 14, 2022

Uh oh!

nodejs-github-bot commented Jul 15, 2022

Uh oh!

nodejs-github-bot commented Jul 19, 2022

Uh oh!

joyeecheung commented Jul 21, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

joyeecheung commented Jul 12, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.