Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit c51d85e

Browse filesBrowse files
codebyterejoyeecheung
authored and
danielleadams
committed
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 <joyeec9h3@gmail.com> PR-URL: #43521 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>
1 parent 70bb6a3 commit c51d85e
Copy full SHA for c51d85e

File tree

Expand file treeCollapse file tree

8 files changed

+66
-23
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

8 files changed

+66
-23
lines changed
Open diff view settings
Collapse file

‎src/base_object.h‎

Copy file name to clipboardExpand all lines: src/base_object.h
+4-2Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,13 @@ namespace worker {
3838
class TransferData;
3939
}
4040

41+
extern uint16_t kNodeEmbedderId;
42+
4143
class BaseObject : public MemoryRetainer {
4244
public:
43-
enum InternalFields { kSlot, kInternalFieldCount };
45+
enum InternalFields { kEmbedderType, kSlot, kInternalFieldCount };
4446

45-
// Associates this object with `object`. It uses the 0th internal field for
47+
// Associates this object with `object`. It uses the 1st internal field for
4648
// that, and in particular aborts if there is no such field.
4749
BaseObject(Environment* env, v8::Local<v8::Object> object);
4850
~BaseObject() override;
Collapse file

‎src/env.cc‎

Copy file name to clipboardExpand all lines: src/env.cc
+14-2Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1884,6 +1884,7 @@ void Environment::EnqueueDeserializeRequest(DeserializeRequestCallback cb,
18841884
Local<Object> holder,
18851885
int index,
18861886
InternalFieldInfo* info) {
1887+
DCHECK_EQ(index, BaseObject::kEmbedderType);
18871888
DeserializeRequest request{cb, {isolate(), holder}, index, info};
18881889
deserialize_requests_.push_back(std::move(request));
18891890
}
@@ -2166,7 +2167,9 @@ void Environment::RunWeakRefCleanup() {
21662167
BaseObject::BaseObject(Environment* env, Local<Object> object)
21672168
: persistent_handle_(env->isolate(), object), env_(env) {
21682169
CHECK_EQ(false, object.IsEmpty());
2169-
CHECK_GT(object->InternalFieldCount(), 0);
2170+
CHECK_GE(object->InternalFieldCount(), BaseObject::kInternalFieldCount);
2171+
object->SetAlignedPointerInInternalField(BaseObject::kEmbedderType,
2172+
&kNodeEmbedderId);
21702173
object->SetAlignedPointerInInternalField(BaseObject::kSlot,
21712174
static_cast<void*>(this));
21722175
env->AddCleanupHook(DeleteMe, static_cast<void*>(this));
@@ -2217,10 +2220,19 @@ void BaseObject::MakeWeak() {
22172220
WeakCallbackType::kParameter);
22182221
}
22192222

2223+
// This just has to be different from the Chromium ones:
2224+
// https://source.chromium.org/chromium/chromium/src/+/main:gin/public/gin_embedders.h;l=18-23;drc=5a758a97032f0b656c3c36a3497560762495501a
2225+
// Otherwise, when Node is loaded in an isolate which uses cppgc, cppgc will
2226+
// misinterpret the data stored in the embedder fields and try to garbage
2227+
// collect them.
2228+
uint16_t kNodeEmbedderId = 0x90de;
2229+
22202230
void BaseObject::LazilyInitializedJSTemplateConstructor(
22212231
const FunctionCallbackInfo<Value>& args) {
22222232
DCHECK(args.IsConstructCall());
2223-
DCHECK_GT(args.This()->InternalFieldCount(), 0);
2233+
CHECK_GE(args.This()->InternalFieldCount(), BaseObject::kInternalFieldCount);
2234+
args.This()->SetAlignedPointerInInternalField(BaseObject::kEmbedderType,
2235+
&kNodeEmbedderId);
22242236
args.This()->SetAlignedPointerInInternalField(BaseObject::kSlot, nullptr);
22252237
}
22262238

Collapse file

‎src/node_blob.cc‎

Copy file name to clipboardExpand all lines: src/node_blob.cc
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -464,7 +464,7 @@ void BlobBindingData::Deserialize(
464464
Local<Object> holder,
465465
int index,
466466
InternalFieldInfo* info) {
467-
DCHECK_EQ(index, BaseObject::kSlot);
467+
DCHECK_EQ(index, BaseObject::kEmbedderType);
468468
HandleScope scope(context->GetIsolate());
469469
Environment* env = Environment::GetCurrent(context);
470470
BlobBindingData* binding =
@@ -479,7 +479,7 @@ void BlobBindingData::PrepareForSerialization(
479479
}
480480

481481
InternalFieldInfo* BlobBindingData::Serialize(int index) {
482-
DCHECK_EQ(index, BaseObject::kSlot);
482+
DCHECK_EQ(index, BaseObject::kEmbedderType);
483483
InternalFieldInfo* info = InternalFieldInfo::New(type());
484484
return info;
485485
}
Collapse file

‎src/node_file.cc‎

Copy file name to clipboardExpand all lines: src/node_file.cc
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2401,7 +2401,7 @@ void BindingData::Deserialize(Local<Context> context,
24012401
Local<Object> holder,
24022402
int index,
24032403
InternalFieldInfo* info) {
2404-
DCHECK_EQ(index, BaseObject::kSlot);
2404+
DCHECK_EQ(index, BaseObject::kEmbedderType);
24052405
HandleScope scope(context->GetIsolate());
24062406
Environment* env = Environment::GetCurrent(context);
24072407
BindingData* binding = env->AddBindingData<BindingData>(context, holder);
@@ -2418,7 +2418,7 @@ void BindingData::PrepareForSerialization(Local<Context> context,
24182418
}
24192419

24202420
InternalFieldInfo* BindingData::Serialize(int index) {
2421-
DCHECK_EQ(index, BaseObject::kSlot);
2421+
DCHECK_EQ(index, BaseObject::kEmbedderType);
24222422
InternalFieldInfo* info = InternalFieldInfo::New(type());
24232423
return info;
24242424
}
Collapse file

‎src/node_process_methods.cc‎

Copy file name to clipboardExpand all lines: src/node_process_methods.cc
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -531,7 +531,7 @@ void BindingData::PrepareForSerialization(Local<Context> context,
531531
}
532532

533533
InternalFieldInfo* BindingData::Serialize(int index) {
534-
DCHECK_EQ(index, BaseObject::kSlot);
534+
DCHECK_EQ(index, BaseObject::kEmbedderType);
535535
InternalFieldInfo* info = InternalFieldInfo::New(type());
536536
return info;
537537
}
@@ -540,7 +540,7 @@ void BindingData::Deserialize(Local<Context> context,
540540
Local<Object> holder,
541541
int index,
542542
InternalFieldInfo* info) {
543-
DCHECK_EQ(index, BaseObject::kSlot);
543+
DCHECK_EQ(index, BaseObject::kEmbedderType);
544544
v8::HandleScope scope(context->GetIsolate());
545545
Environment* env = Environment::GetCurrent(context);
546546
// Recreate the buffer in the constructor.
Collapse file

‎src/node_snapshotable.cc‎

Copy file name to clipboardExpand all lines: src/node_snapshotable.cc
+40-6Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1089,10 +1089,20 @@ void DeserializeNodeInternalFields(Local<Object> holder,
10891089
static_cast<int>(index),
10901090
(*holder),
10911091
static_cast<int>(payload.raw_size));
1092+
1093+
if (payload.raw_size == 0) {
1094+
holder->SetAlignedPointerInInternalField(index, nullptr);
1095+
return;
1096+
}
1097+
1098+
DCHECK_EQ(index, BaseObject::kEmbedderType);
1099+
10921100
Environment* env_ptr = static_cast<Environment*>(env);
10931101
const InternalFieldInfo* info =
10941102
reinterpret_cast<const InternalFieldInfo*>(payload.data);
1095-
1103+
// TODO(joyeecheung): we can add a constant kNodeEmbedderId to the
1104+
// beginning of every InternalFieldInfo to ensure that we don't
1105+
// step on payloads that were not serialized by Node.js.
10961106
switch (info->type) {
10971107
#define V(PropertyName, NativeTypeName) \
10981108
case EmbedderObjectType::k_##PropertyName: { \
@@ -1113,21 +1123,44 @@ void DeserializeNodeInternalFields(Local<Object> holder,
11131123
StartupData SerializeNodeContextInternalFields(Local<Object> holder,
11141124
int index,
11151125
void* env) {
1116-
void* ptr = holder->GetAlignedPointerFromInternalField(BaseObject::kSlot);
1117-
if (ptr == nullptr) {
1126+
// We only do one serialization for the kEmbedderType slot, the result
1127+
// contains everything necessary for deserializing the entire object,
1128+
// including the fields whose index is bigger than kEmbedderType
1129+
// (most importantly, BaseObject::kSlot).
1130+
// For Node.js this design is enough for all the native binding that are
1131+
// serializable.
1132+
if (index != BaseObject::kEmbedderType) {
1133+
return StartupData{nullptr, 0};
1134+
}
1135+
1136+
void* type_ptr = holder->GetAlignedPointerFromInternalField(index);
1137+
if (type_ptr == nullptr) {
1138+
return StartupData{nullptr, 0};
1139+
}
1140+
1141+
uint16_t type = *(static_cast<uint16_t*>(type_ptr));
1142+
per_process::Debug(DebugCategory::MKSNAPSHOT, "type = 0x%x\n", type);
1143+
if (type != kNodeEmbedderId) {
11181144
return StartupData{nullptr, 0};
11191145
}
1146+
11201147
per_process::Debug(DebugCategory::MKSNAPSHOT,
11211148
"Serialize internal field, index=%d, holder=%p\n",
11221149
static_cast<int>(index),
11231150
*holder);
1124-
DCHECK(static_cast<BaseObject*>(ptr)->is_snapshotable());
1125-
SnapshotableObject* obj = static_cast<SnapshotableObject*>(ptr);
1151+
1152+
void* binding_ptr =
1153+
holder->GetAlignedPointerFromInternalField(BaseObject::kSlot);
1154+
per_process::Debug(DebugCategory::MKSNAPSHOT, "binding = %p\n", binding_ptr);
1155+
DCHECK(static_cast<BaseObject*>(binding_ptr)->is_snapshotable());
1156+
SnapshotableObject* obj = static_cast<SnapshotableObject*>(binding_ptr);
1157+
11261158
per_process::Debug(DebugCategory::MKSNAPSHOT,
11271159
"Object %p is %s, ",
11281160
*holder,
11291161
obj->GetTypeNameChars());
11301162
InternalFieldInfo* info = obj->Serialize(index);
1163+
11311164
per_process::Debug(DebugCategory::MKSNAPSHOT,
11321165
"payload size=%d\n",
11331166
static_cast<int>(info->length));
@@ -1142,8 +1175,9 @@ void SerializeBindingData(Environment* env,
11421175
env->ForEachBindingData([&](FastStringKey key,
11431176
BaseObjectPtr<BaseObject> binding) {
11441177
per_process::Debug(DebugCategory::MKSNAPSHOT,
1145-
"Serialize binding %i, %p, type=%s\n",
1178+
"Serialize binding %i (%p), object=%p, type=%s\n",
11461179
static_cast<int>(i),
1180+
binding.get(),
11471181
*(binding->object()),
11481182
key.c_str());
11491183

Collapse file

‎src/node_snapshotable.h‎

Copy file name to clipboardExpand all lines: src/node_snapshotable.h
-5Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,6 @@ enum class EmbedderObjectType : uint8_t {
3030
// When serializing an embedder object, we'll serialize the native states
3131
// into a chunk that can be mapped into a subclass of InternalFieldInfo,
3232
// and pass it into the V8 callback as the payload of StartupData.
33-
// TODO(joyeecheung): the classification of types seem to be wrong.
34-
// We'd need a type for each field of each class of native object.
35-
// Maybe it's fine - we'll just use the type to invoke BaseObject constructors
36-
// and specify that the BaseObject has only one field for us to serialize.
37-
// And for non-BaseObject embedder objects, we'll use field-wise types.
3833
// The memory chunk looks like this:
3934
//
4035
// [ type ] - EmbedderObjectType (a uint8_t)
Collapse file

‎src/node_v8.cc‎

Copy file name to clipboardExpand all lines: src/node_v8.cc
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,15 +124,15 @@ void BindingData::Deserialize(Local<Context> context,
124124
Local<Object> holder,
125125
int index,
126126
InternalFieldInfo* info) {
127-
DCHECK_EQ(index, BaseObject::kSlot);
127+
DCHECK_EQ(index, BaseObject::kEmbedderType);
128128
HandleScope scope(context->GetIsolate());
129129
Environment* env = Environment::GetCurrent(context);
130130
BindingData* binding = env->AddBindingData<BindingData>(context, holder);
131131
CHECK_NOT_NULL(binding);
132132
}
133133

134134
InternalFieldInfo* BindingData::Serialize(int index) {
135-
DCHECK_EQ(index, BaseObject::kSlot);
135+
DCHECK_EQ(index, BaseObject::kEmbedderType);
136136
InternalFieldInfo* info = InternalFieldInfo::New(type());
137137
return info;
138138
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.