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: https://github.com/nodejs/node/pull/48660
Refs: 9327503128
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

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,6 +124,11 @@ 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,
@@ -131,6 +136,7 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
               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,7 +307,7 @@ 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);
     }
 
@@ -315,7 +315,7 @@ class SerializerDelegate : public ValueSerializer::Delegate {
   }
 
   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

@@ -1164,14 +1164,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();

test/cctest/test_cppgc.cc

@@ -0,0 +1,127 @@
+#include <cppgc/allocation.h>
+#include <cppgc/garbage-collected.h>
+#include <cppgc/heap.h>
+#include <node.h>
+#include <v8-cppgc.h>
+#include <v8.h>
+#include "node_test_fixture.h"
+
+// This tests that Node.js can work with an existing CppHeap.
+
+// Mimic the Blink layout.
+static int kWrappableTypeIndex = 0;
+static int kWrappableInstanceIndex = 1;
+static uint16_t kEmbedderID = 0x1;
+
+// Mimic a class that does not know about Node.js.
+class CppGCed : public cppgc::GarbageCollected<CppGCed> {
+ public:
+  static int kConstructCount;
+  static int kDestructCount;
+  static int kTraceCount;
+
+  static void New(const v8::FunctionCallbackInfo<v8::Value>& args) {
+    v8::Isolate* isolate = args.GetIsolate();
+    v8::Local<v8::Object> js_object = args.This();
+    CppGCed* gc_object = cppgc::MakeGarbageCollected<CppGCed>(
+        isolate->GetCppHeap()->GetAllocationHandle());
+    js_object->SetAlignedPointerInInternalField(kWrappableTypeIndex,
+                                                &kEmbedderID);
+    js_object->SetAlignedPointerInInternalField(kWrappableInstanceIndex,
+                                                gc_object);
+    kConstructCount++;
+    args.GetReturnValue().Set(js_object);
+  }
+
+  static v8::Local<v8::Function> GetConstructor(
+      v8::Local<v8::Context> context) {
+    auto ft = v8::FunctionTemplate::New(context->GetIsolate(), New);
+    auto ot = ft->InstanceTemplate();
+    ot->SetInternalFieldCount(2);
+    return ft->GetFunction(context).ToLocalChecked();
+  }
+
+  CppGCed() = default;
+
+  ~CppGCed() { kDestructCount++; }
+
+  void Trace(cppgc::Visitor* visitor) const { kTraceCount++; }
+};
+
+int CppGCed::kConstructCount = 0;
+int CppGCed::kDestructCount = 0;
+int CppGCed::kTraceCount = 0;
+
+TEST_F(NodeZeroIsolateTestFixture, ExistingCppHeapTest) {
+  v8::Isolate* isolate =
+      node::NewIsolate(allocator.get(), &current_loop, platform.get());
+
+  // Create and attach the CppHeap before we set up the IsolateData so that
+  // it recognizes the existing heap.
+  std::unique_ptr<v8::CppHeap> cpp_heap = v8::CppHeap::Create(
+      platform.get(),
+      v8::CppHeapCreateParams(
+          {},
+          v8::WrapperDescriptor(
+              kWrappableTypeIndex, kWrappableInstanceIndex, kEmbedderID)));
+  isolate->AttachCppHeap(cpp_heap.get());
+
+  // Try creating Context + IsolateData + Environment.
+  {
+    v8::Isolate::Scope isolate_scope(isolate);
+    v8::HandleScope handle_scope(isolate);
+
+    std::unique_ptr<node::IsolateData, decltype(&node::FreeIsolateData)>
+        isolate_data{
+            node::CreateIsolateData(isolate, &current_loop, platform.get()),
+            node::FreeIsolateData};
+    CHECK(isolate_data);
+
+    {
+      auto context = node::NewContext(isolate);
+      CHECK(!context.IsEmpty());
+      v8::Context::Scope context_scope(context);
+
+      // An Environment should already contain a few BaseObjects that are
+      // supposed to have non-cppgc IDs.
+      std::unique_ptr<node::Environment, decltype(&node::FreeEnvironment)>
+          environment{
+              node::CreateEnvironment(isolate_data.get(), context, {}, {}),
+              node::FreeEnvironment};
+      CHECK(environment);
+
+      context->Global()
+          ->Set(context,
+                v8::String::NewFromUtf8(isolate, "CppGCed").ToLocalChecked(),
+                CppGCed::GetConstructor(context))
+          .FromJust();
+
+      const char* code =
+          "globalThis.array = [];"
+          "for (let i = 0; i < 100; ++i) { array.push(new CppGCed()); }";
+      node::LoadEnvironment(environment.get(), code).ToLocalChecked();
+      // Request a GC and check if CppGCed::Trace() is invoked.
+      isolate->LowMemoryNotification();
+      int exit_code = SpinEventLoop(environment.get()).FromJust();
+      EXPECT_EQ(exit_code, 0);
+    }
+
+    platform->DrainTasks(isolate);
+
+    // Cleanup.
+    isolate->DetachCppHeap();
+    cpp_heap->Terminate();
+    platform->DrainTasks(isolate);
+  }
+
+  platform->UnregisterIsolate(isolate);
+  isolate->Dispose();
+
+  // Check that all the objects are created and destroyed properly.
+  EXPECT_EQ(CppGCed::kConstructCount, 100);
+  EXPECT_EQ(CppGCed::kDestructCount, 100);
+  // GC does not have to feel pressured enough to visit all of them to
+  // reclaim memory before we are done with the isolate and the entire
+  // heap can be reclaimed. So just check at least some of them are traced.
+  EXPECT_GT(CppGCed::kTraceCount, 0);
+}