src: use effective cppgc wrapper id to deduce non-cppgc id

common.gypi

@@ -36,7 +36,7 @@
 
     # Reset this number to 0 on major V8 upgrades.
     # Increment by one for each non-official patch applied to deps/v8.
-    'v8_embedder_string': '-node.13',
+    'v8_embedder_string': '-node.14',
 
     ##### V8 defaults for Node.js #####
 

deps/v8/include/v8-cppgc.h

@@ -177,6 +177,11 @@ class V8_EXPORT CppHeap {
   void CollectGarbageInYoungGenerationForTesting(
       cppgc::EmbedderStackState stack_state);
 
+  /**
+   * \returns the wrapper descriptor of this CppHeap.
+   */
+  v8::WrapperDescriptor wrapper_descriptor() const;
+
  private:
   CppHeap() = default;
 

deps/v8/src/heap/cppgc-js/cpp-heap.cc

@@ -147,6 +147,10 @@ void CppHeap::CollectGarbageInYoungGenerationForTesting(
       internal::CppHeap::CollectionType::kMinor, stack_state);
 }
 
+v8::WrapperDescriptor CppHeap::wrapper_descriptor() const {
+  return internal::CppHeap::From(this)->wrapper_descriptor();
+}
+
 namespace internal {
 
 namespace {

deps/v8/test/unittests/heap/cppgc-js/unified-heap-unittest.cc

@@ -706,4 +706,45 @@ TEST_F(UnifiedHeapTest, TracedReferenceHandlesDoNotLeak) {
   EXPECT_EQ(initial_count, final_count + 1);
 }
 
+namespace {
+class Wrappable2 final : public cppgc::GarbageCollected<Wrappable2> {
+ public:
+  static size_t destructor_call_count;
+  void Trace(cppgc::Visitor* visitor) const {}
+  ~Wrappable2() { destructor_call_count++; }
+};
+
+size_t Wrappable2::destructor_call_count = 0;
+}  // namespace
+
+TEST_F(UnifiedHeapTest, WrapperDescriptorGetter) {
+  v8::Isolate* isolate = v8_isolate();
+  v8::HandleScope scope(isolate);
+  auto* wrappable_object =
+      cppgc::MakeGarbageCollected<Wrappable2>(allocation_handle());
+  v8::WrapperDescriptor descriptor =
+      isolate->GetCppHeap()->wrapper_descriptor();
+  v8::Local<v8::ObjectTemplate> tmpl = v8::ObjectTemplate::New(isolate);
+  int size = std::max(descriptor.wrappable_type_index,
+                      descriptor.wrappable_instance_index) +
+             1;
+  tmpl->SetInternalFieldCount(size);
+  v8::Local<v8::Object> api_object =
+      tmpl->NewInstance(isolate->GetCurrentContext()).ToLocalChecked();
+  api_object->SetAlignedPointerInInternalField(
+      descriptor.wrappable_type_index,
+      &descriptor.embedder_id_for_garbage_collected);
+  api_object->SetAlignedPointerInInternalField(
+      descriptor.wrappable_instance_index, wrappable_object);
+
+  Wrappable2::destructor_call_count = 0;
+  EXPECT_EQ(0u, Wrappable2::destructor_call_count);
+  CollectGarbageWithoutEmbedderStack(cppgc::Heap::SweepingType::kAtomic);
+  EXPECT_EQ(0u, Wrappable2::destructor_call_count);
+  api_object->SetAlignedPointerInInternalField(
+      descriptor.wrappable_instance_index, nullptr);
+  CollectGarbageWithoutEmbedderStack(cppgc::Heap::SweepingType::kAtomic);
+  EXPECT_EQ(1u, Wrappable2::destructor_call_count);
+}
+
 }  // namespace v8::internal

node.gyp

@@ -1047,6 +1047,7 @@
         'test/cctest/test_aliased_buffer.cc',
         'test/cctest/test_base64.cc',
         'test/cctest/test_base_object_ptr.cc',
+        'test/cctest/test_cppgc.cc',
         'test/cctest/test_node_postmortem_metadata.cc',
         'test/cctest/test_environment.cc',
         'test/cctest/test_linked_binding.cc',

src/base_object-inl.h

@@ -70,23 +70,28 @@ Realm* BaseObject::realm() const {
   return realm_;
 }
 
-bool BaseObject::IsBaseObject(v8::Local<v8::Object> obj) {
+bool BaseObject::IsBaseObject(IsolateData* isolate_data,
+                              v8::Local<v8::Object> obj) {
   if (obj->InternalFieldCount() < BaseObject::kInternalFieldCount) {
     return false;
   }
-  void* ptr =
-      obj->GetAlignedPointerFromInternalField(BaseObject::kEmbedderType);
-  return ptr == &kNodeEmbedderId;
+
+  uint16_t* ptr = static_cast<uint16_t*>(
+      obj->GetAlignedPointerFromInternalField(BaseObject::kEmbedderType));
+  return ptr == isolate_data->embedder_id_for_non_cppgc();
 }
 
-void BaseObject::TagBaseObject(v8::Local<v8::Object> object) {
+void BaseObject::TagBaseObject(IsolateData* isolate_data,
+                               v8::Local<v8::Object> object) {
   DCHECK_GE(object->InternalFieldCount(), BaseObject::kInternalFieldCount);
-  object->SetAlignedPointerInInternalField(BaseObject::kEmbedderType,
-                                           &kNodeEmbedderId);
+  object->SetAlignedPointerInInternalField(
+      BaseObject::kEmbedderType, isolate_data->embedder_id_for_non_cppgc());
 }
 
-void BaseObject::SetInternalFields(v8::Local<v8::Object> object, void* slot) {
-  TagBaseObject(object);
+void BaseObject::SetInternalFields(IsolateData* isolate_data,
+                                   v8::Local<v8::Object> object,
+                                   void* slot) {
+  TagBaseObject(isolate_data, object);
   object->SetAlignedPointerInInternalField(BaseObject::kSlot, slot);
 }
 

src/base_object.cc

@@ -22,7 +22,7 @@ BaseObject::BaseObject(Realm* realm, Local<Object> object)
     : persistent_handle_(realm->isolate(), object), realm_(realm) {
   CHECK_EQ(false, object.IsEmpty());
   CHECK_GE(object->InternalFieldCount(), BaseObject::kInternalFieldCount);
-  SetInternalFields(object, static_cast<void*>(this));
+  SetInternalFields(realm->isolate_data(), object, static_cast<void*>(this));
   realm->AddCleanupHook(DeleteMe, static_cast<void*>(this));
   realm->modify_base_object_count(1);
 }
@@ -71,18 +71,13 @@ 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());
   CHECK_GE(args.This()->InternalFieldCount(), BaseObject::kInternalFieldCount);
-  SetInternalFields(args.This(), nullptr);
+  Environment* env = Environment::GetCurrent(args);
+  DCHECK_NOT_NULL(env);
+  SetInternalFields(env->isolate_data(), args.This(), nullptr);
 }
 
 Local<FunctionTemplate> BaseObject::MakeLazilyInitializedJSTemplate(

src/base_object.h

@@ -41,8 +41,6 @@ namespace worker {
 class TransferData;
 }
 
-extern uint16_t kNodeEmbedderId;
-
 class BaseObject : public MemoryRetainer {
  public:
   enum InternalFields { kEmbedderType, kSlot, kInternalFieldCount };
@@ -74,10 +72,13 @@ class BaseObject : public MemoryRetainer {
   // was also passed to the `BaseObject()` constructor initially.
   // This may return `nullptr` if the C++ object has not been constructed yet,
   // e.g. when the JS object used `MakeLazilyInitializedJSTemplate`.
-  static inline void SetInternalFields(v8::Local<v8::Object> object,
+  static inline void SetInternalFields(IsolateData* isolate_data,
+                                       v8::Local<v8::Object> object,
                                        void* slot);
-  static inline bool IsBaseObject(v8::Local<v8::Object> object);
-  static inline void TagBaseObject(v8::Local<v8::Object> object);
+  static inline bool IsBaseObject(IsolateData* isolate_data,
+                                  v8::Local<v8::Object> object);
+  static inline void TagBaseObject(IsolateData* isolate_data,
+                                   v8::Local<v8::Object> object);
   static void LazilyInitializedJSTemplateConstructor(
       const v8::FunctionCallbackInfo<v8::Value>& args);
   static inline BaseObject* FromJSObject(v8::Local<v8::Value> object);

src/env-inl.h

@@ -61,6 +61,14 @@ inline uv_loop_t* IsolateData::event_loop() const {
   return event_loop_;
 }
 
+inline uint16_t* IsolateData::embedder_id_for_cppgc() const {
+  return &(wrapper_data_->cppgc_id);
+}
+
+inline uint16_t* IsolateData::embedder_id_for_non_cppgc() const {
+  return &(wrapper_data_->non_cppgc_id);
+}
+
 inline NodeArrayBufferAllocator* IsolateData::node_allocator() const {
   return node_allocator_;
 }

src/env.cc

@@ -19,6 +19,7 @@
 #include "tracing/agent.h"
 #include "tracing/traced_value.h"
 #include "util-inl.h"
+#include "v8-cppgc.h"
 #include "v8-profiler.h"
 
 #include <algorithm>
@@ -28,6 +29,7 @@
 #include <iostream>
 #include <limits>
 #include <memory>
+#include <unordered_map>
 
 namespace node {
 
@@ -497,6 +499,11 @@ void IsolateData::CreateProperties() {
   contextify::ContextifyContext::InitializeGlobalTemplates(this);
 }
 
+constexpr uint16_t kDefaultCppGCEmebdderID = 0x90de;
+Mutex IsolateData::isolate_data_mutex_;
+std::unordered_map<uint16_t, std::unique_ptr<PerIsolateWrapperData>>
+    IsolateData::wrapper_data_map_;
+
 IsolateData::IsolateData(Isolate* isolate,
                          uv_loop_t* event_loop,
                          MultiIsolatePlatform* platform,
@@ -510,6 +517,46 @@ IsolateData::IsolateData(Isolate* isolate,
       snapshot_data_(snapshot_data) {
   options_.reset(
       new PerIsolateOptions(*(per_process::cli_options->per_isolate)));
+  v8::CppHeap* cpp_heap = isolate->GetCppHeap();
+
+  uint16_t cppgc_id = kDefaultCppGCEmebdderID;
+  if (cpp_heap != nullptr) {
+    // The general convention of the wrappable layout for cppgc in the
+    // ecosystem is:
+    // [  0  ] -> embedder id
+    // [  1  ] -> wrappable instance
+    // If the Isolate includes a CppHeap attached by another embedder,
+    // And if they also use the field 0 for the ID, we DCHECK that
+    // the layout matches our layout, and record the embedder ID for cppgc
+    // to avoid accidentally enabling cppgc on non-cppgc-managed wrappers .
+    v8::WrapperDescriptor descriptor = cpp_heap->wrapper_descriptor();
+    if (descriptor.wrappable_type_index == BaseObject::kEmbedderType) {
+      cppgc_id = descriptor.embedder_id_for_garbage_collected;
+      DCHECK_EQ(descriptor.wrappable_instance_index, BaseObject::kSlot);
+    }
+    // If the CppHeap uses the slot we use to put non-cppgc-traced BaseObject
+    // for embedder ID, V8 could accidentally enable cppgc on them. So
+    // safe guard against this.
+    DCHECK_NE(descriptor.wrappable_type_index, BaseObject::kSlot);
+  }
+  // We do not care about overflow since we just want this to be different
+  // from the cppgc id.
+  uint16_t non_cppgc_id = cppgc_id + 1;
+
+  {
+    // GC could still be run after the IsolateData is destroyed, so we store
+    // the ids in a static map to ensure pointers to them are still valid
+    // then. In practice there should be very few variants of the cppgc id
+    // in one process so the size of this map should be very small.
+    node::Mutex::ScopedLock lock(isolate_data_mutex_);
+    auto it = wrapper_data_map_.find(cppgc_id);
+    if (it == wrapper_data_map_.end()) {
+      auto pair = wrapper_data_map_.emplace(
+          cppgc_id, new PerIsolateWrapperData{cppgc_id, non_cppgc_id});
+      it = pair.first;
+    }
+    wrapper_data_ = it->second.get();
+  }
 
   if (snapshot_data == nullptr) {
     CreateProperties();

src/env.h

@@ -124,13 +124,19 @@ struct IsolateDataSerializeInfo {
                                   const IsolateDataSerializeInfo& i);
 };
 
+struct PerIsolateWrapperData {
+  uint16_t cppgc_id;
+  uint16_t non_cppgc_id;
+};
+
 class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
  public:
   IsolateData(v8::Isolate* isolate,
               uv_loop_t* event_loop,
               MultiIsolatePlatform* platform = nullptr,
               ArrayBufferAllocator* node_allocator = nullptr,
               const SnapshotData* snapshot_data = nullptr);
+
   SET_MEMORY_INFO_NAME(IsolateData)
   SET_SELF_SIZE(IsolateData)
   void MemoryInfo(MemoryTracker* tracker) const override;
@@ -139,6 +145,9 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
   bool is_building_snapshot() const { return is_building_snapshot_; }
   void set_is_building_snapshot(bool value) { is_building_snapshot_ = value; }
 
+  uint16_t* embedder_id_for_cppgc() const;
+  uint16_t* embedder_id_for_non_cppgc() const;
+
   inline uv_loop_t* event_loop() const;
   inline MultiIsolatePlatform* platform() const;
   inline const SnapshotData* snapshot_data() const;
@@ -223,6 +232,11 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
   std::shared_ptr<PerIsolateOptions> options_;
   worker::Worker* worker_context_ = nullptr;
   bool is_building_snapshot_ = false;
+  PerIsolateWrapperData* wrapper_data_;
+
+  static Mutex isolate_data_mutex_;
+  static std::unordered_map<uint16_t, std::unique_ptr<PerIsolateWrapperData>>
+      wrapper_data_map_;
 };
 
 struct ContextInfo {

src/node_messaging.cc

@@ -307,15 +307,15 @@ class SerializerDelegate : public ValueSerializer::Delegate {
   bool HasCustomHostObject(Isolate* isolate) override { return true; }
 
   Maybe<bool> IsHostObject(Isolate* isolate, Local<Object> object) override {
-    if (BaseObject::IsBaseObject(object)) {
+    if (BaseObject::IsBaseObject(env_->isolate_data(), object)) {
       return Just(true);
     }
 
     return Just(JSTransferable::IsJSTransferable(env_, context_, object));
   }
 
   Maybe<bool> WriteHostObject(Isolate* isolate, Local<Object> object) override {
-    if (BaseObject::IsBaseObject(object)) {
+    if (BaseObject::IsBaseObject(env_->isolate_data(), object)) {
       return WriteHostObject(
           BaseObjectPtr<BaseObject> { Unwrap<BaseObject>(object) });
     }
@@ -529,7 +529,7 @@ Maybe<bool> Message::Serialize(Environment* env,
       return Nothing<bool>();
     }
     BaseObjectPtr<BaseObject> host_object;
-    if (BaseObject::IsBaseObject(entry)) {
+    if (BaseObject::IsBaseObject(env->isolate_data(), entry)) {
       host_object = BaseObjectPtr<BaseObject>{Unwrap<BaseObject>(entry)};
     } else {
       if (!JSTransferable::IsJSTransferable(env, context, entry)) {
@@ -1328,7 +1328,7 @@ JSTransferable::NestedTransferables() const {
       continue;
     }
     Local<Object> obj = value.As<Object>();
-    if (BaseObject::IsBaseObject(obj)) {
+    if (BaseObject::IsBaseObject(env()->isolate_data(), obj)) {
       ret.emplace_back(Unwrap<BaseObject>(obj));
       continue;
     }

src/node_snapshotable.cc

@@ -1159,14 +1159,16 @@ void DeserializeNodeInternalFields(Local<Object> holder,
 
 StartupData SerializeNodeContextInternalFields(Local<Object> holder,
                                                int index,
-                                               void* env) {
+                                               void* callback_data) {
   // 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 || !BaseObject::IsBaseObject(holder)) {
+  Environment* env = static_cast<Environment*>(callback_data);
+  if (index != BaseObject::kEmbedderType ||
+      !BaseObject::IsBaseObject(env->isolate_data(), holder)) {
     return StartupData{nullptr, 0};
   }
 

test/cctest/node_test_fixture.cc

@@ -34,6 +34,7 @@ void NodeTestEnvironment::SetUp() {
 }
 
 void NodeTestEnvironment::TearDown() {
+  cppgc::ShutdownProcess();
   v8::V8::Dispose();
   v8::V8::DisposePlatform();
   NodeZeroIsolateTestFixture::platform->Shutdown();