Visitar URL original
src: use effective cppgc wrapper id to deduce non-cppgc id · joyeecheung/node@4b1fb30 · GitHub
Skip to content

Commit 4b1fb30

Browse files
joyeecheungjoyeecheung
committed
src: use effective cppgc wrapper id to deduce non-cppgc id
Previously we hard-code a wrapper id to be used in BaseObjects to avoid accidentally triggering cppgc on these non-cppgc-managed objects, but hard-coding can be be hacky and result in mismatch when we start to create CppHeap ourselves. This patch makes it more robust by deducing non-cppgc id from the effective cppgc id, if there is one. PR-URL: nodejs#48660 Refs: v8/v8@9327503 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
1 parent aa1a676 commit 4b1fb30

11 files changed

Lines changed: 233 additions & 29 deletions

‎node.gyp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1036,6 +1036,7 @@
10361036
'test/cctest/test_aliased_buffer.cc',
10371037
'test/cctest/test_base64.cc',
10381038
'test/cctest/test_base_object_ptr.cc',
1039+
'test/cctest/test_cppgc.cc',
10391040
'test/cctest/test_node_postmortem_metadata.cc',
10401041
'test/cctest/test_environment.cc',
10411042
'test/cctest/test_linked_binding.cc',

‎src/base_object-inl.h‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -70,23 +70,28 @@ Realm* BaseObject::realm() const {
7070
return realm_;
7171
}
7272

73-
bool BaseObject::IsBaseObject(v8::Local<v8::Object> obj) {
73+
bool BaseObject::IsBaseObject(IsolateData* isolate_data,
74+
v8::Local<v8::Object> obj) {
7475
if (obj->InternalFieldCount() < BaseObject::kInternalFieldCount) {
7576
return false;
7677
}
77-
void* ptr =
78-
obj->GetAlignedPointerFromInternalField(BaseObject::kEmbedderType);
79-
return ptr == &kNodeEmbedderId;
78+
79+
uint16_t* ptr = static_cast<uint16_t*>(
80+
obj->GetAlignedPointerFromInternalField(BaseObject::kEmbedderType));
81+
return ptr == isolate_data->embedder_id_for_non_cppgc();
8082
}
8183

82-
void BaseObject::TagBaseObject(v8::Local<v8::Object> object) {
84+
void BaseObject::TagBaseObject(IsolateData* isolate_data,
85+
v8::Local<v8::Object> object) {
8386
DCHECK_GE(object->InternalFieldCount(), BaseObject::kInternalFieldCount);
84-
object->SetAlignedPointerInInternalField(BaseObject::kEmbedderType,
85-
&kNodeEmbedderId);
87+
object->SetAlignedPointerInInternalField(
88+
BaseObject::kEmbedderType, isolate_data->embedder_id_for_non_cppgc());
8689
}
8790

88-
void BaseObject::SetInternalFields(v8::Local<v8::Object> object, void* slot) {
89-
TagBaseObject(object);
91+
void BaseObject::SetInternalFields(IsolateData* isolate_data,
92+
v8::Local<v8::Object> object,
93+
void* slot) {
94+
TagBaseObject(isolate_data, object);
9095
object->SetAlignedPointerInInternalField(BaseObject::kSlot, slot);
9196
}
9297

‎src/base_object.cc‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ BaseObject::BaseObject(Realm* realm, Local<Object> object)
1717
: persistent_handle_(realm->isolate(), object), realm_(realm) {
1818
CHECK_EQ(false, object.IsEmpty());
1919
CHECK_GE(object->InternalFieldCount(), BaseObject::kInternalFieldCount);
20-
SetInternalFields(object, static_cast<void*>(this));
20+
SetInternalFields(realm->isolate_data(), object, static_cast<void*>(this));
2121
realm->AddCleanupHook(DeleteMe, static_cast<void*>(this));
2222
realm->modify_base_object_count(1);
2323
}
@@ -66,18 +66,13 @@ void BaseObject::MakeWeak() {
6666
WeakCallbackType::kParameter);
6767
}
6868

69-
// This just has to be different from the Chromium ones:
70-
// https://source.chromium.org/chromium/chromium/src/+/main:gin/public/gin_embedders.h;l=18-23;drc=5a758a97032f0b656c3c36a3497560762495501a
71-
// Otherwise, when Node is loaded in an isolate which uses cppgc, cppgc will
72-
// misinterpret the data stored in the embedder fields and try to garbage
73-
// collect them.
74-
uint16_t kNodeEmbedderId = 0x90de;
75-
7669
void BaseObject::LazilyInitializedJSTemplateConstructor(
7770
const FunctionCallbackInfo<Value>& args) {
7871
DCHECK(args.IsConstructCall());
7972
CHECK_GE(args.This()->InternalFieldCount(), BaseObject::kInternalFieldCount);
80-
SetInternalFields(args.This(), nullptr);
73+
Environment* env = Environment::GetCurrent(args);
74+
DCHECK_NOT_NULL(env);
75+
SetInternalFields(env->isolate_data(), args.This(), nullptr);
8176
}
8277

8378
Local<FunctionTemplate> BaseObject::MakeLazilyInitializedJSTemplate(

‎src/base_object.h‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,6 @@ namespace worker {
4141
class TransferData;
4242
}
4343

44-
extern uint16_t kNodeEmbedderId;
45-
4644
class BaseObject : public MemoryRetainer {
4745
public:
4846
enum InternalFields { kEmbedderType, kSlot, kInternalFieldCount };
@@ -74,10 +72,13 @@ class BaseObject : public MemoryRetainer {
7472
// was also passed to the `BaseObject()` constructor initially.
7573
// This may return `nullptr` if the C++ object has not been constructed yet,
7674
// e.g. when the JS object used `MakeLazilyInitializedJSTemplate`.
77-
static inline void SetInternalFields(v8::Local<v8::Object> object,
75+
static inline void SetInternalFields(IsolateData* isolate_data,
76+
v8::Local<v8::Object> object,
7877
void* slot);
79-
static inline bool IsBaseObject(v8::Local<v8::Object> object);
80-
static inline void TagBaseObject(v8::Local<v8::Object> object);
78+
static inline bool IsBaseObject(IsolateData* isolate_data,
79+
v8::Local<v8::Object> object);
80+
static inline void TagBaseObject(IsolateData* isolate_data,
81+
v8::Local<v8::Object> object);
8182
static void LazilyInitializedJSTemplateConstructor(
8283
const v8::FunctionCallbackInfo<v8::Value>& args);
8384
static inline BaseObject* FromJSObject(v8::Local<v8::Value> object);

‎src/env-inl.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,14 @@ inline uv_loop_t* IsolateData::event_loop() const {
6161
return event_loop_;
6262
}
6363

64+
inline uint16_t* IsolateData::embedder_id_for_cppgc() const {
65+
return &(wrapper_data_->cppgc_id);
66+
}
67+
68+
inline uint16_t* IsolateData::embedder_id_for_non_cppgc() const {
69+
return &(wrapper_data_->non_cppgc_id);
70+
}
71+
6472
inline NodeArrayBufferAllocator* IsolateData::node_allocator() const {
6573
return node_allocator_;
6674
}

‎src/env.cc‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include "tracing/agent.h"
2020
#include "tracing/traced_value.h"
2121
#include "util-inl.h"
22+
#include "v8-cppgc.h"
2223
#include "v8-profiler.h"
2324

2425
#include <algorithm>
@@ -28,6 +29,7 @@
2829
#include <iostream>
2930
#include <limits>
3031
#include <memory>
32+
#include <unordered_map>
3133

3234
namespace node {
3335

@@ -497,6 +499,11 @@ void IsolateData::CreateProperties() {
497499
contextify::ContextifyContext::InitializeGlobalTemplates(this);
498500
}
499501

502+
constexpr uint16_t kDefaultCppGCEmebdderID = 0x90de;
503+
Mutex IsolateData::isolate_data_mutex_;
504+
std::unordered_map<uint16_t, std::unique_ptr<PerIsolateWrapperData>>
505+
IsolateData::wrapper_data_map_;
506+
500507
IsolateData::IsolateData(Isolate* isolate,
501508
uv_loop_t* event_loop,
502509
MultiIsolatePlatform* platform,
@@ -510,6 +517,46 @@ IsolateData::IsolateData(Isolate* isolate,
510517
snapshot_data_(snapshot_data) {
511518
options_.reset(
512519
new PerIsolateOptions(*(per_process::cli_options->per_isolate)));
520+
v8::CppHeap* cpp_heap = isolate->GetCppHeap();
521+
522+
uint16_t cppgc_id = kDefaultCppGCEmebdderID;
523+
if (cpp_heap != nullptr) {
524+
// The general convention of the wrappable layout for cppgc in the
525+
// ecosystem is:
526+
// [ 0 ] -> embedder id
527+
// [ 1 ] -> wrappable instance
528+
// If the Isolate includes a CppHeap attached by another embedder,
529+
// And if they also use the field 0 for the ID, we DCHECK that
530+
// the layout matches our layout, and record the embedder ID for cppgc
531+
// to avoid accidentally enabling cppgc on non-cppgc-managed wrappers .
532+
v8::WrapperDescriptor descriptor = cpp_heap->wrapper_descriptor();
533+
if (descriptor.wrappable_type_index == BaseObject::kEmbedderType) {
534+
cppgc_id = descriptor.embedder_id_for_garbage_collected;
535+
DCHECK_EQ(descriptor.wrappable_instance_index, BaseObject::kSlot);
536+
}
537+
// If the CppHeap uses the slot we use to put non-cppgc-traced BaseObject
538+
// for embedder ID, V8 could accidentally enable cppgc on them. So
539+
// safe guard against this.
540+
DCHECK_NE(descriptor.wrappable_type_index, BaseObject::kSlot);
541+
}
542+
// We do not care about overflow since we just want this to be different
543+
// from the cppgc id.
544+
uint16_t non_cppgc_id = cppgc_id + 1;
545+
546+
{
547+
// GC could still be run after the IsolateData is destroyed, so we store
548+
// the ids in a static map to ensure pointers to them are still valid
549+
// then. In practice there should be very few variants of the cppgc id
550+
// in one process so the size of this map should be very small.
551+
node::Mutex::ScopedLock lock(isolate_data_mutex_);
552+
auto it = wrapper_data_map_.find(cppgc_id);
553+
if (it == wrapper_data_map_.end()) {
554+
auto pair = wrapper_data_map_.emplace(
555+
cppgc_id, new PerIsolateWrapperData{cppgc_id, non_cppgc_id});
556+
it = pair.first;
557+
}
558+
wrapper_data_ = it->second.get();
559+
}
513560

514561
if (snapshot_data == nullptr) {
515562
CreateProperties();

‎src/env.h‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,13 +124,19 @@ struct IsolateDataSerializeInfo {
124124
const IsolateDataSerializeInfo& i);
125125
};
126126

127+
struct PerIsolateWrapperData {
128+
uint16_t cppgc_id;
129+
uint16_t non_cppgc_id;
130+
};
131+
127132
class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
128133
public:
129134
IsolateData(v8::Isolate* isolate,
130135
uv_loop_t* event_loop,
131136
MultiIsolatePlatform* platform = nullptr,
132137
ArrayBufferAllocator* node_allocator = nullptr,
133138
const SnapshotData* snapshot_data = nullptr);
139+
134140
SET_MEMORY_INFO_NAME(IsolateData)
135141
SET_SELF_SIZE(IsolateData)
136142
void MemoryInfo(MemoryTracker* tracker) const override;
@@ -139,6 +145,9 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
139145
bool is_building_snapshot() const { return is_building_snapshot_; }
140146
void set_is_building_snapshot(bool value) { is_building_snapshot_ = value; }
141147

148+
uint16_t* embedder_id_for_cppgc() const;
149+
uint16_t* embedder_id_for_non_cppgc() const;
150+
142151
inline uv_loop_t* event_loop() const;
143152
inline MultiIsolatePlatform* platform() const;
144153
inline const SnapshotData* snapshot_data() const;
@@ -223,6 +232,11 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
223232
std::shared_ptr<PerIsolateOptions> options_;
224233
worker::Worker* worker_context_ = nullptr;
225234
bool is_building_snapshot_ = false;
235+
PerIsolateWrapperData* wrapper_data_;
236+
237+
static Mutex isolate_data_mutex_;
238+
static std::unordered_map<uint16_t, std::unique_ptr<PerIsolateWrapperData>>
239+
wrapper_data_map_;
226240
};
227241

228242
struct ContextInfo {

‎src/node_messaging.cc‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,7 @@ class SerializerDelegate : public ValueSerializer::Delegate {
318318
}
319319

320320
Maybe<bool> WriteHostObject(Isolate* isolate, Local<Object> object) override {
321-
if (BaseObject::IsBaseObject(object)) {
321+
if (BaseObject::IsBaseObject(env_->isolate_data(), object)) {
322322
return WriteHostObject(
323323
BaseObjectPtr<BaseObject> { Unwrap<BaseObject>(object) });
324324
}
@@ -514,7 +514,8 @@ Maybe<bool> Message::Serialize(Environment* env,
514514
serializer.TransferArrayBuffer(id, ab);
515515
continue;
516516
} else if (entry->IsObject() &&
517-
BaseObject::IsBaseObject(entry.As<Object>())) {
517+
BaseObject::IsBaseObject(env->isolate_data(),
518+
entry.As<Object>())) {
518519
// Check if the source MessagePort is being transferred.
519520
if (!source_port.IsEmpty() && entry == source_port) {
520521
ThrowDataCloneException(
@@ -1281,7 +1282,8 @@ JSTransferable::NestedTransferables() const {
12811282
Local<Value> value;
12821283
if (!list->Get(context, i).ToLocal(&value))
12831284
return Nothing<BaseObjectList>();
1284-
if (value->IsObject() && BaseObject::IsBaseObject(value.As<Object>()))
1285+
if (value->IsObject() &&
1286+
BaseObject::IsBaseObject(env()->isolate_data(), value.As<Object>()))
12851287
ret.emplace_back(Unwrap<BaseObject>(value));
12861288
}
12871289
return Just(ret);
@@ -1336,7 +1338,8 @@ BaseObjectPtr<BaseObject> JSTransferable::Data::Deserialize(
13361338
if (!env->messaging_deserialize_create_object()
13371339
->Call(context, Null(env->isolate()), 1, &info)
13381340
.ToLocal(&ret) ||
1339-
!ret->IsObject() || !BaseObject::IsBaseObject(ret.As<Object>())) {
1341+
!ret->IsObject() ||
1342+
!BaseObject::IsBaseObject(env->isolate_data(), ret.As<Object>())) {
13401343
return {};
13411344
}
13421345

‎src/node_snapshotable.cc‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1202,14 +1202,16 @@ void DeserializeNodeInternalFields(Local<Object> holder,
12021202

12031203
StartupData SerializeNodeContextInternalFields(Local<Object> holder,
12041204
int index,
1205-
void* env) {
1205+
void* callback_data) {
12061206
// We only do one serialization for the kEmbedderType slot, the result
12071207
// contains everything necessary for deserializing the entire object,
12081208
// including the fields whose index is bigger than kEmbedderType
12091209
// (most importantly, BaseObject::kSlot).
12101210
// For Node.js this design is enough for all the native binding that are
12111211
// serializable.
1212-
if (index != BaseObject::kEmbedderType || !BaseObject::IsBaseObject(holder)) {
1212+
Environment* env = static_cast<Environment*>(callback_data);
1213+
if (index != BaseObject::kEmbedderType ||
1214+
!BaseObject::IsBaseObject(env->isolate_data(), holder)) {
12131215
return StartupData{nullptr, 0};
12141216
}
12151217

‎test/cctest/node_test_fixture.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ void NodeTestEnvironment::SetUp() {
3434
}
3535

3636
void NodeTestEnvironment::TearDown() {
37+
cppgc::ShutdownProcess();
3738
v8::V8::Dispose();
3839
v8::V8::DisposePlatform();
3940
NodeZeroIsolateTestFixture::platform->Shutdown();

0 commit comments

Comments
 (0)