diff --git a/src/api/embed_helpers.cc b/src/api/embed_helpers.cc index 03800832cd6..d05b26c73fa 100644 --- a/src/api/embed_helpers.cc +++ b/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) diff --git a/src/node.h b/src/node.h index c2b6148c783..f773096c959 100644 --- a/src/node.h +++ b/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, diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 2d20b8d3a99..dd6ecd1f9d8 100644 --- a/src/node_main_instance.cc +++ b/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() { diff --git a/src/node_platform.cc b/src/node_platform.cc index 3aab5196a2b..50f5d4ef02a 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -669,4 +669,18 @@ std::queue> TaskQueue::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 diff --git a/src/node_worker.cc b/src/node_worker.cc index 4b22f1428cd..27be25796f7 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -230,19 +230,7 @@ class WorkerThreadData { *static_cast(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) { diff --git a/src/util.cc b/src/util.cc index 9a85953237e..1512f262fd9 100644 --- a/src/util.cc +++ b/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) diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index 82013ffe766..ad9047b928e 100644 --- a/test/cctest/node_test_fixture.h +++ b/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(); } diff --git a/test/cctest/test_cppgc.cc b/test/cctest/test_cppgc.cc index fef784e47b3..21f981f9ab2 100644 --- a/test/cctest/test_cppgc.cc +++ b/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); diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 7edf5d2e9c6..f57e638f800 100644 --- a/test/cctest/test_environment.cc +++ b/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 diff --git a/test/cctest/test_platform.cc b/test/cctest/test_platform.cc index d4a11d9404f..f1c1d52d92c 100644 --- a/test/cctest/test_platform.cc +++ b/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) { diff --git a/test/fuzzers/fuzz_env.cc b/test/fuzzers/fuzz_env.cc index 5ca295848a9..326d82ae20e 100644 --- a/test/fuzzers/fuzz_env.cc +++ b/test/fuzzers/fuzz_env.cc @@ -65,8 +65,7 @@ public: void Teardown() { platform->DrainTasks(isolate_); isolate_->Exit(); - isolate_->Dispose(); - platform->UnregisterIsolate(isolate_); + platform->DisposeIsolate(isolate_); isolate_ = nullptr; } }; diff --git a/test/fuzzers/fuzz_strings.cc b/test/fuzzers/fuzz_strings.cc index 936876cdae2..2226a471f3b 100644 --- a/test/fuzzers/fuzz_strings.cc +++ b/test/fuzzers/fuzz_strings.cc @@ -72,8 +72,7 @@ public: void Teardown() { platform->DrainTasks(isolate_); isolate_->Exit(); - isolate_->Dispose(); - platform->UnregisterIsolate(isolate_); + platform->DisposeIsolate(isolate_); isolate_ = nullptr; } };