From 015829e5ad88a2db9e3e66f2d7d889637e07ec38 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 20 May 2024 16:59:20 +0200 Subject: [PATCH] src: remove dependency on wrapper-descriptor-based cpp heap As V8 is moving towards built-in CppHeap creation, change the management so that the automatic CppHeap creation on Node.js's end is also enforced at Isolate creation time. 1. If embedder uses NewIsolate(), either they use IsolateSettings::cpp_heap to specify a CppHeap that will be owned by V8, or if it's not configured, Node.js will create a CppHeap that will be owned by V8. 2. If the embedder uses SetIsolateUpForNode(), IsolateSettings::cpp_heap will be ignored (as V8 has deprecated attaching CppHeap post-isolate-creation). The embedders need to ensure that the v8::Isolate has a CppHeap attached while it's still used by Node.js, preferably using v8::CreateParams. See https://issues.chromium.org/issues/42203693 for details. In future version of V8, this CppHeap will be created by V8 if not provided, and we can remove our own "if no CppHeap provided, create one" code in NewIsolate(). This also deprecates node::SetCppgcReference() in favor of v8::Object::Wrap() since the wrapper descriptor is no longer relevant. It is still kept as a compatibility layer for addons that need to also work on Node.js versions without v8::Object::Wrap(). --- src/api/environment.cc | 11 ++++++ src/env-inl.h | 26 +-------------- src/env.cc | 52 +++++++---------------------- src/env.h | 9 ----- src/node.h | 40 ++++++++++++---------- test/addons/cppgc-object/binding.cc | 14 ++++---- test/cctest/test_cppgc.cc | 42 +++++++---------------- 7 files changed, 64 insertions(+), 130 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 2cc2108b4c0f1c..c22f581f60d909 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -19,12 +19,15 @@ #if HAVE_INSPECTOR #include "inspector/worker_inspector.h" // ParentInspectorHandle #endif +#include "v8-cppgc.h" namespace node { using errors::TryCatchScope; using v8::Array; using v8::Boolean; using v8::Context; +using v8::CppHeap; +using v8::CppHeapCreateParams; using v8::EscapableHandleScope; using v8::Function; using v8::FunctionCallbackInfo; @@ -354,6 +357,14 @@ Isolate* NewIsolate(Isolate::CreateParams* params, // so that the isolate can access the platform during initialization. platform->RegisterIsolate(isolate, event_loop); + // Ensure that there is always a CppHeap. + if (settings.cpp_heap == nullptr) { + params->cpp_heap = + CppHeap::Create(platform, CppHeapCreateParams{{}}).release(); + } else { + params->cpp_heap = settings.cpp_heap; + } + SetIsolateCreateParamsForNode(params); Isolate::Initialize(isolate, *params); diff --git a/src/env-inl.h b/src/env-inl.h index 116cbf4c4d5144..ffee257f1620d1 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -35,6 +35,7 @@ #include "util-inl.h" #include "uv.h" #include "v8-cppgc.h" +#include "v8-sandbox.h" #include "v8.h" #include @@ -62,31 +63,6 @@ inline uv_loop_t* IsolateData::event_loop() const { return event_loop_; } -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); -} - inline uint16_t* IsolateData::embedder_id_for_cppgc() const { return &(wrapper_data_->cppgc_id); } diff --git a/src/env.cc b/src/env.cc index 3d36416cdfa0f9..14ab9188faa24c 100644 --- a/src/env.cc +++ b/src/env.cc @@ -40,8 +40,6 @@ using errors::TryCatchScope; using v8::Array; using v8::Boolean; using v8::Context; -using v8::CppHeap; -using v8::CppHeapCreateParams; using v8::EmbedderGraph; using v8::EscapableHandleScope; using v8::Function; @@ -70,7 +68,6 @@ using v8::TryCatch; using v8::Uint32; using v8::Undefined; using v8::Value; -using v8::WrapperDescriptor; using worker::Worker; int const ContextEmbedderTag::kNodeContextTag = 0x6e6f64; @@ -520,6 +517,14 @@ void IsolateData::CreateProperties() { CreateEnvProxyTemplate(this); } +// Previously, the general convention of the wrappable layout for cppgc in +// the ecosystem is: +// [ 0 ] -> embedder id +// [ 1 ] -> wrappable instance +// Now V8 has deprecated this layout-based tracing enablement, embeders +// should simply use v8::Object::Wrap() and v8::Object::Unwrap(). We preserve +// this layout only to distinguish internally how the memory of a Node.js +// wrapper is managed or whether a wrapper is managed by Node.js. constexpr uint16_t kDefaultCppGCEmebdderID = 0x90de; Mutex IsolateData::isolate_data_mutex_; std::unordered_map> @@ -538,36 +543,8 @@ 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); - } else { - cpp_heap_ = CppHeap::Create( - platform, - CppHeapCreateParams{ - {}, - WrapperDescriptor( - BaseObject::kEmbedderType, BaseObject::kSlot, cppgc_id)}); - isolate->AttachCppHeap(cpp_heap_.get()); - } // 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; @@ -594,19 +571,14 @@ IsolateData::IsolateData(Isolate* isolate, } } -IsolateData::~IsolateData() { - if (cpp_heap_ != nullptr) { - // The CppHeap must be detached before being terminated. - isolate_->DetachCppHeap(); - cpp_heap_->Terminate(); - } -} +IsolateData::~IsolateData() {} -// Public API +// Deprecated API, embedders should use v8::Object::Wrap() directly instead. void SetCppgcReference(Isolate* isolate, Local object, void* wrappable) { - IsolateData::SetCppgcReference(isolate, object, wrappable); + v8::Object::Wrap( + isolate, object, wrappable); } void IsolateData::MemoryInfo(MemoryTracker* tracker) const { diff --git a/src/env.h b/src/env.h index 58419bd31f1d65..58a51b8a6a50d9 100644 --- a/src/env.h +++ b/src/env.h @@ -67,10 +67,6 @@ #include #include -namespace v8 { -class CppHeap; -} - namespace node { namespace shadow_realm { @@ -165,10 +161,6 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer { uint16_t* embedder_id_for_cppgc() const; uint16_t* embedder_id_for_non_cppgc() const; - static inline void SetCppgcReference(v8::Isolate* isolate, - v8::Local object, - void* wrappable); - inline uv_loop_t* event_loop() const; inline MultiIsolatePlatform* platform() const; inline const SnapshotData* snapshot_data() const; @@ -253,7 +245,6 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer { const SnapshotData* snapshot_data_; std::optional snapshot_config_; - std::unique_ptr cpp_heap_; std::shared_ptr options_; worker::Worker* worker_context_ = nullptr; PerIsolateWrapperData* wrapper_data_; diff --git a/src/node.h b/src/node.h index 95e13cd9ea2b49..5ac0cf545d29ef 100644 --- a/src/node.h +++ b/src/node.h @@ -491,6 +491,21 @@ struct IsolateSettings { allow_wasm_code_generation_callback = nullptr; v8::ModifyCodeGenerationFromStringsCallback2 modify_code_generation_from_strings_callback = nullptr; + + // When the settings is passed to NewIsolate(): + // - If cpp_heap is not nullptr, this CppHeap will be used to create + // the isolate and its ownership will be passed to V8. + // - If this is nullptr, Node.js will create a CppHeap that will be + // owned by V8. + // + // When the settings is passed to SetIsolateUpForNode(): + // cpp_heap will be ignored. Embedders must ensure that the + // v8::Isolate has a CppHeap attached while it's still used by + // Node.js, for example using v8::CreateParams. + // + // See https://issues.chromium.org/issues/42203693. In future version + // of V8, this CppHeap will be created by V8 if not provided. + v8::CppHeap* cpp_heap = nullptr; }; // Represents a startup snapshot blob, e.g. created by passing @@ -1548,24 +1563,13 @@ void RegisterSignalHandler(int signal, bool reset_handler = false); #endif // _WIN32 -// Configure the layout of the JavaScript object with a cppgc::GarbageCollected -// instance so that when the JavaScript object is reachable, the garbage -// collected instance would have its Trace() method invoked per the cppgc -// contract. To make it work, the process must have called -// cppgc::InitializeProcess() before, which is usually the case for addons -// loaded by the stand-alone Node.js executable. Embedders of Node.js can use -// either need to call it themselves or make sure that -// ProcessInitializationFlags::kNoInitializeCppgc is *not* set for cppgc to -// work. -// If the CppHeap is owned by Node.js, which is usually the case for addon, -// the object must be created with at least two internal fields available, -// and the first two internal fields would be configured by Node.js. -// This may be superseded by a V8 API in the future, see -// https://bugs.chromium.org/p/v8/issues/detail?id=13960. Until then this -// serves as a helper for Node.js isolates. -NODE_EXTERN void SetCppgcReference(v8::Isolate* isolate, - v8::Local object, - void* wrappable); +// This is kept as a compatibility layer for addons to wrap cppgc-managed objects +// on Node.js versions without v8::Object::Wrap(). Addons created to work with +// only Node.js versions with v8::Object::Wrap() should use that instead. +NODE_DEPRECATED("Use v8::Object::Wrap()", + NODE_EXTERN void SetCppgcReference(v8::Isolate* isolate, + v8::Local object, + void* wrappable)); } // namespace node diff --git a/test/addons/cppgc-object/binding.cc b/test/addons/cppgc-object/binding.cc index 1b70ff11dc561a..7fc16a87b843ce 100644 --- a/test/addons/cppgc-object/binding.cc +++ b/test/addons/cppgc-object/binding.cc @@ -1,8 +1,10 @@ +#include #include #include #include #include #include +#include #include #include @@ -15,8 +17,10 @@ class CppGCed : public cppgc::GarbageCollected { static void New(const v8::FunctionCallbackInfo& args) { v8::Isolate* isolate = args.GetIsolate(); v8::Local js_object = args.This(); - CppGCed* gc_object = cppgc::MakeGarbageCollected( - isolate->GetCppHeap()->GetAllocationHandle()); + auto* heap = isolate->GetCppHeap(); + assert(heap != nullptr); + CppGCed* gc_object = + cppgc::MakeGarbageCollected(heap->GetAllocationHandle()); node::SetCppgcReference(isolate, js_object, gc_object); args.GetReturnValue().Set(js_object); } @@ -24,12 +28,6 @@ class CppGCed : public cppgc::GarbageCollected { static v8::Local GetConstructor( 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..a8bbd7db0f76ca 100644 --- a/test/cctest/test_cppgc.cc +++ b/test/cctest/test_cppgc.cc @@ -3,16 +3,12 @@ #include #include #include +#include #include #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 { public: @@ -23,12 +19,11 @@ class CppGCed : public cppgc::GarbageCollected { static void New(const v8::FunctionCallbackInfo& args) { v8::Isolate* isolate = args.GetIsolate(); 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); + auto* heap = isolate->GetCppHeap(); + CHECK_NOT_NULL(heap); + CppGCed* gc_object = + cppgc::MakeGarbageCollected(heap->GetAllocationHandle()); + node::SetCppgcReference(isolate, js_object, gc_object); kConstructCount++; args.GetReturnValue().Set(js_object); } @@ -36,8 +31,6 @@ class CppGCed : public cppgc::GarbageCollected { static v8::Local GetConstructor( v8::Local context) { auto ft = v8::FunctionTemplate::New(context->GetIsolate(), New); - auto ot = ft->InstanceTemplate(); - ot->SetInternalFieldCount(2); return ft->GetFunction(context).ToLocalChecked(); } @@ -53,18 +46,12 @@ int CppGCed::kDestructCount = 0; int CppGCed::kTraceCount = 0; TEST_F(NodeZeroIsolateTestFixture, ExistingCppHeapTest) { - v8::Isolate* isolate = - node::NewIsolate(allocator.get(), ¤t_loop, platform.get()); - - // Create and attach the CppHeap before we set up the IsolateData so that - // it recognizes the existing heap. - std::unique_ptr cpp_heap = v8::CppHeap::Create( - platform.get(), - v8::CppHeapCreateParams( - {}, - v8::WrapperDescriptor( - kWrappableTypeIndex, kWrappableInstanceIndex, kEmbedderID))); - isolate->AttachCppHeap(cpp_heap.get()); + node::IsolateSettings settings; + settings.cpp_heap = + v8::CppHeap::Create(platform.get(), v8::CppHeapCreateParams{{}}) + .release(); + v8::Isolate* isolate = node::NewIsolate( + allocator.get(), ¤t_loop, platform.get(), nullptr, settings); // Try creating Context + IsolateData + Environment. { @@ -107,11 +94,6 @@ TEST_F(NodeZeroIsolateTestFixture, ExistingCppHeapTest) { } platform->DrainTasks(isolate); - - // Cleanup. - isolate->DetachCppHeap(); - cpp_heap->Terminate(); - platform->DrainTasks(isolate); } platform->UnregisterIsolate(isolate);