src, test: unregister the isolate after disposal and before freeing

The order of these calls is important. When the Isolate is disposed,
it may still post tasks to the platform, so it must still be registered
for the task runner to be found from the map. After the isolate is torn
down, we need to remove it from the map before we can free the address,
so that when another Isolate::Allocate() is called, that would not be
allocated to the same address and be registered on an existing map
entry.

Author: joyeecheung

src/api/embed_helpers.cc

@@ -244,8 +244,7 @@ CommonEnvironmentSetup::~CommonEnvironmentSetup() {
     if (impl_->snapshot_creator.has_value()) {
       impl_->snapshot_creator.reset();
     }
-    isolate->Dispose();
-    impl_->platform->UnregisterIsolate(isolate);
+    impl_->platform->DisposeIsolate(isolate);
 
     // Wait until the platform has cleaned up all relevant resources.
     while (!platform_finished)

src/node.h

@@ -449,6 +449,9 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform {
   // pending delayed tasks scheduled for that isolate.
   // This needs to be called right after calling `Isolate::Dispose()`.
   virtual void UnregisterIsolate(v8::Isolate* isolate) = 0;
+  // This disposes, unregisters and frees up an isolate that's allocated using
+  // v8::Isolate::Allocate() in the correct order to prevent race conditions.
+  void DisposeIsolate(v8::Isolate* isolate);
 
   // The platform should call the passed function once all state associated
   // with the given isolate has been cleaned up. This can, but does not have to,

src/node_main_instance.cc

@@ -82,9 +82,7 @@ NodeMainInstance::~NodeMainInstance() {
     // IsolateData must be freed before UnregisterIsolate() is called.
     isolate_data_.reset();
   }
-  // TODO(joyeecheung): split Isolate::Free() here?
-  isolate_->Dispose();
-  platform_->UnregisterIsolate(isolate_);
+  platform_->DisposeIsolate(isolate_);
 }
 
 ExitCode NodeMainInstance::Run() {

src/node_platform.cc

@@ -669,4 +669,18 @@ std::queue<std::unique_ptr<T>> TaskQueue<T>::Locked::PopAll() {
   return result;
 }
 
+void MultiIsolatePlatform::DisposeIsolate(Isolate* isolate) {
+  // The order of these calls is important. When the Isolate is disposed,
+  // it may still post tasks to the platform, so it must still be registered
+  // for the task runner to be found from the map. After the isolate is torn
+  // down, we need to remove it from the map before we can free the address,
+  // so that when another Isolate::Allocate() is called, that would not be
+  // allocated to the same address and be registered on an existing map
+  // entry.
+  // Refs: https://github.com/nodejs/node/issues/30846
+  isolate->Deinitialize();
+  this->UnregisterIsolate(isolate);
+  Isolate::Free(isolate);
+}
+
 }  // namespace node

src/node_worker.cc

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

src/util.cc

@@ -729,8 +729,7 @@ RAIIIsolateWithoutEntering::RAIIIsolateWithoutEntering(const SnapshotData* data)
 }
 
 RAIIIsolateWithoutEntering::~RAIIIsolateWithoutEntering() {
-  isolate_->Dispose();
-  per_process::v8_platform.Platform()->UnregisterIsolate(isolate_);
+  per_process::v8_platform.Platform()->DisposeIsolate(isolate_);
 }
 
 RAIIIsolate::RAIIIsolate(const SnapshotData* data)

test/cctest/node_test_fixture.h

@@ -123,8 +123,7 @@ class NodeTestFixture : public NodeZeroIsolateTestFixture {
   void TearDown() override {
     platform->DrainTasks(isolate_);
     isolate_->Exit();
-    isolate_->Dispose();
-    platform->UnregisterIsolate(isolate_);
+    platform->DisposeIsolate(isolate_);
     isolate_ = nullptr;
     NodeZeroIsolateTestFixture::TearDown();
   }

test/cctest/test_cppgc.cc

@@ -96,8 +96,7 @@ TEST_F(NodeZeroIsolateTestFixture, ExistingCppHeapTest) {
     platform->DrainTasks(isolate);
   }
 
-  isolate->Dispose();
-  platform->UnregisterIsolate(isolate);
+  platform->DisposeIsolate(isolate);
 
   // Check that all the objects are created and destroyed properly.
   EXPECT_EQ(CppGCed::kConstructCount, 100);

test/cctest/test_environment.cc

@@ -544,8 +544,7 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) {
       node::FreeIsolateData(isolate_data);
     }
 
-    isolate->Dispose();
-    data->platform->UnregisterIsolate(isolate);
+    data->platform->DisposeIsolate(isolate);
     uv_run(&loop, UV_RUN_DEFAULT);
     CHECK_EQ(uv_loop_close(&loop), 0);
 
@@ -678,8 +677,7 @@ TEST_F(NodeZeroIsolateTestFixture, CtrlCWithOnlySafeTerminationTest) {
   }
 
   // Cleanup.
-  isolate->Dispose();
-  platform->UnregisterIsolate(isolate);
+  platform->DisposeIsolate(isolate);
 }
 #endif  // _WIN32
 

test/cctest/test_platform.cc

@@ -107,8 +107,7 @@ TEST_F(NodeZeroIsolateTestFixture, IsolatePlatformDelegateTest) {
 
   // Graceful shutdown
   delegate->Shutdown();
-  isolate->Dispose();
-  platform->UnregisterIsolate(isolate);
+  platform->DisposeIsolate(isolate);
 }
 
 TEST_F(PlatformTest, TracingControllerNullptr) {