From 5db35b4d7d73ab6484785f3f19fe709e33174c6b Mon Sep 17 00:00:00 2001
From: Chengzhong Wu <chengzhong.wcz@alibaba-inc.com>
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 <contact@viniciusl.com.br>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
---
 src/api/environment.cc                        |  7 ------
 src/node_main_instance.cc                     | 22 ++++++++++++++----
 src/node_platform.cc                          |  5 ++++
 .../test-finalization-registry-shutdown.js    | 23 +++++++++++++++++++
 4 files changed, 46 insertions(+), 11 deletions(-)
 create mode 100644 test/parallel/test-finalization-registry-shutdown.js

diff --git a/src/api/environment.cc b/src/api/environment.cc
index 7e30a35ad7e611..8d7ecad56175ff 100644
--- a/src/api/environment.cc
+++ b/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;
 }
 
diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc
index 64ab1375708c00..22b35e33e8fa50 100644
--- a/src/node_main_instance.cc
+++ b/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();
 }
 
diff --git a/src/node_platform.cc b/src/node_platform.cc
index 927fdddb8d9a1a..97cf6cb840bab5 100644
--- a/src/node_platform.cc
+++ b/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
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();
+});