From 9fab2281155e67fdd80ec528eca7c21cb78b25b8 Mon Sep 17 00:00:00 2001
From: ywave620 <60539365+ywave620@users.noreply.github.com>
Date: Tue, 28 Feb 2023 11:17:46 +0800
Subject: [PATCH] src: fix cb scope bugs involved in termination

Be more aggresive to clean up the async id stack,
and ensure the cleanup when terminating.

Calling SetIdle() when terminating is not harmless.
When node terminates due to an unhandled exception,
v8 preseves the vm state, which is JS and notifies
node through PerIsolateMessageListener(). If node
calls SetIdle() later, v8 complains because it
requires the vm state to either be EXTERNEL or IDLE
when embedder calling SetIdle().

PR-URL: https://github.com/nodejs/node/pull/45596
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
---
 src/api/callback.cc                           | 10 ++++--
 src/api/environment.cc                        |  1 +
 ...t-unhandled-exception-with-worker-inuse.js | 32 +++++++++++++++++++
 .../test-worker-http2-stream-terminate.js     |  0
 4 files changed, 40 insertions(+), 3 deletions(-)
 create mode 100644 test/parallel/test-unhandled-exception-with-worker-inuse.js
 rename test/{sequential => parallel}/test-worker-http2-stream-terminate.js (100%)

diff --git a/src/api/callback.cc b/src/api/callback.cc
index 1287eb466fddd3..3a8bd9155a8545 100644
--- a/src/api/callback.cc
+++ b/src/api/callback.cc
@@ -97,10 +97,9 @@ void InternalCallbackScope::Close() {
   if (closed_) return;
   closed_ = true;
 
-  Isolate* isolate = env_->isolate();
-  auto idle = OnScopeLeave([&]() { isolate->SetIdle(true); });
+  // This function must ends up with either cleanup the
+  // async id stack or pop the topmost one from it
 
-  if (!env_->can_call_into_js()) return;
   auto perform_stopping_check = [&]() {
     if (env_->is_stopping()) {
       MarkAsFailed();
@@ -109,6 +108,11 @@ void InternalCallbackScope::Close() {
   };
   perform_stopping_check();
 
+  if (env_->is_stopping()) return;
+
+  Isolate* isolate = env_->isolate();
+  auto idle = OnScopeLeave([&]() { isolate->SetIdle(true); });
+
   if (!failed_ && async_context_.async_id != 0 && !skip_hooks_) {
     AsyncWrap::EmitAfter(env_, async_context_.async_id);
   }
diff --git a/src/api/environment.cc b/src/api/environment.cc
index 389c6666dc6966..19c50398fa3da5 100644
--- a/src/api/environment.cc
+++ b/src/api/environment.cc
@@ -881,6 +881,7 @@ ThreadId AllocateEnvironmentThreadId() {
 }
 
 void DefaultProcessExitHandlerInternal(Environment* env, ExitCode exit_code) {
+  env->set_stopping(true);
   env->set_can_call_into_js(false);
   env->stop_sub_worker_contexts();
   env->isolate()->DumpAndResetStats();
diff --git a/test/parallel/test-unhandled-exception-with-worker-inuse.js b/test/parallel/test-unhandled-exception-with-worker-inuse.js
new file mode 100644
index 00000000000000..a3e823ca70bf0f
--- /dev/null
+++ b/test/parallel/test-unhandled-exception-with-worker-inuse.js
@@ -0,0 +1,32 @@
+'use strict';
+const common = require('../common');
+
+// https://github.com/nodejs/node/issues/45421
+//
+// Check that node will NOT call v8::Isolate::SetIdle() when exiting
+// due to an unhandled exception, otherwise the assertion(enabled in
+// debug build only) in the SetIdle(), which checks that the vm state
+// is either EXTERNAL or IDLE will fail.
+//
+// The root cause of this issue is that before PerIsolateMessageListener()
+// is invoked by v8, v8 preserves the JS vm state, although it should
+// switch to EXTERNEL. https://bugs.chromium.org/p/v8/issues/detail?id=13464
+//
+// Therefore, this commit can be considered as an workaround of the v8 bug,
+// but we also find it not useful to call SetIdle() when terminating.
+
+if (process.argv[2] === 'child') {
+  const { Worker } = require('worker_threads');
+  new Worker('', { eval: true });
+  throw new Error('xxx');
+} else {
+  const assert = require('assert');
+  const { spawnSync } = require('child_process');
+  const result = spawnSync(process.execPath, [__filename, 'child']);
+
+  const stderr = result.stderr.toString().trim();
+  // Expect error message to be preserved
+  assert.match(stderr, /xxx/);
+  // Expect no crash
+  assert(!common.nodeProcessAborted(result.status, result.signal), stderr);
+}
diff --git a/test/sequential/test-worker-http2-stream-terminate.js b/test/parallel/test-worker-http2-stream-terminate.js
similarity index 100%
rename from test/sequential/test-worker-http2-stream-terminate.js
rename to test/parallel/test-worker-http2-stream-terminate.js