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 e00ea5951244..c420a51eb35a 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -411,6 +411,32 @@ TEST_F(EnvironmentTest, CollectExternalReferencesFromSeveralThreads) { EXPECT_EQ(node::SnapshotBuilder::CollectExternalReferences().back(), 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_);