inspector: defer JS execution from V8 interrupts

Inspector messages for a main thread connection are dispatched
from a V8 interrupt handler. Defer frontend messages in this
case using a microtask. This preserves the existing timing
contract for main thread inspector connections, where
notifications are observable after a microtask checkpoint.

Starting the inspector I/O thread and toggling network tracking
can also call into JS from an interrupt.

Signed-off-by: deepak1556 <hop2deep@gmail.com>
Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>

Author: deepak1556

src/inspector_agent.cc

@@ -80,6 +80,52 @@ static Mutex start_io_thread_agents_mutex;
 static std::vector<Agent*> start_io_thread_agents;
 static bool debug_signal_handler_started = false;
 
+template <typename Defer, typename OnFailure>
+void SyncJavaScriptHookState(Environment* env,
+                             JavaScriptHookState* state,
+                             Local<Function> enable_function,
+                             Local<Function> disable_function,
+                             bool require_bootstrapping,
+                             Defer defer,
+                             OnFailure on_failure) {
+  // The debugger can request an interrupt from within a hook's JS toggle
+  // function. A nested sync only records the new requested state; the
+  // outermost sync sees it when re-checking the loop after each toggle.
+  if (state->syncing) return;
+  state->syncing = true;
+  auto on_exit = OnScopeLeave([state]() { state->syncing = false; });
+
+  Isolate* isolate = env->isolate();
+  while (state->wanted != state->enabled) {
+    // No hook events will be emitted during cleanup, and calling into JS is no
+    // longer possible.
+    // Refs: https://github.com/nodejs/node/pull/34362#discussion_r456006039
+    if (!env->can_call_into_js()) return;
+
+    bool enable = state->wanted;
+    Local<Function> function = enable ? enable_function : disable_function;
+    if (function.IsEmpty()) return;
+
+    if (env->is_processing_v8_interrupt()) {
+      defer();
+      return;
+    }
+
+    if (require_bootstrapping) CHECK(env->has_run_bootstrapping_code());
+    Local<Context> context = env->context();
+    v8::TryCatch try_catch(isolate);
+    USE(function->Call(context, Undefined(isolate), 0, nullptr));
+    if (try_catch.HasCaught()) {
+      // Termination may abort the toggle invocation. Do not record a state that
+      // may not have taken effect, so a later sync can retry it.
+      if (try_catch.HasTerminated()) return;
+      PrintCaughtException(isolate, context, try_catch);
+      on_failure();
+    }
+    state->enabled = enable;
+  }
+}
+
 static void RequestIoThreadStartOnAgents() {
   Mutex::ScopedLock lock(start_io_thread_agents_mutex);
   for (Agent* agent : start_io_thread_agents) agent->RequestIoThreadStart();
@@ -483,6 +529,16 @@ class SameThreadInspectorSession : public InspectorSession {
 
 void NotifyClusterWorkersDebugEnabled(Environment* env) {
   Isolate* isolate = env->isolate();
+
+  // Starting the inspector from an interrupt is safe, but emitting the cluster
+  // notification calls into JS.
+  if (env->is_processing_v8_interrupt()) {
+    env->SetImmediate(
+        [](Environment* env) { NotifyClusterWorkersDebugEnabled(env); },
+        CallbackFlags::kUnrefed);
+    return;
+  }
+
   HandleScope handle_scope(isolate);
 
   // Send message to enable debug in cluster workers
@@ -1000,54 +1056,40 @@ void Agent::SetupNetworkTracking(Local<Function> enable_function,
                                  Local<Function> disable_function) {
   parent_env_->set_inspector_enable_network_tracking(enable_function);
   parent_env_->set_inspector_disable_network_tracking(disable_function);
-  if (pending_enable_network_tracking) {
-    pending_enable_network_tracking = false;
-    EnableNetworkTracking();
-  } else if (pending_disable_network_tracking) {
-    pending_disable_network_tracking = false;
-    DisableNetworkTracking();
-  }
+  SyncNetworkTrackingState();
 }
 
 void Agent::EnableNetworkTracking() {
-  if (network_tracking_enabled_) {
-    return;
-  }
-  HandleScope scope(parent_env_->isolate());
-  Local<Function> enable = parent_env_->inspector_enable_network_tracking();
-  if (enable.IsEmpty()) {
-    pending_enable_network_tracking = true;
-  } else {
-    ToggleNetworkTracking(parent_env_->isolate(), enable);
-    network_tracking_enabled_ = true;
-  }
+  network_tracking_state_.wanted = true;
+  SyncNetworkTrackingState();
 }
 
 void Agent::DisableNetworkTracking() {
-  if (!network_tracking_enabled_) {
-    return;
-  }
-  HandleScope scope(parent_env_->isolate());
-  Local<Function> disable = parent_env_->inspector_disable_network_tracking();
-  if (disable.IsEmpty()) {
-    pending_disable_network_tracking = true;
-  } else if (!client_->hasConnectedSessions()) {
-    ToggleNetworkTracking(parent_env_->isolate(), disable);
-    network_tracking_enabled_ = false;
-  }
+  if (client_->hasConnectedSessions()) return;
+  network_tracking_state_.wanted = false;
+  SyncNetworkTrackingState();
 }
 
-void Agent::ToggleNetworkTracking(Isolate* isolate, Local<Function> fn) {
-  if (!parent_env_->can_call_into_js()) return;
-  auto context = parent_env_->context();
+void Agent::SyncNetworkTrackingState() {
+  Isolate* isolate = parent_env_->isolate();
   HandleScope scope(isolate);
-  CHECK(!fn.IsEmpty());
-  v8::TryCatch try_catch(isolate);
-  USE(fn->Call(context, Undefined(isolate), 0, nullptr));
-  if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
-    PrintCaughtException(isolate, context, try_catch);
-    UNREACHABLE("Cannot toggle network tracking, please report this.");
-  }
+  SyncJavaScriptHookState(
+      parent_env_,
+      &network_tracking_state_,
+      parent_env_->inspector_enable_network_tracking(),
+      parent_env_->inspector_disable_network_tracking(),
+      false,
+      [this]() {
+        parent_env_->SetImmediate(
+            [](Environment* env) {
+              Agent* agent = env->inspector_agent();
+              if (agent != nullptr) agent->SyncNetworkTrackingState();
+            },
+            CallbackFlags::kUnrefed);
+      },
+      []() {
+        UNREACHABLE("Cannot toggle network tracking, please report this.");
+      });
 }
 
 void Agent::WaitForDisconnect() {
@@ -1098,7 +1140,7 @@ void Agent::RegisterAsyncHook(Isolate* isolate,
 }
 
 void Agent::SetAsyncHookTrackingEnabled(bool enabled) {
-  async_hook_wanted_ = enabled;
+  async_hook_state_.wanted = enabled;
   SyncAsyncHookState();
 }
 
@@ -1112,52 +1154,25 @@ void Agent::SetAsyncHookTrackingEnabled(bool enabled) {
 // When it's not safe to call into JS, this is a no-op and we'll try again in
 // RegisterAsyncHook() (for 1) or from a scheduled immediate (for 2).
 void Agent::SyncAsyncHookState() {
-  // The debugger can request an interrupt within the toggle JS function itself,
-  // A nested call only records the new requested state, the outermost call sees
-  // it when re-checking the loop condition after each toggle.
-  if (syncing_async_hook_state_) return;
-  syncing_async_hook_state_ = true;
-  auto on_exit = OnScopeLeave([this]() { syncing_async_hook_state_ = false; });
-
   Isolate* isolate = parent_env_->isolate();
   HandleScope scope(isolate);
-  while (async_hook_wanted_ != async_hook_enabled_) {
-    // Guard against running this during cleanup -- no async events will be
-    // emitted anyway at that point anymore, and calling into JS is not
-    // possible. This should probably not be something we're attempting in the
-    // first place,
-    // Refs: https://github.com/nodejs/node/pull/34362#discussion_r456006039
-    if (!parent_env_->can_call_into_js()) return;
-
-    bool enable = async_hook_wanted_;
-    Local<Function> fn = enable ? parent_env_->inspector_enable_async_hooks()
-                                : parent_env_->inspector_disable_async_hooks();
-    if (fn.IsEmpty()) return;
-
-    if (parent_env_->is_processing_v8_interrupt()) {
-      parent_env_->SetImmediate(
-          [](Environment* env) {
-            Agent* agent = env->inspector_agent();
-            if (agent != nullptr) agent->SyncAsyncHookState();
-          },
-          CallbackFlags::kUnrefed);
-      return;
-    }
-
-    CHECK(parent_env_->has_run_bootstrapping_code());
-    Local<Context> context = parent_env_->context();
-    v8::TryCatch try_catch(isolate);
-    USE(fn->Call(context, Undefined(isolate), 0, nullptr));
-    if (try_catch.HasCaught()) {
-      // Termination may abort the toggle invocation, retrying now would just
-      // be terminated again. Instead of recording the toggle that may not have
-      // taken effect, leave the states as-is so that a later sync retries.
-      if (try_catch.HasTerminated()) return;
-      PrintCaughtException(isolate, context, try_catch);
-      UNREACHABLE("Cannot toggle Inspector's AsyncHook, please report this.");
-    }
-    async_hook_enabled_ = enable;
-  }
+  SyncJavaScriptHookState(
+      parent_env_,
+      &async_hook_state_,
+      parent_env_->inspector_enable_async_hooks(),
+      parent_env_->inspector_disable_async_hooks(),
+      true,
+      [this]() {
+        parent_env_->SetImmediate(
+            [](Environment* env) {
+              Agent* agent = env->inspector_agent();
+              if (agent != nullptr) agent->SyncAsyncHookState();
+            },
+            CallbackFlags::kUnrefed);
+      },
+      []() {
+        UNREACHABLE("Cannot toggle Inspector's AsyncHook, please report this.");
+      });
 }
 
 void Agent::AsyncTaskScheduled(const StringView& task_name, void* task,

src/inspector_agent.h

@@ -29,6 +29,12 @@ class ParentInspectorHandle;
 class NodeInspectorClient;
 class WorkerManager;
 
+struct JavaScriptHookState {
+  bool wanted = false;
+  bool enabled = false;
+  bool syncing = false;
+};
+
 class InspectorSession {
  public:
   virtual ~InspectorSession() = default;
@@ -134,7 +140,7 @@ class Agent {
 
  private:
   void SyncAsyncHookState();
-  void ToggleNetworkTracking(v8::Isolate* isolate, v8::Local<v8::Function> fn);
+  void SyncNetworkTrackingState();
 
   node::Environment* parent_env_;
   // Encapsulates majority of the Inspector functionality
@@ -154,19 +160,17 @@ class Agent {
   // The state of the async hook used for async stack traces that the protocol
   // last requested, and the state JS currently has. SyncAsyncHookState()
   // reconciles the two when it is possible and safe to call into JS.
-  bool async_hook_wanted_ = false;
-  bool async_hook_enabled_ = false;
-  bool syncing_async_hook_state_ = false;
+  JavaScriptHookState async_hook_state_;
 
   // Woken by the SIGUSR1 watchdog; closed by the cleanup hook or ~Agent(),
   // whichever runs first, and freed by its close callback.
   uv_async_t* start_io_thread_async_ = nullptr;
   void StopAcceptingIoThreadStarts();
   static void StopAcceptingIoThreadStartsHook(void* agent);
 
-  bool network_tracking_enabled_ = false;
-  bool pending_enable_network_tracking = false;
-  bool pending_disable_network_tracking = false;
+  // Network tracking uses JS hooks. Reconcile the protocol requested and
+  // applied states after leaving a V8 interrupt.
+  JavaScriptHookState network_tracking_state_;
   std::shared_ptr<NetworkResourceManager> network_resource_manager_;
 };
 

src/inspector_js_api.cc

@@ -62,21 +62,38 @@ struct MainThreadConnection {
 template <typename ConnectionType>
 class JSBindingsConnection : public BaseObject {
  public:
+  struct PendingMicrotask {
+    explicit PendingMicrotask(JSBindingsConnection* connection)
+        : env(connection->env()), connection(connection) {
+      env->AddCleanupHook(Delete, this);
+    }
+
+    static void Delete(void* data) {
+      delete static_cast<PendingMicrotask*>(data);
+    }
+
+    static void Run(void* data) {
+      std::unique_ptr<PendingMicrotask> pending(
+          static_cast<PendingMicrotask*>(data));
+      pending->env->RemoveCleanupHook(Delete, pending.get());
+      pending->connection->FlushPendingMessages();
+    }
+
+    Environment* env;
+    BaseObjectPtr<JSBindingsConnection> connection;
+  };
+
   class JSBindingsSessionDelegate : public InspectorSessionDelegate {
    public:
-    JSBindingsSessionDelegate(Environment* env,
-                              JSBindingsConnection* connection)
-                              : env_(env),
-                                connection_(connection) {
-    }
+    explicit JSBindingsSessionDelegate(JSBindingsConnection* connection)
+        : connection_(connection) {}
 
     void SendMessageToFrontend(const v8_inspector::StringView& message)
         override {
       connection_->SendMessageToFrontend(message);
     }
 
    private:
-    Environment* env_;
     BaseObjectPtr<JSBindingsConnection> connection_;
   };
 
@@ -86,10 +103,10 @@ class JSBindingsConnection : public BaseObject {
       : BaseObject(env, wrap), callback_(env->isolate(), callback) {
     Agent* inspector = env->inspector_agent();
     session_ = ConnectionType::Connect(
-        inspector, std::make_unique<JSBindingsSessionDelegate>(env, this));
-    // Inspector responses may be produced from a GC weak callback, where
-    // invoking the JavaScript session callback is forbidden. Defer delivery
-    // until the GC has completed.
+        inspector, std::make_unique<JSBindingsSessionDelegate>(this));
+    // Inspector responses may be produced from a GC weak callback or a V8
+    // interrupt, where invoking the JavaScript session callback is forbidden.
+    // Defer delivery until it is safe to call into JavaScript.
     env->isolate()->AddGCPrologueCallback(GCPrologueCallback, this);
     env->isolate()->AddGCEpilogueCallback(GCEpilogueCallback, this);
   }
@@ -100,7 +117,8 @@ class JSBindingsConnection : public BaseObject {
   }
 
   void SendMessageToFrontend(const v8_inspector::StringView& message) {
-    if (in_gc_ || delivering_ || !pending_messages_.empty()) {
+    if (in_gc_ || env()->is_processing_v8_interrupt() || delivering_ ||
+        !pending_messages_.empty()) {
       pending_messages_.emplace_back(
           message.is8Bit()
               ? std::u16string(message.characters8(),
@@ -128,6 +146,12 @@ class JSBindingsConnection : public BaseObject {
   void ScheduleFlush() {
     if (flush_scheduled_ || delivering_) return;
     flush_scheduled_ = true;
+    if (!in_gc_ && env()->is_processing_v8_interrupt()) {
+      auto* pending = new PendingMicrotask(this);
+      env()->context()->GetMicrotaskQueue()->EnqueueMicrotask(
+          env()->isolate(), PendingMicrotask::Run, pending);
+      return;
+    }
     BaseObjectPtr<JSBindingsConnection> strong_ref{this};
     env()->SetImmediate(
         [strong_ref](Environment*) { strong_ref->FlushPendingMessages(); },