From 923bd8623bdb92ea5200265ede01ec5c734063ca Mon Sep 17 00:00:00 2001
From: Shelley Vohr <shelley.vohr@gmail.com>
Date: Tue, 21 Jun 2022 14:08:41 +0200
Subject: [PATCH 1/3] src: fix cppgc incompatibility in v8
---
src/base_object.h | 11 +++++++++--
src/env.cc | 8 ++++++--
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/base_object.h b/src/base_object.h
index 842f763a56d75c..c2f4b6d028d6bb 100644
--- a/src/base_object.h
+++ b/src/base_object.h
@@ -38,11 +38,18 @@ namespace worker {
class TransferData;
}
+// 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;
+
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<v8::Object> object);
~BaseObject() override;
diff --git a/src/env.cc b/src/env.cc
index 83f65f2ad20dba..823e21c5567b3b 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -2026,7 +2026,9 @@ void Environment::RunWeakRefCleanup() {
BaseObject::BaseObject(Environment* env, Local<Object> 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<void*>(this));
env->AddCleanupHook(DeleteMe, static_cast<void*>(this));
@@ -2080,7 +2082,9 @@ void BaseObject::MakeWeak() {
void BaseObject::LazilyInitializedJSTemplateConstructor(
const FunctionCallbackInfo<Value>& 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);
}
From 2cb817e743cd4f887c6e88c443c0773562967dfc Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Tue, 12 Jul 2022 22:37:59 +0800
Subject: [PATCH 2/3] work with the new BaseObject layout in snapshot callbacks
---
src/base_object.h | 7 +-----
src/env.cc | 9 ++++++++
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, 58 insertions(+), 25 deletions(-)
diff --git a/src/base_object.h b/src/base_object.h
index c2f4b6d028d6bb..6f072bf5ec0357 100644
--- a/src/base_object.h
+++ b/src/base_object.h
@@ -38,12 +38,7 @@ namespace worker {
class TransferData;
}
-// 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;
+extern uint16_t kNodeEmbedderId;
class BaseObject : public MemoryRetainer {
public:
diff --git a/src/env.cc b/src/env.cc
index 823e21c5567b3b..67891e80e7579c 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -1744,6 +1744,7 @@ void Environment::EnqueueDeserializeRequest(DeserializeRequestCallback cb,
Local<Object> holder,
int index,
InternalFieldInfo* info) {
+ DCHECK_EQ(index, BaseObject::kEmbedderType);
DeserializeRequest request{cb, {isolate(), holder}, index, info};
deserialize_requests_.push_back(std::move(request));
}
@@ -2079,6 +2080,14 @@ 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<Value>& args) {
DCHECK(args.IsConstructCall());
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<Object> 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> context,
Local<Object> 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<BindingData>(context, holder);
@@ -2418,7 +2418,7 @@ void BindingData::PrepareForSerialization(Local<Context> 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> 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> context,
Local<Object> 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<Object> holder,
static_cast<int>(index),
(*holder),
static_cast<int>(payload.raw_size));
+
+ if (payload.raw_size == 0) {
+ holder->SetAlignedPointerInInternalField(index, nullptr);
+ return;
+ }
+
+ DCHECK_EQ(index, BaseObject::kEmbedderType);
+
Environment* env_ptr = static_cast<Environment*>(env);
const InternalFieldInfo* info =
reinterpret_cast<const InternalFieldInfo*>(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<Object> holder,
StartupData SerializeNodeContextInternalFields(Local<Object> 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<uint16_t*>(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<int>(index),
*holder);
- DCHECK(static_cast<BaseObject*>(ptr)->is_snapshotable());
- SnapshotableObject* obj = static_cast<SnapshotableObject*>(ptr);
+
+ void* binding_ptr =
+ holder->GetAlignedPointerFromInternalField(BaseObject::kSlot);
+ per_process::Debug(DebugCategory::MKSNAPSHOT, "binding = %p\n", binding_ptr);
+ DCHECK(static_cast<BaseObject*>(binding_ptr)->is_snapshotable());
+ SnapshotableObject* obj = static_cast<SnapshotableObject*>(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<int>(info->length));
@@ -1142,8 +1175,9 @@ void SerializeBindingData(Environment* env,
env->ForEachBindingData([&](FastStringKey key,
BaseObjectPtr<BaseObject> binding) {
per_process::Debug(DebugCategory::MKSNAPSHOT,
- "Serialize binding %i, %p, type=%s\n",
+ "Serialize binding %i (%p), object=%p, type=%s\n",
static_cast<int>(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> context,
Local<Object> 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<BindingData>(context, holder);
@@ -132,7 +132,7 @@ void BindingData::Deserialize(Local<Context> context,
}
InternalFieldInfo* BindingData::Serialize(int index) {
- DCHECK_EQ(index, BaseObject::kSlot);
+ DCHECK_EQ(index, BaseObject::kEmbedderType);
InternalFieldInfo* info = InternalFieldInfo::New(type());
return info;
}
From c18aa1d692a2a85caa34113bfb4fb648e54d68ee Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Tue, 9 Aug 2022 02:56:19 +0800
Subject: [PATCH 3/3] fixup! work with the new BaseObject layout in snapshot
callbacks
---
src/env.cc | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/env.cc b/src/env.cc
index 67891e80e7579c..d2a3390d4356cb 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -2080,7 +2080,6 @@ 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