src: fix Stop() terminating the next Environment on the isolate

After `Stop(env)`, freeing the Environment and creating another one on
the same isolate failed whenever no JavaScript ran in between: the new
Environment's first script was terminated before it started. That is
the normal case when `Stop()` is called from the process exit handler
for an uncaught exception, or by an embedder while the loop is idle.
`Stop()` calls `isolate->TerminateExecution()` unless
`kDoNotTerminateIsolate` is set, and V8 only clears that request the
next time JavaScript runs, so it outlived the Environment it was meant
for.

Cancel a pending termination when the Environment it was meant for is
freed. `Worker::Run()` already did this by hand before freeing its
Environment, with a TODO asking why V8 hit a DCHECK without it; this is
why, and that call now happens in `FreeEnvironment()`.

Refs: #33347
Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
PR-URL: #65819
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Author: codebytere

src/api/environment.cc

@@ -518,6 +518,9 @@ void FreeEnvironment(Environment* env) {
   Isolate* isolate = env->isolate();
   Isolate::DisallowJavascriptExecutionScope disallow_js(isolate,
       Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE);
+  // A termination requested by Stop() targets this Environment; if no JS ran
+  // since, it is still pending and must not hit the isolate's next user.
+  isolate->CancelTerminateExecution();
   {
     HandleScope handle_scope(isolate);  // For env->context().
     Context::Scope context_scope(env->context());

src/node_worker.cc

@@ -326,11 +326,6 @@ void Worker::Run() {
 
     DeleteFnPtr<Environment, FreeEnvironment> env_;
     auto cleanup_env = OnScopeLeave([&]() {
-      // TODO(addaleax): This call is harmless but should not be necessary.
-      // Figure out why V8 is raising a DCHECK() here without it
-      // (in test/parallel/test-async-hooks-worker-asyncfn-terminate-4.js).
-      isolate_->CancelTerminateExecution();
-
       if (!env_) return;
       env_->set_can_call_into_js(false);
 

test/cctest/test_environment.cc

@@ -367,6 +367,27 @@ TEST_F(EnvironmentTest, WorkerInEnvironmentWithoutSnapshot) {
   EXPECT_EQ(node::SpinEventLoop(*env).FromJust(), 0);
 }
 
+TEST_F(EnvironmentTest, StopFromExitHandlerDoesNotLeakIntoNextEnvironment) {
+  const v8::HandleScope handle_scope(isolate_);
+  const Argv argv;
+  {
+    Env env{handle_scope, argv};
+    node::SetProcessExitHandler(
+        *env, [](node::Environment* env_, int) { node::Stop(env_); });
+    // The uncaught exception runs the exit handler from C++ and does not
+    // re-enter JS afterwards, so nothing consumes the termination request.
+    EXPECT_TRUE(
+        node::LoadEnvironment(*env, "throw new Error('uncaught')").IsEmpty());
+    EXPECT_TRUE(node::SpinEventLoop(*env).IsNothing());
+  }
+  {
+    Env env{handle_scope, argv, node::EnvironmentFlags::kNoCreateInspector};
+    v8::Local<v8::Value> result =
+        node::LoadEnvironment(*env, "return 42;").ToLocalChecked();
+    EXPECT_EQ(result->Int32Value(env.context()).FromJust(), 42);
+  }
+}
+
 TEST_F(EnvironmentTest, NoEnvironmentSanity) {
   const v8::HandleScope handle_scope(isolate_);
   v8::Local<v8::Context> context = v8::Context::New(isolate_);