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.

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>
This commit is contained in:
Joyee Cheung 2025-04-21 18:13:01 +02:00 committed by Michaël Zasso
parent 3ec86eb028
commit 5d3e1b555c
No known key found for this signature in database
GPG key ID: 770F7A9A5AE15600
12 changed files with 28 additions and 34 deletions

View file

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

View file

@ -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,

View file

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

View file

@ -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

View file

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

View file

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

View file

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

View file

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

View file

@ -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

View file

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

View file

@ -65,8 +65,7 @@ public:
void Teardown() {
platform->DrainTasks(isolate_);
isolate_->Exit();
isolate_->Dispose();
platform->UnregisterIsolate(isolate_);
platform->DisposeIsolate(isolate_);
isolate_ = nullptr;
}
};

View file

@ -72,8 +72,7 @@ public:
void Teardown() {
platform->DrainTasks(isolate_);
isolate_->Exit();
isolate_->Dispose();
platform->UnregisterIsolate(isolate_);
platform->DisposeIsolate(isolate_);
isolate_ = nullptr;
}
};