From cd2561772e188c2787be9730d01c3c7a44e608b7 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Thu, 3 Sep 2026 00:21:29 +0000 Subject: [PATCH] src: fix FreeEnvironment() breaking JS in sibling Environments `FreeEnvironment()` forbids JavaScript on the whole isolate with a `DisallowJavascriptExecutionScope` and then spins the event loop in `Environment::CleanupHandles()` until its handles are closed. When the loop and isolate are shared with other Environments, as they are for an embedder hosting several Environments on one thread, those loop turns also run the other Environments' timers and I/O callbacks, and every one of them failed with an "illegal access" exception thrown into unrelated code. While an Environment is closing its handles, let `InternalCallbackScope` re-allow JavaScript for callbacks of Environments that can still call into JS. The Environment being freed stays blocked: its callbacks fail the `can_call_into_js()` check before that point, and anything that bypasses `InternalCallbackScope` (V8 foreground tasks, raw addon callbacks) still runs under the disallow scope. Refs: https://github.com/nodejs/node/pull/33874 Refs: https://github.com/nodejs/node/pull/65819 Signed-off-by: Shelley Vohr --- src/api/callback.cc | 2 ++ src/env.cc | 2 ++ src/env.h | 5 +++++ src/node_internals.h | 2 ++ test/cctest/test_environment.cc | 26 ++++++++++++++++++++++++++ 5 files changed, 37 insertions(+) diff --git a/src/api/callback.cc b/src/api/callback.cc index 85da82ff84a5..c3850fa4afef 100644 --- a/src/api/callback.cc +++ b/src/api/callback.cc @@ -96,6 +96,8 @@ InternalCallbackScope::InternalCallbackScope( } Isolate* isolate = env->isolate(); + // See IsolateData::handle_cleanup_depth. + if (env->isolate_data()->handle_cleanup_depth > 0) allow_js_.emplace(isolate); HandleScope handle_scope(isolate); Local current_context = isolate->GetCurrentContext(); diff --git a/src/env.cc b/src/env.cc index d35c28fcf7ed..24cd7b62e4f0 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1453,6 +1453,8 @@ void Environment::CleanupHandles() { for (HandleWrap* handle : handle_wrap_queue_) handle->Close(); + isolate_data()->handle_cleanup_depth++; + auto done = OnScopeLeave([&]() { isolate_data()->handle_cleanup_depth--; }); while (handle_cleanup_waiting_ != 0 || request_waiting_ != 0 || !handle_wrap_queue_.IsEmpty()) { diff --git a/src/env.h b/src/env.h index 29754ddb6371..b21bf61d5302 100644 --- a/src/env.h +++ b/src/env.h @@ -182,6 +182,11 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer { inline worker::Worker* worker_context() const; inline void set_worker_context(worker::Worker* context); + // Non-zero while an Environment on this isolate is closing its handles with + // JS disallowed isolate-wide; InternalCallbackScope re-allows it for the + // other Environments whose callbacks run in those loop turns. + int handle_cleanup_depth = 0; + #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) #define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName) #define VS(PropertyName, StringValue) V(v8::String, PropertyName) diff --git a/src/node_internals.h b/src/node_internals.h index cf3ce9e13f4d..1eacd95e3a42 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -37,6 +37,7 @@ #include #include +#include #include #include #include @@ -279,6 +280,7 @@ class InternalCallbackScope { bool pushed_ids_ = false; bool closed_ = false; v8::Global prior_context_frame_; + std::optional allow_js_; }; class DebugSealHandleScope { diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 96259ec50227..c826a1b56d2b 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -432,6 +432,32 @@ TEST_F(EnvironmentTest, SharedIsolateDataLoadsBindingsTwice) { EXPECT_EQ(node::SpinEventLoop(*env2).FromJust(), 0); } +TEST_F(EnvironmentTest, FreeEnvironmentWhileSiblingHasActiveHandles) { + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + Env env1{handle_scope, argv}; + node::LoadEnvironment(*env1, + "globalThis.ticks = 0;" + "const t = setInterval(() => {" + " if (++globalThis.ticks == 20) clearInterval(t);" + "}, 1);") + .ToLocalChecked(); + { + Env env2{handle_scope, argv, node::EnvironmentFlags::kNoCreateInspector}; + node::LoadEnvironment(*env2, "setInterval(() => {}, 1);").ToLocalChecked(); + uv_sleep(5); + } + v8::Context::Scope context_scope(env1.context()); + EXPECT_EQ(node::SpinEventLoop(*env1).FromJust(), 0); + v8::Local ticks = + env1.context() + ->Global() + ->Get(env1.context(), + v8::String::NewFromUtf8Literal(isolate_, "ticks")) + .ToLocalChecked(); + EXPECT_EQ(ticks->Int32Value(env1.context()).FromJust(), 20); +} + TEST_F(EnvironmentTest, NoEnvironmentSanity) { const v8::HandleScope handle_scope(isolate_); v8::Local context = v8::Context::New(isolate_);