src: track handle cleanup per thread, not per IsolateData

The FreeEnvironment() fix for sibling Environments keeps the depth of
nested Environment::CleanupHandles() calls on the IsolateData, so that
InternalCallbackScope can re-allow JavaScript for sibling Environments
while one of them is being freed. Environments that each have their own
IsolateData on the same isolate and loop never see that counter and
still fail with "illegal access". Environments that share a loop share a
thread, so keep the depth in a thread_local instead.

Refs: #65977
Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
PR-URL: #66239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Author: codebytere

src/api/callback.cc

@@ -96,8 +96,7 @@ InternalCallbackScope::InternalCallbackScope(
   }
 
   Isolate* isolate = env->isolate();
-  // See IsolateData::handle_cleanup_depth.
-  if (env->isolate_data()->handle_cleanup_depth > 0) allow_js_.emplace(isolate);
+  if (handle_cleanup_depth > 0) allow_js_.emplace(isolate);
 
   HandleScope handle_scope(isolate);
   Local<Context> current_context = isolate->GetCurrentContext();

src/env.cc

@@ -1467,6 +1467,8 @@ void Environment::ClosePerEnvHandles() {
   close_and_finish(reinterpret_cast<uv_handle_t*>(&task_queues_async_));
 }
 
+thread_local int handle_cleanup_depth = 0;
+
 void Environment::CleanupHandles() {
   {
     Mutex::ScopedLock lock(native_immediates_threadsafe_mutex_);
@@ -1484,8 +1486,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--; });
+  handle_cleanup_depth++;
+  auto done = OnScopeLeave([]() { handle_cleanup_depth--; });
   while (handle_cleanup_waiting_ != 0 ||
          request_waiting_ != 0 ||
          !handle_wrap_queue_.IsEmpty()) {

src/env.h

@@ -182,11 +182,6 @@ 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

@@ -283,6 +283,11 @@ class InternalCallbackScope {
   std::optional<v8::Isolate::AllowJavascriptExecutionScope> allow_js_;
 };
 
+// Non-zero while an Environment on this thread is closing its handles with JS
+// disallowed isolate-wide; InternalCallbackScope re-allows it for the other
+// Environments whose callbacks run in those loop turns.
+extern thread_local int handle_cleanup_depth;
+
 class DebugSealHandleScope {
  public:
   explicit inline DebugSealHandleScope(v8::Isolate* isolate = nullptr)