From 7054ce143bc83188caa4116c511054694f2b2e9a Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Fri, 11 Mar 2022 11:25:21 -0800 Subject: [PATCH 1/2] fix: cppgc/node collisions in renderer process --- patches/node/.patches | 1 + patches/node/be_compatible_with_cppgc.patch | 95 +++++++++++++++++++++ 2 files changed, 96 insertions(+) create mode 100644 patches/node/be_compatible_with_cppgc.patch diff --git a/patches/node/.patches b/patches/node/.patches index 3dd50b85a8588..c73c85f1b3fd9 100644 --- a/patches/node/.patches +++ b/patches/node/.patches @@ -32,3 +32,4 @@ darwin_translate_eprototype_to_econnreset_3413.patch darwin_bump_minimum_supported_version_to_10_15_3406.patch fix_serdes_test.patch fix_failing_node_js_test_on_outdated.patch +be_compatible_with_cppgc.patch diff --git a/patches/node/be_compatible_with_cppgc.patch b/patches/node/be_compatible_with_cppgc.patch new file mode 100644 index 0000000000000..a03ea126dbb95 --- /dev/null +++ b/patches/node/be_compatible_with_cppgc.patch @@ -0,0 +1,95 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Jeremy Rose +Date: Fri, 11 Mar 2022 11:17:29 -0800 +Subject: be compatible with cppgc + +This fixes a crash that happens sporadically when Node is used in the same V8 +Isolate as Blink. For example: + +# +# Fatal error in ../../v8/src/objects/js-objects-inl.h, line 306 +# Debug check failed: static_cast(index) < static_cast(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) + 101 +9 Electron Framework 0x000000011d347b6e node::crypto::DiffieHellman::DiffieHellman(node::Environment*, v8::Local) + 14 +10 Electron Framework 0x000000011d348413 node::crypto::DiffieHellman::New(v8::FunctionCallbackInfo const&) + 147 +[...] + +This crash happens because this V8 isolate 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. Node did not adhere to this +requirement. Sometimes--mostly, even--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 object[1], 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. + +[1]: https://source.chromium.org/chromium/chromium/src/+/main:gin/public/gin_embedders.h;l=20;drc=5a758a97032f0b656c3c36a3497560762495501a + +See also: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/v8-cppgc.h;l=70-76;drc=5a758a97032f0b656c3c36a3497560762495501a + +diff --git a/src/base_object-inl.h b/src/base_object-inl.h +index bb1e8d4b46bce3bf08f730ac5d43f7113d17ae39..6da0669943fc6465ffc47a1c8c3dadfea6beb1c9 100644 +--- a/src/base_object-inl.h ++++ b/src/base_object-inl.h +@@ -32,10 +32,21 @@ + + namespace node { + ++namespace { ++// This just has to be different from the Chromium ones: ++// https://source.chromium.org/chromium/chromium/src/+/main:gin/public/gin_embedders.h;l=18-23;drc=5a758a97032f0b656c3c36a3497560762495501a ++// 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; ++} ++ + BaseObject::BaseObject(Environment* env, v8::Local object) + : persistent_handle_(env->isolate(), object), env_(env) { + CHECK_EQ(false, object.IsEmpty()); +- CHECK_GT(object->InternalFieldCount(), 0); ++ CHECK_GT(object->InternalFieldCount(), BaseObject::kSlot); ++ object->SetAlignedPointerInInternalField(BaseObject::kWrapperType, ++ &kNodeEmbedderId); + object->SetAlignedPointerInInternalField( + BaseObject::kSlot, + static_cast(this)); +@@ -151,7 +162,8 @@ bool BaseObject::IsWeakOrDetached() const { + void BaseObject::LazilyInitializedJSTemplateConstructor( + const v8::FunctionCallbackInfo& args) { + DCHECK(args.IsConstructCall()); +- DCHECK_GT(args.This()->InternalFieldCount(), 0); ++ DCHECK_GT(args.This()->InternalFieldCount(), BaseObject::kSlot); ++ args.This()->SetAlignedPointerInInternalField(BaseObject::kWrapperType, &kNodeEmbedderId); + args.This()->SetAlignedPointerInInternalField(BaseObject::kSlot, nullptr); + } + +diff --git a/src/base_object.h b/src/base_object.h +index d46a0f216009c63f45c440fc352b54d1ac4a08d8..81913c0d7762bf499ee19aaa3b63b986ca370bb4 100644 +--- a/src/base_object.h ++++ b/src/base_object.h +@@ -40,7 +40,7 @@ class TransferData; + + class BaseObject : public MemoryRetainer { + public: +- enum InternalFields { kSlot, kInternalFieldCount }; ++ enum InternalFields { kWrapperType, kSlot, kInternalFieldCount }; + + // Associates this object with `object`. It uses the 0th internal field for + // that, and in particular aborts if there is no such field. From 693cf0ebdebb272494610a5ed2e9c405e825813e Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Fri, 11 Mar 2022 11:27:05 -0800 Subject: [PATCH 2/2] Update be_compatible_with_cppgc.patch --- patches/node/be_compatible_with_cppgc.patch | 2 ++ 1 file changed, 2 insertions(+) diff --git a/patches/node/be_compatible_with_cppgc.patch b/patches/node/be_compatible_with_cppgc.patch index a03ea126dbb95..3bbc3ce0bde16 100644 --- a/patches/node/be_compatible_with_cppgc.patch +++ b/patches/node/be_compatible_with_cppgc.patch @@ -39,6 +39,8 @@ 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. +This patch should be upstreamed to Node. + [1]: https://source.chromium.org/chromium/chromium/src/+/main:gin/public/gin_embedders.h;l=20;drc=5a758a97032f0b656c3c36a3497560762495501a See also: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/v8-cppgc.h;l=70-76;drc=5a758a97032f0b656c3c36a3497560762495501a