src: dispose isolate before unregistering it

Author: targos

src/api/embed_helpers.cc

@@ -219,11 +219,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/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

src/node_main_instance.cc

@@ -82,12 +82,10 @@ NodeMainInstance::~NodeMainInstance() {
     Isolate::DisallowJavascriptExecutionScope disallow_js(
         isolate_, Isolate::DisallowJavascriptExecutionScope::CRASH_ON_FAILURE);
 #endif
-    // 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_);
+    isolate_data_.reset();
   }
-  isolate_->Dispose();
 }
 
 ExitCode NodeMainInstance::Run() {

src/node_worker.cc

@@ -234,13 +234,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) {

src/util.cc

@@ -730,8 +730,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/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

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

test/cctest/test_platform.cc

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

test/fuzzers/fuzz_env.cc

@@ -65,8 +65,8 @@ class FuzzerFixtureHelper {
   void Teardown() {
     platform->DrainTasks(isolate_);
     isolate_->Exit();
-    platform->UnregisterIsolate(isolate_);
     isolate_->Dispose();
+    platform->UnregisterIsolate(isolate_);
     isolate_ = nullptr;
   }
 };