src: fix cppgc incompatibility in v8 #43521

codebytere · 2022-06-21T12:14:32Z

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.

targos · 2022-06-21T12:32:18Z

@nodejs/cpp-reviewers

src/base_object.h

joyeecheung · 2022-06-21T12:39:19Z

src/base_object-inl.h

+// Otherwise, when Node is loaded in an isolate which uses cppgc, cppgc will
+// misinterpret the data stored in the embedder fields and try to garbage
+// collect them.
+static uint16_t kNodeEmbedderId = 0x90de;


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

joyeecheung · 2022-06-21T12:43:03Z

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)

src/base_object-inl.h

RaisinTen

Not sure why the CHECK in

node/src/env-inl.h

Line 441 in bc5f9e1

CHECK(result.second);

is failing. 🤔

src/base_object-inl.h

src/base_object.h

src/base_object-inl.h

joyeecheung · 2022-06-30T07:20:43Z

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)

RaisinTen

LGTM % formatting fixes

codebytere · 2022-06-30T07:58:05Z

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

joyeecheung · 2022-07-12T14:38:37Z

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

codebytere · 2022-07-14T07:25:51Z

@joyeecheung sure - go for it :)

joyeecheung · 2022-07-14T09:20:12Z

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

nodejs-github-bot · 2022-07-14T09:20:13Z

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

joyeecheung · 2022-07-14T11:24:11Z

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

nodejs-github-bot · 2022-07-14T11:24:36Z

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

nodejs-github-bot · 2022-07-14T15:00:56Z

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

nodejs-github-bot · 2022-07-15T07:22:08Z

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

nodejs-github-bot · 2022-07-19T09:09:24Z

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

joyeecheung · 2022-07-21T07:21:43Z

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

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 Backport-PR-URL: #49187 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit

codebytere requested review from targos and joyeecheung 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

src/base_object-inl.h Outdated Show resolved Hide resolved

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

src/base_object-inl.h Outdated Show resolved Hide resolved

src/base_object-inl.h Outdated Show resolved Hide resolved

src/base_object.h Outdated Show resolved Hide resolved

src/base_object-inl.h Outdated Show resolved Hide resolved

src/base_object-inl.h Outdated Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: fix cppgc incompatibility in v8 #43521

src: fix cppgc incompatibility in v8 #43521

codebytere commented Jun 21, 2022

targos commented Jun 21, 2022

joyeecheung Jun 21, 2022

joyeecheung commented Jun 21, 2022

RaisinTen left a comment

joyeecheung commented Jun 30, 2022

RaisinTen left a comment

codebytere commented Jun 30, 2022

joyeecheung commented Jul 12, 2022 •

edited

codebytere commented Jul 14, 2022

joyeecheung commented Jul 14, 2022

nodejs-github-bot commented Jul 14, 2022

joyeecheung commented Jul 14, 2022

nodejs-github-bot commented Jul 14, 2022

nodejs-github-bot commented Jul 14, 2022

nodejs-github-bot commented Jul 15, 2022

nodejs-github-bot commented Jul 19, 2022

joyeecheung commented Jul 21, 2022

src: fix cppgc incompatibility in v8 #43521

src: fix cppgc incompatibility in v8 #43521

Conversation

codebytere commented Jun 21, 2022

targos commented Jun 21, 2022

joyeecheung Jun 21, 2022

Choose a reason for hiding this comment

joyeecheung commented Jun 21, 2022

RaisinTen left a comment

Choose a reason for hiding this comment

joyeecheung commented Jun 30, 2022

RaisinTen left a comment

Choose a reason for hiding this comment

codebytere commented Jun 30, 2022

joyeecheung commented Jul 12, 2022 • edited

codebytere commented Jul 14, 2022

joyeecheung commented Jul 14, 2022

nodejs-github-bot commented Jul 14, 2022

joyeecheung commented Jul 14, 2022

nodejs-github-bot commented Jul 14, 2022

nodejs-github-bot commented Jul 14, 2022

nodejs-github-bot commented Jul 15, 2022

nodejs-github-bot commented Jul 19, 2022

joyeecheung commented Jul 21, 2022

joyeecheung commented Jul 12, 2022 •

edited