From 0daffdfb1ab01b36ef4c41cecb503cccb024d40a Mon Sep 17 00:00:00 2001 From: Michael Lippautz Date: Wed, 29 May 2024 17:26:43 +0200 Subject: [PATCH] Don't use WrapperDescriptor and instead use Wrap/Unwrap APIs (#187) --- src/env-inl.h | 22 ++-------------------- src/env.cc | 24 ++---------------------- test/addons/cppgc-object/binding.cc | 5 ----- test/cctest/test_cppgc.cc | 12 +++--------- 4 files changed, 7 insertions(+), 56 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index cdc77211667e26..342289fb0b3d90 100644 --- 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 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( + isolate, object, wrappable); } inline uint16_t* IsolateData::embedder_id_for_cppgc() const { diff --git a/src/env.cc b/src/env.cc index b8574fcfb775b8..279c5c739e5557 100644 --- 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 index 1b70ff11dc561a..67916af530731f 100644 --- a/test/addons/cppgc-object/binding.cc +++ b/test/addons/cppgc-object/binding.cc @@ -25,11 +25,6 @@ class CppGCed : public cppgc::GarbageCollected { v8::Local 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 index 49665174615870..b9da5ab8cc0392 100644 --- a/test/cctest/test_cppgc.cc +++ b/test/cctest/test_cppgc.cc @@ -25,10 +25,8 @@ class CppGCed : public cppgc::GarbageCollected { v8::Local js_object = args.This(); CppGCed* gc_object = cppgc::MakeGarbageCollected( isolate->GetCppHeap()->GetAllocationHandle()); - js_object->SetAlignedPointerInInternalField(kWrappableTypeIndex, - &kEmbedderID); - js_object->SetAlignedPointerInInternalField(kWrappableInstanceIndex, - gc_object); + v8::Object::Wrap( + isolate, js_object, gc_object); kConstructCount++; args.GetReturnValue().Set(js_object); } @@ -37,7 +35,6 @@ class CppGCed : public cppgc::GarbageCollected { v8::Local 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 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.