Don't use WrapperDescriptor and instead use Wrap/Unwrap APIs (nodejs#187

) (nodejs#188) Co-authored-by: Michael Lippautz <[email protected]>
syg · Jun 3, 2024 · 32379a7 · 32379a7
1 parent d2a94a3
commit 32379a7
Show file tree

Hide file tree

Showing 4 changed files with 7 additions and 56 deletions.
diff --git a/src/env-inl.h b/src/env-inl.h
@@ -65,26 +65,8 @@ inline uv_loop_t* IsolateData::event_loop() const {
 inline void IsolateData::SetCppgcReference(v8::Isolate* isolate,
                                            v8::Local<v8::Object> object,
                                            void* wrappable) {
-  v8::CppHeap* heap = isolate->GetCppHeap();
-  CHECK_NOT_NULL(heap);
-  v8::WrapperDescriptor descriptor = heap->wrapper_descriptor();
-  uint16_t required_size = std::max(descriptor.wrappable_instance_index,
-                                    descriptor.wrappable_type_index);
-  CHECK_GT(object->InternalFieldCount(), required_size);
-
-  uint16_t* id_ptr = nullptr;
-  {
-    Mutex::ScopedLock lock(isolate_data_mutex_);
-    auto it =
-        wrapper_data_map_.find(descriptor.embedder_id_for_garbage_collected);
-    CHECK_NE(it, wrapper_data_map_.end());
-    id_ptr = &(it->second->cppgc_id);
-  }
-
-  object->SetAlignedPointerInInternalField(descriptor.wrappable_type_index,
-                                           id_ptr);
-  object->SetAlignedPointerInInternalField(descriptor.wrappable_instance_index,
-                                           wrappable);
+  v8::Object::Wrap<v8::CppHeapPointerTag::kDefaultTag>(
+      isolate, object, wrappable);
 }
 
 inline uint16_t* IsolateData::embedder_id_for_cppgc() const {

diff --git a/src/env.cc b/src/env.cc
@@ -70,7 +70,6 @@ using v8::TryCatch;
 using v8::Uint32;
 using v8::Undefined;
 using v8::Value;
-using v8::WrapperDescriptor;
 using worker::Worker;
 
 int const ContextEmbedderTag::kNodeContextTag = 0x6e6f64;
@@ -542,30 +541,11 @@ IsolateData::IsolateData(Isolate* isolate,
 
   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);
+    // All CppHeap's support only one way of wrapping objects.
   } else {
     cpp_heap_ = CppHeap::Create(
         platform,
-        CppHeapCreateParams{
-            {},
-            WrapperDescriptor(
-                BaseObject::kEmbedderType, BaseObject::kSlot, cppgc_id)});
+        CppHeapCreateParams{{}});
     isolate->AttachCppHeap(cpp_heap_.get());
   }
   // We do not care about overflow since we just want this to be different

diff --git a/test/addons/cppgc-object/binding.cc b/test/addons/cppgc-object/binding.cc
@@ -25,11 +25,6 @@ class CppGCed : public cppgc::GarbageCollected<CppGCed> {
       v8::Local<v8::Context> context) {
     auto ft = v8::FunctionTemplate::New(context->GetIsolate(), New);
     auto ot = ft->InstanceTemplate();
-    v8::WrapperDescriptor descriptor =
-        context->GetIsolate()->GetCppHeap()->wrapper_descriptor();
-    uint16_t required_size = std::max(descriptor.wrappable_instance_index,
-                                      descriptor.wrappable_type_index);
-    ot->SetInternalFieldCount(required_size + 1);
     return ft->GetFunction(context).ToLocalChecked();
   }
 

diff --git a/test/cctest/test_cppgc.cc b/test/cctest/test_cppgc.cc
@@ -25,10 +25,8 @@ class CppGCed : public cppgc::GarbageCollected<CppGCed> {
     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);
+    v8::Object::Wrap<v8::CppHeapPointerTag::kDefaultTag>(
+        isolate, js_object, gc_object);
     kConstructCount++;
     args.GetReturnValue().Set(js_object);
   }
@@ -37,7 +35,6 @@ class CppGCed : public cppgc::GarbageCollected<CppGCed> {
       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();
   }
 
@@ -60,10 +57,7 @@ TEST_F(NodeZeroIsolateTestFixture, ExistingCppHeapTest) {
   // it recognizes the existing heap.
   std::unique_ptr<v8::CppHeap> cpp_heap = v8::CppHeap::Create(
       platform.get(),
-      v8::CppHeapCreateParams(
-          {},
-          v8::WrapperDescriptor(
-              kWrappableTypeIndex, kWrappableInstanceIndex, kEmbedderID)));
+      v8::CppHeapCreateParams({}));
   isolate->AttachCppHeap(cpp_heap.get());
 
   // Try creating Context + IsolateData + Environment.