Skip to content

Commit

Permalink
Don't use WrapperDescriptor and instead use Wrap/Unwrap APIs (#187) (#…
Browse files Browse the repository at this point in the history
…188)

Co-authored-by: Michael Lippautz <[email protected]>
  • Loading branch information
2 people authored and pthier committed Jul 15, 2024
1 parent a09ad15 commit 0513198
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 56 deletions.
22 changes: 2 additions & 20 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
24 changes: 2 additions & 22 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ using v8::TryCatch;
using v8::Uint32;
using v8::Undefined;
using v8::Value;
using v8::WrapperDescriptor;
using worker::Worker;

int const ContextEmbedderTag::kNodeContextTag = 0x6e6f64;
Expand Down Expand Up @@ -555,30 +554,11 @@ IsolateData::IsolateData(Isolate* isolate,

uint16_t cppgc_id = kDefaultCppGCEmbedderID;
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
Expand Down
5 changes: 0 additions & 5 deletions test/addons/cppgc-object/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
12 changes: 3 additions & 9 deletions test/cctest/test_cppgc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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();
}

Expand All @@ -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.
Expand Down

0 comments on commit 0513198

Please sign in to comment.