From cd56b965442d254ff8801446303343f36ea8acef Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 11 Mar 2025 18:54:33 +0100 Subject: [PATCH] refactor: remove usage of V8's `{Attach|Detach}CppHeap()` (#45922) * refactor: remove usage of V8's {Attach|Detach}CppHeap() * chore: remove revert patch --- patches/node/.patches | 1 + ...ch_cppgc_heap_on_v8_isolate_creation.patch | 320 ++++++++++++++++++ patches/v8/.patches | 1 - ...ated_attachcppheap_and_detachcppheap.patch | 137 -------- 4 files changed, 321 insertions(+), 138 deletions(-) create mode 100644 patches/node/refactor_attach_cppgc_heap_on_v8_isolate_creation.patch delete mode 100644 patches/v8/revert_api_delete_deprecated_attachcppheap_and_detachcppheap.patch diff --git a/patches/node/.patches b/patches/node/.patches index 9322abd8557a..d0f24de31659 100644 --- a/patches/node/.patches +++ b/patches/node/.patches @@ -46,3 +46,4 @@ fix_adjust_wpt_and_webidl_tests_for_enabled_float16array.patch chore_add_createexternalizabletwobytestring_to_globals.patch feat_add_oom_error_callback_in_node_isolatesettings.patch fix_-wnonnull_warning.patch +refactor_attach_cppgc_heap_on_v8_isolate_creation.patch diff --git a/patches/node/refactor_attach_cppgc_heap_on_v8_isolate_creation.patch b/patches/node/refactor_attach_cppgc_heap_on_v8_isolate_creation.patch new file mode 100644 index 000000000000..15b57e690506 --- /dev/null +++ b/patches/node/refactor_attach_cppgc_heap_on_v8_isolate_creation.patch @@ -0,0 +1,320 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Shelley Vohr +Date: Fri, 7 Mar 2025 11:18:41 -0600 +Subject: refactor: attach cppgc heap on v8::Isolate creation + +Refs https://issues.chromium.org/issues/42203693 + +v8/Node Commits by V8 Team: + +* https://github.com/v8/node/pull/208 +* https://github.com/v8/node/pull/209 +* https://github.com/v8/node/pull/210 +* https://github.com/v8/node/pull/211 +* https://github.com/v8/node/pull/212 +* https://github.com/v8/node/pull/213 + +This can be removed when Node.js upgrades to a version of V8 containing CLs +from the above issue. + +diff --git a/src/api/environment.cc b/src/api/environment.cc +index e72bee385865c7d34e9eea6b90c6d911d592f8af..d3d1040b7a1a6b9c4a1fa2399e9235ec3b0b2990 100644 +--- a/src/api/environment.cc ++++ b/src/api/environment.cc +@@ -315,6 +315,10 @@ Isolate* NewIsolate(Isolate::CreateParams* params, + MultiIsolatePlatform* platform, + const SnapshotData* snapshot_data, + const IsolateSettings& settings) { ++ if (params->cpp_heap == nullptr) { ++ params->cpp_heap = ++ v8::CppHeap::Create(platform, v8::CppHeapCreateParams{{}}).release(); ++ } + Isolate* isolate = Isolate::Allocate(); + if (isolate == nullptr) return nullptr; + +@@ -358,9 +362,12 @@ Isolate* NewIsolate(ArrayBufferAllocator* allocator, + uv_loop_t* event_loop, + MultiIsolatePlatform* platform, + const EmbedderSnapshotData* snapshot_data, +- const IsolateSettings& settings) { ++ const IsolateSettings& settings, ++ std::unique_ptr cpp_heap) { + Isolate::CreateParams params; + if (allocator != nullptr) params.array_buffer_allocator = allocator; ++ if (cpp_heap) ++ params.cpp_heap = cpp_heap.release(); + return NewIsolate(¶ms, + event_loop, + platform, +diff --git a/src/env.cc b/src/env.cc +index 1c1062a3996f2bb7de9e91f7f4385c8f8d20f490..b0156ee26c29ebe5b79c97834f3bfe8b56df50c6 100644 +--- a/src/env.cc ++++ b/src/env.cc +@@ -575,14 +575,6 @@ IsolateData::IsolateData(Isolate* isolate, + // 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 + // the ids in a static map to ensure pointers to them are still valid +@@ -605,14 +597,6 @@ IsolateData::IsolateData(Isolate* isolate, + } + } + +-IsolateData::~IsolateData() { +- if (cpp_heap_ != nullptr) { +- // The CppHeap must be detached before being terminated. +- isolate_->DetachCppHeap(); +- cpp_heap_->Terminate(); +- } +-} +- + // Deprecated API, embedders should use v8::Object::Wrap() directly instead. + void SetCppgcReference(Isolate* isolate, + Local object, +diff --git a/src/env.h b/src/env.h +index 1f8dc8f88d40ca95ba13d6517b2b5ed83184e1ce..ec3a813b3b864a0eda2e4aa0748b1447001fca9a 100644 +--- a/src/env.h ++++ b/src/env.h +@@ -155,7 +155,6 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer { + ArrayBufferAllocator* node_allocator = nullptr, + const EmbedderSnapshotData* embedder_snapshot_data = nullptr, + std::shared_ptr options = nullptr); +- ~IsolateData(); + + SET_MEMORY_INFO_NAME(IsolateData) + SET_SELF_SIZE(IsolateData) +@@ -258,7 +257,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 b2b10ffb572f010992f221de752618fd56b5d50e..0ed78ab6b52906e980eebf1f625a1c7cbfc8097b 100644 +--- a/src/node.cc ++++ b/src/node.cc +@@ -1222,10 +1222,6 @@ InitializeOncePerProcessInternal(const std::vector& args, + result->platform_ = per_process::v8_platform.Platform(); + } + +- if (!(flags & ProcessInitializationFlags::kNoInitializeV8)) { +- V8::Initialize(); +- } +- + if (!(flags & ProcessInitializationFlags::kNoInitializeCppgc)) { + v8::PageAllocator* allocator = nullptr; + if (result->platform_ != nullptr) { +@@ -1234,6 +1230,10 @@ InitializeOncePerProcessInternal(const std::vector& args, + cppgc::InitializeProcess(allocator); + } + ++ if (!(flags & ProcessInitializationFlags::kNoInitializeV8)) { ++ V8::Initialize(); ++ } ++ + #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 98ad0ea649eaef43d1f5231f7bc4044e100e08d7..c295cce8f5c7965cce4d2e4c0614dbe076986a4c 100644 +--- a/src/node.h ++++ b/src/node.h +@@ -589,7 +589,8 @@ NODE_EXTERN v8::Isolate* NewIsolate( + struct uv_loop_s* event_loop, + MultiIsolatePlatform* platform, + const EmbedderSnapshotData* snapshot_data = nullptr, +- const IsolateSettings& settings = {}); ++ const IsolateSettings& settings = {}, ++ std::unique_ptr cpp_heap = {}); + NODE_EXTERN v8::Isolate* NewIsolate( + std::shared_ptr allocator, + struct uv_loop_s* event_loop, +diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc +index 4119ac1b002681d39711eac810ca2fcc2702ffc7..790347056cde949ffe6cf8498a7eca0c4864c997 100644 +--- a/src/node_main_instance.cc ++++ b/src/node_main_instance.cc +@@ -44,6 +44,8 @@ NodeMainInstance::NodeMainInstance(const SnapshotData* snapshot_data, + isolate_params_(std::make_unique()), + snapshot_data_(snapshot_data) { + isolate_params_->array_buffer_allocator = array_buffer_allocator_.get(); ++ isolate_params_->cpp_heap = ++ v8::CppHeap::Create(platform_, v8::CppHeapCreateParams{{}}).release(); + + isolate_ = + NewIsolate(isolate_params_.get(), event_loop, platform, snapshot_data); +@@ -81,9 +83,9 @@ 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_); + } + isolate_->Dispose(); ++ platform_->UnregisterIsolate(isolate_); + } + + ExitCode NodeMainInstance::Run() { +diff --git a/src/node_worker.cc b/src/node_worker.cc +index 1fc3774948dae3c0aae7d2aef563e18ecd4243a3..a610ee24ff18bddc3849aec3a43c2037b9ab5d53 100644 +--- a/src/node_worker.cc ++++ b/src/node_worker.cc +@@ -230,13 +230,8 @@ class WorkerThreadData { + *static_cast(data) = true; + }, &platform_finished); + +- // The order of these calls is important; if the Isolate is first disposed +- // and then unregistered, there is a race condition window in which no +- // 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); + 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 3e9dfb4392fb3e3deaab5506771f01be65bc5dda..416e0479ddf740c6d3e2d4ea9466ac060b038294 100644 +--- a/src/util.cc ++++ b/src/util.cc +@@ -726,8 +726,8 @@ RAIIIsolateWithoutEntering::RAIIIsolateWithoutEntering(const SnapshotData* data) + } + + 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 3414c0be8ad777f0b9836323150071b688831a38..82013ffe7667d53248bd616efb79b294e4ae47dd 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 edd413ae9b956b2e59e8166785adef6a8ff06d51..d1c1549efcb0320bc0f7d354db2101acc0930005 100644 +--- a/test/cctest/test_cppgc.cc ++++ b/test/cctest/test_cppgc.cc +@@ -46,18 +46,15 @@ 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()); ++ v8::Isolate* isolate = ++ node::NewIsolate(allocator.get(), ¤t_loop, platform.get(), ++ nullptr, {}, std::move(cpp_heap)); + + // Try creating Context + IsolateData + Environment. + { +@@ -102,13 +99,11 @@ 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 14e82cc80ff73084fb43b2ef07febfd2667a0abc..b6a92f1685d1083c8f0c0b3ed110509f6d76b59f 100644 +--- a/test/cctest/test_environment.cc ++++ b/test/cctest/test_environment.cc +@@ -623,6 +623,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); +@@ -672,8 +675,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 c2d7893813000601502d050f21ad5c740c6a3b8f..3042f63201bd0df3ad316e09f0f54e210cea8831 100644 +--- a/test/cctest/test_platform.cc ++++ b/test/cctest/test_platform.cc +@@ -101,8 +101,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/fuzzers/fuzz_env.cc b/test/fuzzers/fuzz_env.cc +index bace3051f8cecd5050d4707f2431973752a22188..5ca295848a974c70ff1a9094eb288ef7e658d8e5 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 8f5e1a473e3148e0bcdcc3c2fd582685665ce461..936876cdae20d29618d3789a5ab46a1b3101a79d 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; + } + }; diff --git a/patches/v8/.patches b/patches/v8/.patches index 905bd3558cc0..280a34b93603 100644 --- a/patches/v8/.patches +++ b/patches/v8/.patches @@ -1,3 +1,2 @@ chore_allow_customizing_microtask_policy_per_context.patch deps_add_v8_object_setinternalfieldfornodecore.patch -revert_api_delete_deprecated_attachcppheap_and_detachcppheap.patch diff --git a/patches/v8/revert_api_delete_deprecated_attachcppheap_and_detachcppheap.patch b/patches/v8/revert_api_delete_deprecated_attachcppheap_and_detachcppheap.patch deleted file mode 100644 index fb0ccb886a35..000000000000 --- a/patches/v8/revert_api_delete_deprecated_attachcppheap_and_detachcppheap.patch +++ /dev/null @@ -1,137 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Charles Kerr -Date: Thu, 6 Mar 2025 14:31:19 -0600 -Subject: Revert "[api] Delete deprecated AttachCppHeap and DetachCppHeap" - -Restore this API because Node.js needs it. - -This patch can be removed after an upstream fix lands in Node.js, -e.g. in https://github.com/nodejs/node-v8/tree/canary - -diff --git a/include/v8-isolate.h b/include/v8-isolate.h -index 97f1030dd2ca47ca4b58ac64e2e11e615bc46130..24ef6b5e0af63179e557b9896134838e112c59db 100644 ---- a/include/v8-isolate.h -+++ b/include/v8-isolate.h -@@ -1172,6 +1172,28 @@ class V8_EXPORT Isolate { - */ - void SetEmbedderRootsHandler(EmbedderRootsHandler* handler); - -+ /** -+ * Attaches a managed C++ heap as an extension to the JavaScript heap. The -+ * embedder maintains ownership of the CppHeap. At most one C++ heap can be -+ * attached to V8. -+ * -+ * Multi-threaded use requires the use of v8::Locker/v8::Unlocker, see -+ * CppHeap. -+ * -+ * If a CppHeap is set via CreateParams, then this call is a noop. -+ */ -+ V8_DEPRECATED("Set the heap on Isolate creation using CreateParams instead.") -+ void AttachCppHeap(CppHeap*); -+ -+ /** -+ * Detaches a managed C++ heap if one was attached using `AttachCppHeap()`. -+ * -+ * If a CppHeap is set via CreateParams, then this call is a noop. -+ */ -+ V8_DEPRECATED( -+ "The CppHeap gets detached automatically during Isolate tear down.") -+ void DetachCppHeap(); -+ - using ReleaseCppHeapCallback = void (*)(std::unique_ptr); - - /** -@@ -1219,7 +1241,6 @@ class V8_EXPORT Isolate { - class V8_DEPRECATED("AtomicsWaitWakeHandle is unused and will be removed.") - #endif - V8_EXPORT AtomicsWaitWakeHandle { -- - public: - /** - * Stop this `Atomics.wait()` call and call the |AtomicsWaitCallback| -diff --git a/src/api/api.cc b/src/api/api.cc -index 64044e9cf44d401c249787feafb651688ee0d9f9..1677e54b188b6a1699370d8cff37d2acf2933f38 100644 ---- a/src/api/api.cc -+++ b/src/api/api.cc -@@ -9876,6 +9876,16 @@ void Isolate::SetEmbedderRootsHandler(EmbedderRootsHandler* handler) { - i_isolate->heap()->SetEmbedderRootsHandler(handler); - } - -+void Isolate::AttachCppHeap(CppHeap* cpp_heap) { -+ i::Isolate* i_isolate = reinterpret_cast(this); -+ i_isolate->heap()->AttachCppHeap(cpp_heap); -+} -+ -+void Isolate::DetachCppHeap() { -+ i::Isolate* i_isolate = reinterpret_cast(this); -+ i_isolate->heap()->DetachCppHeap(); -+} -+ - CppHeap* Isolate::GetCppHeap() const { - const i::Isolate* i_isolate = reinterpret_cast(this); - return i_isolate->heap()->cpp_heap(); -diff --git a/src/heap/cppgc-js/cpp-heap.cc b/src/heap/cppgc-js/cpp-heap.cc -index e033791ca1cdeba4a304e69b922d4169a22f9caa..706f81f7bbc1b5a7a1b73afe018b0b2c0184d9ef 100644 ---- a/src/heap/cppgc-js/cpp-heap.cc -+++ b/src/heap/cppgc-js/cpp-heap.cc -@@ -513,6 +513,11 @@ CppHeap::CppHeap( - } - - CppHeap::~CppHeap() { -+ if (isolate_) { -+ // TODO(ahaas): Delete this code once `v8::Isolate::DetachCppHeap` has been -+ // deleted. -+ isolate_->heap()->DetachCppHeap(); -+ } - Terminate(); - } - -diff --git a/src/heap/heap.cc b/src/heap/heap.cc -index da9d8810b307e94f01238e56532a0ff93f1ff325..252a1b354110764c6351119d41a4adddca0c2913 100644 ---- a/src/heap/heap.cc -+++ b/src/heap/heap.cc -@@ -6056,6 +6056,21 @@ void Heap::AttachCppHeap(v8::CppHeap* cpp_heap) { - cpp_heap_ = cpp_heap; - } - -+void Heap::DetachCppHeap() { -+ // The API function should be a noop in case a CppHeap was passed on Isolate -+ // creation. -+ if (owning_cpp_heap_) { -+ return; -+ } -+ -+ // The CppHeap may have been detached already. -+ if (!cpp_heap_) return; -+ -+ CppHeap::From(cpp_heap_)->StartDetachingIsolate(); -+ CppHeap::From(cpp_heap_)->DetachIsolate(); -+ cpp_heap_ = nullptr; -+} -+ - std::optional Heap::overridden_stack_state() const { - if (!embedder_stack_state_origin_) return {}; - return embedder_stack_state_; -diff --git a/src/heap/heap.h b/src/heap/heap.h -index 570cb682903cbead5f8f80573290b13ab1d81183..bccdb6c1bdb8fbbc6cc5aee0e54105f210ca2ab9 100644 ---- a/src/heap/heap.h -+++ b/src/heap/heap.h -@@ -1104,6 +1104,9 @@ class Heap final { - // Unified heap (C++) support. =============================================== - // =========================================================================== - -+ V8_EXPORT_PRIVATE void AttachCppHeap(v8::CppHeap* cpp_heap); -+ V8_EXPORT_PRIVATE void DetachCppHeap(); -+ - v8::CppHeap* cpp_heap() const { return cpp_heap_; } - - std::optional overridden_stack_state() const; -@@ -1645,8 +1648,6 @@ class Heap final { - private: - class AllocationTrackerForDebugging; - -- void AttachCppHeap(v8::CppHeap* cpp_heap); -- - using ExternalStringTableUpdaterCallback = - Tagged (*)(Heap* heap, FullObjectSlot pointer); -