mirrors/electron
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages 1 Wiki Activity Actions 1

refactor: remove usage of V8's {Attach|Detach}CppHeap() (#45922)

Browse source 
* refactor: remove usage of V8's {Attach|Detach}CppHeap()

* chore: remove revert patch
This commit is contained in:
Shelley Vohr 2025-03-11 18:54:33 +01:00 • committed by GitHub
parent 22262c14f1
commit cd56b96544
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 321 additions and 138 deletions
Download patch file Download diff file Expand all files Collapse all files

1
patches/node/.patches
View file

@ -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

320
patches/node/refactor_attach_cppgc_heap_on_v8_isolate_creation.patch Normal file
View file

@ -0,0 +1,320 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Shelley Vohr <shelley.vohr@gmail.com>
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<v8::CppHeap> cpp_heap) {
Isolate::CreateParams params;
if (allocator != nullptr) params.array_buffer_allocator = allocator;
+ if (cpp_heap)
+ params.cpp_heap = cpp_heap.release();
return NewIsolate(&params,
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> 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<PerIsolateOptions> 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<SnapshotConfig> snapshot_config_;
- std::unique_ptr<v8::CppHeap> cpp_heap_;
std::shared_ptr<PerIsolateOptions> 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<std::string>& 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<std::string>& 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<v8::CppHeap> cpp_heap = {});
NODE_EXTERN v8::Isolate* NewIsolate(
std::shared_ptr<ArrayBufferAllocator> 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<Isolate::CreateParams>()),
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<bool*>(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(), &current_loop, platform.get());
// Create and attach the CppHeap before we set up the IsolateData so that
// it recognizes the existing heap.
std::unique_ptr<v8::CppHeap> 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(), &current_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, &current_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;
}
};

1
patches/v8/.patches
View file

@ -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

137
patches/v8/revert_api_delete_deprecated_attachcppheap_and_detachcppheap.patch
View file

@ -1,137 +0,0 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Charles Kerr <charles@charleskerr.com>
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<CppHeap>);
/**
@@ -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<i::Isolate*>(this);
+ i_isolate->heap()->AttachCppHeap(cpp_heap);
+}
+
+void Isolate::DetachCppHeap() {
+ i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(this);
+ i_isolate->heap()->DetachCppHeap();
+}
+
CppHeap* Isolate::GetCppHeap() const {
const i::Isolate* i_isolate = reinterpret_cast<const i::Isolate*>(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<StackState> 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<StackState> overridden_stack_state() const;
@@ -1645,8 +1648,6 @@ class Heap final {
private:
class AllocationTrackerForDebugging;
- void AttachCppHeap(v8::CppHeap* cpp_heap);
-
using ExternalStringTableUpdaterCallback =
Tagged<String> (*)(Heap* heap, FullObjectSlot pointer);