src: use V8-owned CppHeap

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().

PR-URL: https://github.com/nodejs/node/pull/58070
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

src/api/embed_helpers.cc

@@ -234,14 +234,18 @@ CommonEnvironmentSetup::~CommonEnvironmentSetup() {
     }
 
     bool platform_finished = false;
-    impl_->platform->AddIsolateFinishedCallback(isolate, [](void* data) {
-      *static_cast<bool*>(data) = true;
-    }, &platform_finished);
-    impl_->platform->UnregisterIsolate(isolate);
+    impl_->platform->AddIsolateFinishedCallback(
+        isolate,
+        [](void* data) {
+          bool* ptr = static_cast<bool*>(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)

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);
 

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,

src/env.h

@@ -68,10 +68,6 @@
 #include <unordered_set>
 #include <vector>
 
-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<SnapshotConfig> snapshot_config_;
 
-  std::unique_ptr<v8::CppHeap> cpp_heap_;
   std::shared_ptr<PerIsolateOptions> options_;
   worker::Worker* worker_context_ = nullptr;
   PerIsolateWrapperData* wrapper_data_;

src/node.cc

@@ -1211,6 +1211,14 @@ InitializeOncePerProcessInternal(const std::vector<std::string>& 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<std::string>& 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;

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

src/node_contextify.cc

@@ -328,7 +328,7 @@ ContextifyContext* ContextifyContext::New(Local<Context> v8_context,
              .ToLocal(&wrapper)) {
       return {};
     }
-
+    DCHECK_NOT_NULL(env->isolate()->GetCppHeap());
     result = cppgc::MakeGarbageCollected<ContextifyContext>(
         env->isolate()->GetCppHeap()->GetAllocationHandle(),
         env,
@@ -975,6 +975,7 @@ void ContextifyScript::RegisterExternalReferences(
 
 ContextifyScript* ContextifyScript::New(Environment* env,
                                         Local<Object> object) {
+  DCHECK_NOT_NULL(env->isolate()->GetCppHeap());
   return cppgc::MakeGarbageCollected<ContextifyScript>(
       env->isolate()->GetCppHeap()->GetAllocationHandle(), env, object);
 }

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() {

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) {

src/util.cc

@@ -726,12 +726,15 @@ RAIIIsolateWithoutEntering::RAIIIsolateWithoutEntering(const SnapshotData* data)
     SnapshotBuilder::InitializeIsolateParams(data, &params);
   }
   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)

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();
   }

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(), &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());
+  node::IsolateSettings settings;
+  settings.cpp_heap =
+      v8::CppHeap::Create(platform.get(), v8::CppHeapCreateParams{{}})
+          .release();
+  v8::Isolate* isolate = node::NewIsolate(
+      allocator.get(), &current_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);

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, &current_loop);
@@ -675,8 +678,8 @@ TEST_F(NodeZeroIsolateTestFixture, CtrlCWithOnlySafeTerminationTest) {
   }
 
   // Cleanup.
-  platform->UnregisterIsolate(isolate);
   isolate->Dispose();
+  platform->UnregisterIsolate(isolate);
 }
 #endif  // _WIN32
 

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) {

test/embedding/embedtest.cc

@@ -2,6 +2,7 @@
 #undef NDEBUG
 #endif
 #include <assert.h>
+#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<MultiIsolatePlatform> platform =
       MultiIsolatePlatform::Create(4);
   V8::InitializePlatform(platform.get());
+  cppgc::InitializeProcess(platform->GetPageAllocator());
   V8::Initialize();
 
   int ret =

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;
   }
 };

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;
   }
 };