From 8b0e5b19bd053714b3ff4e1d9e8743a13e463039 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 21 Jun 2022 14:08:41 +0200 Subject: [PATCH] src: fix cppgc incompatibility in v8 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 PR-URL: https://github.com/nodejs/node/pull/43521 Reviewed-By: James M Snell Reviewed-By: Darshan Sen --- src/base_object.h | 6 +++-- src/env.cc | 16 +++++++++++-- src/node_blob.cc | 4 ++-- src/node_file.cc | 4 ++-- src/node_process_methods.cc | 4 ++-- src/node_snapshotable.cc | 46 ++++++++++++++++++++++++++++++++----- src/node_snapshotable.h | 5 ---- src/node_v8.cc | 4 ++-- 8 files changed, 66 insertions(+), 23 deletions(-) diff --git a/src/base_object.h b/src/base_object.h index 842f763a56d75c..6f072bf5ec0357 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -38,11 +38,13 @@ namespace worker { class TransferData; } +extern uint16_t kNodeEmbedderId; + class BaseObject : public MemoryRetainer { public: - enum InternalFields { kSlot, kInternalFieldCount }; + enum InternalFields { kEmbedderType, kSlot, kInternalFieldCount }; - // Associates this object with `object`. It uses the 0th internal field for + // Associates this object with `object`. It uses the 1st internal field for // that, and in particular aborts if there is no such field. BaseObject(Environment* env, v8::Local object); ~BaseObject() override; diff --git a/src/env.cc b/src/env.cc index 83f65f2ad20dba..d2a3390d4356cb 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1744,6 +1744,7 @@ void Environment::EnqueueDeserializeRequest(DeserializeRequestCallback cb, Local holder, int index, InternalFieldInfo* info) { + DCHECK_EQ(index, BaseObject::kEmbedderType); DeserializeRequest request{cb, {isolate(), holder}, index, info}; deserialize_requests_.push_back(std::move(request)); } @@ -2026,7 +2027,9 @@ void Environment::RunWeakRefCleanup() { BaseObject::BaseObject(Environment* env, Local object) : persistent_handle_(env->isolate(), object), env_(env) { CHECK_EQ(false, object.IsEmpty()); - CHECK_GT(object->InternalFieldCount(), 0); + CHECK_GE(object->InternalFieldCount(), BaseObject::kInternalFieldCount); + object->SetAlignedPointerInInternalField(BaseObject::kEmbedderType, + &kNodeEmbedderId); object->SetAlignedPointerInInternalField(BaseObject::kSlot, static_cast(this)); env->AddCleanupHook(DeleteMe, static_cast(this)); @@ -2077,10 +2080,19 @@ void BaseObject::MakeWeak() { WeakCallbackType::kParameter); } +// 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. +uint16_t kNodeEmbedderId = 0x90de; + void BaseObject::LazilyInitializedJSTemplateConstructor( const FunctionCallbackInfo& args) { DCHECK(args.IsConstructCall()); - DCHECK_GT(args.This()->InternalFieldCount(), 0); + CHECK_GE(args.This()->InternalFieldCount(), BaseObject::kInternalFieldCount); + args.This()->SetAlignedPointerInInternalField(BaseObject::kEmbedderType, + &kNodeEmbedderId); args.This()->SetAlignedPointerInInternalField(BaseObject::kSlot, nullptr); } diff --git a/src/node_blob.cc b/src/node_blob.cc index b319a74ebaedbf..b2ca35ab3bb02f 100644 --- a/src/node_blob.cc +++ b/src/node_blob.cc @@ -467,7 +467,7 @@ void BlobBindingData::Deserialize( Local holder, int index, InternalFieldInfo* info) { - DCHECK_EQ(index, BaseObject::kSlot); + DCHECK_EQ(index, BaseObject::kEmbedderType); HandleScope scope(context->GetIsolate()); Environment* env = Environment::GetCurrent(context); BlobBindingData* binding = @@ -482,7 +482,7 @@ void BlobBindingData::PrepareForSerialization( } InternalFieldInfo* BlobBindingData::Serialize(int index) { - DCHECK_EQ(index, BaseObject::kSlot); + DCHECK_EQ(index, BaseObject::kEmbedderType); InternalFieldInfo* info = InternalFieldInfo::New(type()); return info; } diff --git a/src/node_file.cc b/src/node_file.cc index 8815bd8d52e190..5d3eebd1d6edfa 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2401,7 +2401,7 @@ void BindingData::Deserialize(Local context, Local holder, int index, InternalFieldInfo* info) { - DCHECK_EQ(index, BaseObject::kSlot); + DCHECK_EQ(index, BaseObject::kEmbedderType); HandleScope scope(context->GetIsolate()); Environment* env = Environment::GetCurrent(context); BindingData* binding = env->AddBindingData(context, holder); @@ -2418,7 +2418,7 @@ void BindingData::PrepareForSerialization(Local context, } InternalFieldInfo* BindingData::Serialize(int index) { - DCHECK_EQ(index, BaseObject::kSlot); + DCHECK_EQ(index, BaseObject::kEmbedderType); InternalFieldInfo* info = InternalFieldInfo::New(type()); return info; } diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc index 9d4a5abb0163b6..ae5f38ebf2c6b6 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -532,7 +532,7 @@ void BindingData::PrepareForSerialization(Local context, } InternalFieldInfo* BindingData::Serialize(int index) { - DCHECK_EQ(index, BaseObject::kSlot); + DCHECK_EQ(index, BaseObject::kEmbedderType); InternalFieldInfo* info = InternalFieldInfo::New(type()); return info; } @@ -541,7 +541,7 @@ void BindingData::Deserialize(Local context, Local holder, int index, InternalFieldInfo* info) { - DCHECK_EQ(index, BaseObject::kSlot); + DCHECK_EQ(index, BaseObject::kEmbedderType); v8::HandleScope scope(context->GetIsolate()); Environment* env = Environment::GetCurrent(context); // Recreate the buffer in the constructor. diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 9770d7bc647cb6..a30853aab58f39 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -1089,10 +1089,20 @@ void DeserializeNodeInternalFields(Local holder, static_cast(index), (*holder), static_cast(payload.raw_size)); + + if (payload.raw_size == 0) { + holder->SetAlignedPointerInInternalField(index, nullptr); + return; + } + + DCHECK_EQ(index, BaseObject::kEmbedderType); + Environment* env_ptr = static_cast(env); const InternalFieldInfo* info = reinterpret_cast(payload.data); - + // TODO(joyeecheung): we can add a constant kNodeEmbedderId to the + // beginning of every InternalFieldInfo to ensure that we don't + // step on payloads that were not serialized by Node.js. switch (info->type) { #define V(PropertyName, NativeTypeName) \ case EmbedderObjectType::k_##PropertyName: { \ @@ -1113,21 +1123,44 @@ void DeserializeNodeInternalFields(Local holder, StartupData SerializeNodeContextInternalFields(Local holder, int index, void* env) { - void* ptr = holder->GetAlignedPointerFromInternalField(BaseObject::kSlot); - if (ptr == nullptr) { + // We only do one serialization for the kEmbedderType slot, the result + // contains everything necessary for deserializing the entire object, + // including the fields whose index is bigger than kEmbedderType + // (most importantly, BaseObject::kSlot). + // For Node.js this design is enough for all the native binding that are + // serializable. + if (index != BaseObject::kEmbedderType) { + return StartupData{nullptr, 0}; + } + + void* type_ptr = holder->GetAlignedPointerFromInternalField(index); + if (type_ptr == nullptr) { + return StartupData{nullptr, 0}; + } + + uint16_t type = *(static_cast(type_ptr)); + per_process::Debug(DebugCategory::MKSNAPSHOT, "type = 0x%x\n", type); + if (type != kNodeEmbedderId) { return StartupData{nullptr, 0}; } + per_process::Debug(DebugCategory::MKSNAPSHOT, "Serialize internal field, index=%d, holder=%p\n", static_cast(index), *holder); - DCHECK(static_cast(ptr)->is_snapshotable()); - SnapshotableObject* obj = static_cast(ptr); + + void* binding_ptr = + holder->GetAlignedPointerFromInternalField(BaseObject::kSlot); + per_process::Debug(DebugCategory::MKSNAPSHOT, "binding = %p\n", binding_ptr); + DCHECK(static_cast(binding_ptr)->is_snapshotable()); + SnapshotableObject* obj = static_cast(binding_ptr); + per_process::Debug(DebugCategory::MKSNAPSHOT, "Object %p is %s, ", *holder, obj->GetTypeNameChars()); InternalFieldInfo* info = obj->Serialize(index); + per_process::Debug(DebugCategory::MKSNAPSHOT, "payload size=%d\n", static_cast(info->length)); @@ -1142,8 +1175,9 @@ void SerializeBindingData(Environment* env, env->ForEachBindingData([&](FastStringKey key, BaseObjectPtr binding) { per_process::Debug(DebugCategory::MKSNAPSHOT, - "Serialize binding %i, %p, type=%s\n", + "Serialize binding %i (%p), object=%p, type=%s\n", static_cast(i), + binding.get(), *(binding->object()), key.c_str()); diff --git a/src/node_snapshotable.h b/src/node_snapshotable.h index f0a8bce215e027..ec560b6d4fc633 100644 --- a/src/node_snapshotable.h +++ b/src/node_snapshotable.h @@ -30,11 +30,6 @@ enum class EmbedderObjectType : uint8_t { // When serializing an embedder object, we'll serialize the native states // into a chunk that can be mapped into a subclass of InternalFieldInfo, // and pass it into the V8 callback as the payload of StartupData. -// TODO(joyeecheung): the classification of types seem to be wrong. -// We'd need a type for each field of each class of native object. -// Maybe it's fine - we'll just use the type to invoke BaseObject constructors -// and specify that the BaseObject has only one field for us to serialize. -// And for non-BaseObject embedder objects, we'll use field-wise types. // The memory chunk looks like this: // // [ type ] - EmbedderObjectType (a uint8_t) diff --git a/src/node_v8.cc b/src/node_v8.cc index 5a1346a904e75e..e8bf3fc13a6b77 100644 --- a/src/node_v8.cc +++ b/src/node_v8.cc @@ -124,7 +124,7 @@ void BindingData::Deserialize(Local context, Local holder, int index, InternalFieldInfo* info) { - DCHECK_EQ(index, BaseObject::kSlot); + DCHECK_EQ(index, BaseObject::kEmbedderType); HandleScope scope(context->GetIsolate()); Environment* env = Environment::GetCurrent(context); BindingData* binding = env->AddBindingData(context, holder); @@ -132,7 +132,7 @@ void BindingData::Deserialize(Local context, } InternalFieldInfo* BindingData::Serialize(int index) { - DCHECK_EQ(index, BaseObject::kSlot); + DCHECK_EQ(index, BaseObject::kEmbedderType); InternalFieldInfo* info = InternalFieldInfo::New(type()); return info; }