src: fix cppgc incompatibility in v8 by codebytere · Pull Request #43521 · nodejs/node · GitHub

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.

See also: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/v8-cppgc.h;l=70-76;drc=5a758a97032f0b656c3c36a3497560762495501a

Upstreamed from our existing patch.

@nodejs/cpp-reviewers

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..

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)

Not sure why the CHECK in

node/src/env-inl.h

Line 441 in bc5f9e1

CHECK(result.second);

is failing. 🤔

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)

LGTM % formatting fixes

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

@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?

@joyeecheung sure - go for it :)

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

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

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

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

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

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

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

@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!

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 <joyeec9h3@gmail.com> PR-URL: nodejs/node#43521 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

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 <joyeec9h3@gmail.com> PR-URL: #43521 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

nodejs/node#43521

* 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 <shelley.vohr@gmail.com>

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 <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490}

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 <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503

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 <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503 PR-URL: #48660 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

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 <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503 PR-URL: nodejs#48660 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

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

joyeecheung reviewed Jun 21, 2022

View reviewed changes

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 Outdated

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

Comment thread src/base_object-inl.h Outdated

Comment thread src/base_object-inl.h Outdated

Comment thread src/base_object.h Outdated

Comment thread src/base_object-inl.h Outdated

Comment thread src/base_object-inl.h Outdated

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

joyeecheung force-pushed the fix-isolate-overlap-crash branch from 7997f5f to d49cb4c Compare July 14, 2022 09:19

github-actions Bot mentioned this pull request Jul 15, 2022

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

Open

24 tasks

github-actions Bot mentioned this pull request Jul 19, 2022

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

Open

19 tasks

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

joyeecheung mentioned this pull request Jan 27, 2023

src,tools: initialize cppgc #45704

Merged

src: fix cppgc incompatibility in v8 by codebytere · Pull Request #43521 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: fix cppgc incompatibility in v8 - #43521

src: fix cppgc incompatibility in v8#43521codebytere wants to merge 3 commits intonodejs:mainnodejs/node:mainfrom codebytere:fix-isolate-overlap-crashcodebytere/node:fix-isolate-overlap-crashCopy head branch name to clipboard

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src: fix cppgc incompatibility in v8#43521
codebytere wants to merge 3 commits into
nodejs:mainfrom
codebytere:fix-isolate-overlap-crash

joyeecheung commented Jul 12, 2022 •
edited
Loading