src: fix FreeEnvironment() breaking JS in sibling Environments

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<Context> current_context = isolate->GetCurrentContext();

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()) {

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)

src/node_internals.h

@@ -37,6 +37,7 @@
 #include <cstdint>
 #include <cstdlib>
 
+#include <optional>
 #include <string>
 #include <variant>
 #include <vector>
@@ -279,6 +280,7 @@ class InternalCallbackScope {
   bool pushed_ids_ = false;
   bool closed_ = false;
   v8::Global<v8::Value> prior_context_frame_;
+  std::optional<v8::Isolate::AllowJavascriptExecutionScope> allow_js_;
 };
 
 class DebugSealHandleScope {

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<v8::Value> 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<v8::Context> context = v8::Context::New(isolate_);