Skip to content

Commit

Permalink
src: avoid draining platform tasks at FreeEnvironment
Browse files Browse the repository at this point in the history
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: #51290
Fixes: #47748
Fixes: #49344
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
legendecas authored and richardlau committed May 16, 2024
1 parent e5fc8ec commit d9d9e62
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 9 deletions.
7 changes: 0 additions & 7 deletions src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
20 changes: 18 additions & 2 deletions src/node_main_instance.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
5 changes: 5 additions & 0 deletions src/node_platform.cc
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,11 @@ void PerIsolatePlatformData::RunForegroundTask(std::unique_ptr<Task> 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
Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-finalization-registry-shutdown.js
Original file line number Diff line number Diff line change
@@ -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();
});

0 comments on commit d9d9e62

Please sign in to comment.