From ec575f4cbed0d1fefe1fbc4da86bc7eb69c22bdd Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Wed, 2 Sep 2026 21:57:09 +0000 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/33347 Signed-off-by: Shelley Vohr --- src/api/environment.cc | 3 +++ src/node_worker.cc | 5 ----- test/cctest/test_environment.cc | 21 +++++++++++++++++++++ 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 40aef133596e..a62b150cfa21 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -514,6 +514,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()); diff --git a/src/node_worker.cc b/src/node_worker.cc index 1a12946593ab..54427cef9032 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -326,11 +326,6 @@ void Worker::Run() { DeleteFnPtr 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); diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 449d7d38750b..32f347c5c754 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -355,6 +355,27 @@ TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) { EXPECT_TRUE(called_cb_2); } +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 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 context = v8::Context::New(isolate_);