From d9d9e62474b41f5a937d1900d68ac4739810bee9 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Mon, 8 Jan 2024 22:39:30 +0800 Subject: [PATCH] src: avoid draining platform tasks at FreeEnvironment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the point of `FreeEnvironment` and onwards, no JavaScript execution associated with the Environment should be triggered. Avoid draining platform tasks that can trigger JavaScript execution in `FreeEnvironment`. The holder of `node::Environment` should immediately call `node::MultiIsolatePlatform::UnregisterIsolate` and `v8::Isolate::Dispose` to cancel pending foreground tasks and join concurrent tasks after the environment was freed. `NodePlatform` can properly handle the case in `RunForegroundTask` when an Isolate out-lives its associated `node::Environment`. PR-URL: https://github.com/nodejs/node/pull/51290 Fixes: https://github.com/nodejs/node/issues/47748 Fixes: https://github.com/nodejs/node/issues/49344 Reviewed-By: Vinícius Lourenço Claro Cardoso Reviewed-By: Matteo Collina --- src/api/environment.cc | 7 ------ src/node_main_instance.cc | 20 ++++++++++++++-- src/node_platform.cc | 5 ++++ .../test-finalization-registry-shutdown.js | 23 +++++++++++++++++++ 4 files changed, 46 insertions(+), 9 deletions(-) create mode 100644 test/parallel/test-finalization-registry-shutdown.js diff --git a/src/api/environment.cc b/src/api/environment.cc index 51cd46d0fb1c02..de58a26fde5bae 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -451,13 +451,6 @@ void FreeEnvironment(Environment* env) { RunAtExit(env); } - // This call needs to be made while the `Environment` is still alive - // because we assume that it is available for async tracking in the - // NodePlatform implementation. - MultiIsolatePlatform* platform = env->isolate_data()->platform(); - if (platform != nullptr) - platform->DrainTasks(isolate); - delete env; } diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 1d23631780cf90..0bc6180697e28c 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -103,8 +103,24 @@ NodeMainInstance::~NodeMainInstance() { if (isolate_params_ == nullptr) { return; } - // This should only be done on a main instance that owns its isolate. - platform_->UnregisterIsolate(isolate_); + + { +#ifdef DEBUG + // node::Environment has been disposed and no JavaScript Execution is + // allowed at this point. + // Create a scope to check that no JavaScript is executed in debug build + // and proactively crash the process in the case JavaScript is being + // executed. + // Isolate::Dispose() must be invoked outside of this scope to avoid + // use-after-free. + 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(); + platform_->UnregisterIsolate(isolate_); + } isolate_->Dispose(); } diff --git a/src/node_platform.cc b/src/node_platform.cc index 7dd0526e6ece5f..c16766f76a9187 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -424,6 +424,11 @@ void PerIsolatePlatformData::RunForegroundTask(std::unique_ptr task) { InternalCallbackScope::kNoFlags); task->Run(); } else { + // When the Environment was freed, the tasks of the Isolate should also be + // canceled by `NodePlatform::UnregisterIsolate`. However, if the embedder + // request to run the foreground task after the Environment was freed, run + // the task without InternalCallbackScope. + // The task is moved out of InternalCallbackScope if env is not available. // This is a required else block, and should not be removed. // See comment: https://github.com/nodejs/node/pull/34688#pullrequestreview-463867489 diff --git a/test/parallel/test-finalization-registry-shutdown.js b/test/parallel/test-finalization-registry-shutdown.js new file mode 100644 index 00000000000000..f896aa2f285c75 --- /dev/null +++ b/test/parallel/test-finalization-registry-shutdown.js @@ -0,0 +1,23 @@ +// Flags: --expose-gc +'use strict'; +const common = require('../common'); + +// This test verifies that when a V8 FinalizationRegistryCleanupTask is queue +// at the last moment when JavaScript can be executed, the callback of a +// FinalizationRegistry will not be invoked and the process should exit +// normally. + +const reg = new FinalizationRegistry( + common.mustNotCall('This FinalizationRegistry callback should never be called')); + +function register() { + // Create a temporary object in a new function scope to allow it to be GC-ed. + reg.register({}); +} + +process.on('exit', () => { + // This is the final chance to execute JavaScript. + register(); + // Queue a FinalizationRegistryCleanupTask by a testing gc request. + global.gc(); +});