Skip to content

src: fix cppgc incompatibility in v8#43521

Closed
codebytere wants to merge 3 commits intonodejs:mainfrom
codebytere:fix-isolate-overlap-crash
Closed

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

Conversation

@codebytere
Copy link
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.

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.

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants