src: avoid draining platform tasks at FreeEnvironment

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: #51290Fixes: #47748Fixes: #49344 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Author: legendecas

src/api/environment.cc

@@ -512,13 +512,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;
 }
 

src/node_main_instance.cc

@@ -65,10 +65,24 @@ NodeMainInstance::~NodeMainInstance(){
 if (isolate_params_ == nullptr){
 return;
  }
-// 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_);
+
+{
+#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();
 }
 

src/node_platform.cc

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

test/parallel/test-finalization-registry-shutdown.js

@@ -0,0 +1,23 @@
+// Flags: --expose-gc
+'use strict';
+constcommon=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.
+
+constreg=newFinalizationRegistry(
+common.mustNotCall('This FinalizationRegistry callback should never be called'));
+
+functionregister(){
+// 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();
+});