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

Author: joyeecheung

src/api/embed_helpers.cc

@@ -232,11 +232,11 @@ CommonEnvironmentSetup::~CommonEnvironmentSetup() {
     impl_->platform->AddIsolateFinishedCallback(isolate, [](void* data) {
       *static_cast<bool*>(data) = true;
     }, &platform_finished);
-    impl_->platform->UnregisterIsolate(isolate);
     if (impl_->snapshot_creator.has_value())
       impl_->snapshot_creator.reset();
     else
       isolate->Dispose();
+    impl_->platform->UnregisterIsolate(isolate);
 
     // Wait until the platform has cleaned up all relevant resources.
     while (!platform_finished)

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

src/env.cc

@@ -41,8 +41,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;
@@ -546,19 +544,11 @@ 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;
   // 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
@@ -582,13 +572,7 @@ 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() {}
 
 // Deprecated API, embedders should use v8::Object::Wrap() directly instead.
 void SetCppgcReference(Isolate* isolate,

src/env.h

@@ -67,10 +67,6 @@
 #include <unordered_set>
 #include <vector>
 
-namespace v8 {
-class CppHeap;
-}
-
 namespace node {
 
 namespace shadow_realm {
@@ -249,7 +245,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.h

@@ -450,7 +450,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
@@ -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,10 +1563,9 @@ void RegisterSignalHandler(int signal,
                            bool reset_handler = false);
 #endif  // _WIN32
 
-// 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.
+// 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<v8::Object> 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();
+    isolate_->Dispose();
     platform_->UnregisterIsolate(isolate_);
+    // TODO(joyeecheung): split Isolate::Free() here?
   }
-  isolate_->Dispose();
 }
 
 ExitCode NodeMainInstance::Run() {

src/node_worker.cc

@@ -234,8 +234,8 @@ 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);
       isolate->Dispose();
+      w_->platform_->UnregisterIsolate(isolate);
 
       // Wait until the platform has cleaned up all relevant resources.
       while (!platform_finished) {

src/util.cc

@@ -724,8 +724,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)

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,11 +94,6 @@ TEST_F(NodeZeroIsolateTestFixture, ExistingCppHeapTest) {
     }
 
     platform->DrainTasks(isolate);
-
-    // Cleanup.
-    isolate->DetachCppHeap();
-    cpp_heap->Terminate();
-    platform->DrainTasks(isolate);
   }
 
   platform->UnregisterIsolate(isolate);