diff --git a/src/api/embed_helpers.cc b/src/api/embed_helpers.cc index 6498c09ee97..03800832cd6 100644 --- a/src/api/embed_helpers.cc +++ b/src/api/embed_helpers.cc @@ -234,14 +234,18 @@ CommonEnvironmentSetup::~CommonEnvironmentSetup() { } bool platform_finished = false; - impl_->platform->AddIsolateFinishedCallback(isolate, [](void* data) { - *static_cast(data) = true; - }, &platform_finished); - impl_->platform->UnregisterIsolate(isolate); + impl_->platform->AddIsolateFinishedCallback( + isolate, + [](void* data) { + bool* ptr = static_cast(data); + *ptr = true; + }, + &platform_finished); if (impl_->snapshot_creator.has_value()) { impl_->snapshot_creator.reset(); } isolate->Dispose(); + impl_->platform->UnregisterIsolate(isolate); // Wait until the platform has cleaned up all relevant resources. while (!platform_finished) diff --git a/src/api/environment.cc b/src/api/environment.cc index 21cbfbfdf12..cdc0c5afb75 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -20,12 +20,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; @@ -326,6 +329,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.cc b/src/env.cc index d58ab96971e..6edb0d45b27 100644 --- a/src/env.cc +++ b/src/env.cc @@ -44,8 +44,6 @@ using v8::BackingStore; using v8::BackingStoreInitializationMode; using v8::Boolean; using v8::Context; -using v8::CppHeap; -using v8::CppHeapCreateParams; using v8::EmbedderGraph; using v8::EscapableHandleScope; using v8::ExternalMemoryAccounter; @@ -580,19 +578,10 @@ IsolateData::IsolateData(Isolate* isolate, platform_(platform), snapshot_data_(snapshot_data), options_(std::move(options)) { - v8::CppHeap* cpp_heap = isolate->GetCppHeap(); - uint16_t cppgc_id = kDefaultCppGCEmbedderID; // 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; - if (cpp_heap == nullptr) { - cpp_heap_ = CppHeap::Create(platform, v8::CppHeapCreateParams{{}}); - // TODO(joyeecheung): pass it into v8::Isolate::CreateParams and let V8 - // own it when we can keep the isolate registered/task runner discoverable - // during isolate disposal. - isolate->AttachCppHeap(cpp_heap_.get()); - } { // GC could still be run after the IsolateData is destroyed, so we store @@ -616,14 +605,7 @@ IsolateData::IsolateData(Isolate* isolate, } } -IsolateData::~IsolateData() { - if (cpp_heap_ != nullptr) { - v8::Locker locker(isolate_); - // The CppHeap must be detached before being terminated. - isolate_->DetachCppHeap(); - cpp_heap_->Terminate(); - } -} +IsolateData::~IsolateData() {} // Deprecated API, embedders should use v8::Object::Wrap() directly instead. void SetCppgcReference(Isolate* isolate, diff --git a/src/env.h b/src/env.h index 95f369a188f..b428f5937b9 100644 --- a/src/env.h +++ b/src/env.h @@ -68,10 +68,6 @@ #include #include -namespace v8 { -class CppHeap; -} - namespace node { namespace shadow_realm { @@ -246,7 +242,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.cc b/src/node.cc index 9f41dc84340..1941404f6fa 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1211,6 +1211,14 @@ InitializeOncePerProcessInternal(const std::vector& args, result->platform_ = per_process::v8_platform.Platform(); } + if (!(flags & ProcessInitializationFlags::kNoInitializeCppgc)) { + v8::PageAllocator* allocator = nullptr; + if (result->platform_ != nullptr) { + allocator = result->platform_->GetPageAllocator(); + } + cppgc::InitializeProcess(allocator); + } + if (!(flags & ProcessInitializationFlags::kNoInitializeV8)) { V8::Initialize(); @@ -1220,14 +1228,6 @@ InitializeOncePerProcessInternal(const std::vector& args, absl::SetMutexDeadlockDetectionMode(absl::OnDeadlockCycle::kIgnore); } - if (!(flags & ProcessInitializationFlags::kNoInitializeCppgc)) { - v8::PageAllocator* allocator = nullptr; - if (result->platform_ != nullptr) { - allocator = result->platform_->GetPageAllocator(); - } - cppgc::InitializeProcess(allocator); - } - #if NODE_USE_V8_WASM_TRAP_HANDLER bool use_wasm_trap_handler = !per_process::cli_options->disable_wasm_trap_handler; diff --git a/src/node.h b/src/node.h index 8c72fecaa04..c2b6148c783 100644 --- a/src/node.h +++ b/src/node.h @@ -447,7 +447,7 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform { // This function may only be called once per `Isolate`, and discard any // pending delayed tasks scheduled for that isolate. - // This needs to be called right before calling `Isolate::Dispose()`. + // This needs to be called right after calling `Isolate::Dispose()`. virtual void UnregisterIsolate(v8::Isolate* isolate) = 0; // The platform should call the passed function once all state associated @@ -489,6 +489,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 diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 4eee89d3c01..9669961c6ef 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -328,7 +328,7 @@ ContextifyContext* ContextifyContext::New(Local v8_context, .ToLocal(&wrapper)) { return {}; } - + DCHECK_NOT_NULL(env->isolate()->GetCppHeap()); result = cppgc::MakeGarbageCollected( env->isolate()->GetCppHeap()->GetAllocationHandle(), env, @@ -975,6 +975,7 @@ void ContextifyScript::RegisterExternalReferences( ContextifyScript* ContextifyScript::New(Environment* env, Local object) { + DCHECK_NOT_NULL(env->isolate()->GetCppHeap()); return cppgc::MakeGarbageCollected( env->isolate()->GetCppHeap()->GetAllocationHandle(), env, object); } diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 4119ac1b002..2d20b8d3a99 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -81,9 +81,10 @@ NodeMainInstance::~NodeMainInstance() { // This should only be done on a main instance that owns its isolate. // IsolateData must be freed before UnregisterIsolate() is called. isolate_data_.reset(); - platform_->UnregisterIsolate(isolate_); } + // TODO(joyeecheung): split Isolate::Free() here? isolate_->Dispose(); + platform_->UnregisterIsolate(isolate_); } ExitCode NodeMainInstance::Run() { diff --git a/src/node_worker.cc b/src/node_worker.cc index 25be2bca2cd..4b22f1428cd 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -235,8 +235,14 @@ class WorkerThreadData { // new Isolate at the same address can successfully be registered with // the platform. // (Refs: https://github.com/nodejs/node/issues/30846) - w_->platform_->UnregisterIsolate(isolate); + // TODO(joyeecheung): we reversed the order because the task runner has + // to be available to handle GC tasks posted during isolate disposal. + // If this flakes comes back again, try splitting Isolate::Delete out + // so that the pointer is still available as map key after disposal + // and we delete it after unregistration. + // Refs: https://github.com/nodejs/node/pull/53086#issuecomment-2128056793 isolate->Dispose(); + w_->platform_->UnregisterIsolate(isolate); // Wait until the platform has cleaned up all relevant resources. while (!platform_finished) { diff --git a/src/util.cc b/src/util.cc index c8b468443c2..ff2d72e9b32 100644 --- a/src/util.cc +++ b/src/util.cc @@ -726,12 +726,15 @@ RAIIIsolateWithoutEntering::RAIIIsolateWithoutEntering(const SnapshotData* data) SnapshotBuilder::InitializeIsolateParams(data, ¶ms); } params.array_buffer_allocator = allocator_.get(); + params.cpp_heap = v8::CppHeap::Create(per_process::v8_platform.Platform(), + v8::CppHeapCreateParams{{}}) + .release(); Isolate::Initialize(isolate_, params); } RAIIIsolateWithoutEntering::~RAIIIsolateWithoutEntering() { - per_process::v8_platform.Platform()->UnregisterIsolate(isolate_); isolate_->Dispose(); + per_process::v8_platform.Platform()->UnregisterIsolate(isolate_); } RAIIIsolate::RAIIIsolate(const SnapshotData* data) diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index 3414c0be8ad..82013ffe766 100644 --- a/test/cctest/node_test_fixture.h +++ b/test/cctest/node_test_fixture.h @@ -123,8 +123,8 @@ class NodeTestFixture : public NodeZeroIsolateTestFixture { void TearDown() override { platform->DrainTasks(isolate_); isolate_->Exit(); - platform->UnregisterIsolate(isolate_); isolate_->Dispose(); + platform->UnregisterIsolate(isolate_); isolate_ = nullptr; NodeZeroIsolateTestFixture::TearDown(); } diff --git a/test/cctest/test_cppgc.cc b/test/cctest/test_cppgc.cc index edd413ae9b9..fef784e47b3 100644 --- a/test/cctest/test_cppgc.cc +++ b/test/cctest/test_cppgc.cc @@ -46,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{{}}); - - // TODO(joyeecheung): pass it into v8::Isolate::CreateParams and let V8 - // own it when we can keep the isolate registered/task runner discoverable - // during isolate disposal. - 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. { @@ -100,15 +94,10 @@ TEST_F(NodeZeroIsolateTestFixture, ExistingCppHeapTest) { } platform->DrainTasks(isolate); - - // Cleanup. - isolate->DetachCppHeap(); - cpp_heap->Terminate(); - platform->DrainTasks(isolate); } - platform->UnregisterIsolate(isolate); isolate->Dispose(); + platform->UnregisterIsolate(isolate); // Check that all the objects are created and destroyed properly. EXPECT_EQ(CppGCed::kConstructCount, 100); diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 008cda77b65..7edf5d2e9c6 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -544,8 +544,8 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { node::FreeIsolateData(isolate_data); } - data->platform->UnregisterIsolate(isolate); isolate->Dispose(); + data->platform->UnregisterIsolate(isolate); uv_run(&loop, UV_RUN_DEFAULT); CHECK_EQ(uv_loop_close(&loop), 0); @@ -625,6 +625,9 @@ TEST_F(NodeZeroIsolateTestFixture, CtrlCWithOnlySafeTerminationTest) { // Allocate and initialize Isolate. v8::Isolate::CreateParams create_params; create_params.array_buffer_allocator = allocator.get(); + create_params.cpp_heap = + v8::CppHeap::Create(platform.get(), v8::CppHeapCreateParams{{}}) + .release(); v8::Isolate* isolate = v8::Isolate::Allocate(); CHECK_NOT_NULL(isolate); platform->RegisterIsolate(isolate, ¤t_loop); @@ -675,8 +678,8 @@ TEST_F(NodeZeroIsolateTestFixture, CtrlCWithOnlySafeTerminationTest) { } // Cleanup. - platform->UnregisterIsolate(isolate); isolate->Dispose(); + platform->UnregisterIsolate(isolate); } #endif // _WIN32 diff --git a/test/cctest/test_platform.cc b/test/cctest/test_platform.cc index 037b6b9cffa..d4a11d9404f 100644 --- a/test/cctest/test_platform.cc +++ b/test/cctest/test_platform.cc @@ -66,6 +66,9 @@ TEST_F(NodeZeroIsolateTestFixture, IsolatePlatformDelegateTest) { // Allocate isolate v8::Isolate::CreateParams create_params; create_params.array_buffer_allocator = allocator.get(); + create_params.cpp_heap = + v8::CppHeap::Create(platform.get(), v8::CppHeapCreateParams{{}}) + .release(); auto isolate = v8::Isolate::Allocate(); CHECK_NOT_NULL(isolate); @@ -104,8 +107,8 @@ TEST_F(NodeZeroIsolateTestFixture, IsolatePlatformDelegateTest) { // Graceful shutdown delegate->Shutdown(); - platform->UnregisterIsolate(isolate); isolate->Dispose(); + platform->UnregisterIsolate(isolate); } TEST_F(PlatformTest, TracingControllerNullptr) { diff --git a/test/embedding/embedtest.cc b/test/embedding/embedtest.cc index f481efc59a6..f0651ad8a85 100644 --- a/test/embedding/embedtest.cc +++ b/test/embedding/embedtest.cc @@ -2,6 +2,7 @@ #undef NDEBUG #endif #include +#include "cppgc/platform.h" #include "executable_wrapper.h" #include "node.h" @@ -43,6 +44,7 @@ NODE_MAIN(int argc, node::argv_type raw_argv[]) { // support in the future, split this configuration out as a // command line option. node::ProcessInitializationFlags::kDisableNodeOptionsEnv, + node::ProcessInitializationFlags::kNoInitializeCppgc, }); for (const std::string& error : result->errors()) @@ -54,6 +56,7 @@ NODE_MAIN(int argc, node::argv_type raw_argv[]) { std::unique_ptr platform = MultiIsolatePlatform::Create(4); V8::InitializePlatform(platform.get()); + cppgc::InitializeProcess(platform->GetPageAllocator()); V8::Initialize(); int ret = diff --git a/test/fuzzers/fuzz_env.cc b/test/fuzzers/fuzz_env.cc index bace3051f8c..5ca295848a9 100644 --- a/test/fuzzers/fuzz_env.cc +++ b/test/fuzzers/fuzz_env.cc @@ -65,8 +65,8 @@ public: void Teardown() { platform->DrainTasks(isolate_); isolate_->Exit(); - platform->UnregisterIsolate(isolate_); isolate_->Dispose(); + platform->UnregisterIsolate(isolate_); isolate_ = nullptr; } }; diff --git a/test/fuzzers/fuzz_strings.cc b/test/fuzzers/fuzz_strings.cc index 8f5e1a473e3..936876cdae2 100644 --- a/test/fuzzers/fuzz_strings.cc +++ b/test/fuzzers/fuzz_strings.cc @@ -72,8 +72,8 @@ public: void Teardown() { platform->DrainTasks(isolate_); isolate_->Exit(); - platform->UnregisterIsolate(isolate_); isolate_->Dispose(); + platform->UnregisterIsolate(isolate_); isolate_ = nullptr; } };