Skip to content

Commit

Permalink
src: fix cppgc incompatibility in v8
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
2 people authored and danielleadams committed Jan 3, 2023
1 parent b8290ff commit f071028
Show file tree
Hide file tree
Showing 8 changed files with 66 additions and 23 deletions.
6 changes: 4 additions & 2 deletions src/base_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::Object> object);
~BaseObject() override;
Expand Down
16 changes: 14 additions & 2 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1738,6 +1738,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));
}
Expand Down Expand Up @@ -2017,7 +2018,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));
Expand Down Expand Up @@ -2068,10 +2071,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<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);
}

Expand Down
4 changes: 2 additions & 2 deletions src/node_blob.cc
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,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 =
Expand All @@ -480,7 +480,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;
}
Expand Down
4 changes: 2 additions & 2 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2567,7 +2567,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);
Expand All @@ -2584,7 +2584,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;
}
Expand Down
4 changes: 2 additions & 2 deletions src/node_process_methods.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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.
Expand Down
46 changes: 40 additions & 6 deletions src/node_snapshotable.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1119,10 +1119,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: { \
Expand All @@ -1143,21 +1153,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));
Expand All @@ -1172,8 +1205,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());

Expand Down
5 changes: 0 additions & 5 deletions src/node_snapshotable.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions src/node_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,15 @@ 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);
CHECK_NOT_NULL(binding);
}

InternalFieldInfo* BindingData::Serialize(int index) {
DCHECK_EQ(index, BaseObject::kSlot);
DCHECK_EQ(index, BaseObject::kEmbedderType);
InternalFieldInfo* info = InternalFieldInfo::New(type());
return info;
}
Expand Down

0 comments on commit f071028

Please sign in to comment.