inspector: avoid calling into JS from V8 interrupts

src/env-inl.h

@@ -623,6 +623,10 @@ inline void Environment::set_can_call_into_js(bool can_call_into_js) {
   can_call_into_js_ = can_call_into_js;
 }
 
+inline bool Environment::is_processing_v8_interrupt() const {
+  return is_processing_v8_interrupt_;
+}
+
 inline bool Environment::has_run_bootstrapping_code() const {
   return principal_realm_->has_run_bootstrapping_code();
 }

src/env.cc

@@ -1512,7 +1512,9 @@ void Environment::RequestInterruptFromV8() {
       return;
     }
     env->interrupt_data_.store(nullptr);
+    env->is_processing_v8_interrupt_ = true;
     env->RunAndClearInterrupts();
+    env->is_processing_v8_interrupt_ = false;
   }, interrupt_data);
 }
 

src/env.h

@@ -788,6 +788,12 @@ class Environment final : public MemoryRetainer {
   inline bool can_call_into_js() const;
   inline void set_can_call_into_js(bool can_call_into_js);
 
+  // True while RequestInterrupt() callbacks are being invoked from the
+  // v8::Isolate::RequestInterrupt() handler, i.e. potentially at an
+  // arbitrary point during JS execution. Calling into JS must be avoided
+  // in that case.
+  inline bool is_processing_v8_interrupt() const;
+
   // Increase or decrease a counter that manages whether this Environment
   // keeps the event loop alive on its own or not. The counter starts out at 0,
   // meaning it does not, and any positive value will make it keep the event
@@ -1236,6 +1242,7 @@ class Environment final : public MemoryRetainer {
   bool task_queues_async_initialized_ = false;
 
   std::atomic<Environment**> interrupt_data_ {nullptr};
+  bool is_processing_v8_interrupt_ = false;
   void RequestInterruptFromV8();
   static void CheckImmediate(uv_check_t* handle);
 

src/inspector_agent.cc

@@ -555,11 +555,7 @@ class NodeInspectorClient : public V8InspectorClient {
       return;
     }
     if (auto agent = env_->inspector_agent()) {
-      if (depth == 0) {
-        agent->DisableAsyncHook();
-      } else {
-        agent->EnableAsyncHook();
-      }
+      agent->SetAsyncHookTrackingEnabled(depth != 0);
     }
   }
 
@@ -655,6 +651,7 @@ class NodeInspectorClient : public V8InspectorClient {
 
   void installAdditionalCommandLineAPI(Local<Context> context,
                                        Local<Object> target) override {
+    if (!env_->can_call_into_js()) return;
     Local<Function> installer = env_->inspector_console_extension_installer();
     if (!installer.IsEmpty()) {
       Local<Value> argv[] = {target};
@@ -1076,58 +1073,69 @@ void Agent::RegisterAsyncHook(Isolate* isolate,
                               Local<Function> disable_function) {
   parent_env_->set_inspector_enable_async_hooks(enable_function);
   parent_env_->set_inspector_disable_async_hooks(disable_function);
-  if (pending_enable_async_hook_) {
-    CHECK(!pending_disable_async_hook_);
-    pending_enable_async_hook_ = false;
-    EnableAsyncHook();
-  } else if (pending_disable_async_hook_) {
-    CHECK(!pending_enable_async_hook_);
-    pending_disable_async_hook_ = false;
-    DisableAsyncHook();
-  }
+  SyncAsyncHookState();
 }
 
-void Agent::EnableAsyncHook() {
-  HandleScope scope(parent_env_->isolate());
-  Local<Function> enable = parent_env_->inspector_enable_async_hooks();
-  if (!enable.IsEmpty()) {
-    ToggleAsyncHook(parent_env_->isolate(), enable);
-  } else if (pending_disable_async_hook_) {
-    CHECK(!pending_enable_async_hook_);
-    pending_disable_async_hook_ = false;
-  } else {
-    pending_enable_async_hook_ = true;
-  }
+void Agent::SetAsyncHookTrackingEnabled(bool enabled) {
+  async_hook_wanted_ = enabled;
+  SyncAsyncHookState();
 }
 
-void Agent::DisableAsyncHook() {
-  HandleScope scope(parent_env_->isolate());
-  Local<Function> disable = parent_env_->inspector_disable_async_hooks();
-  if (!disable.IsEmpty()) {
-    ToggleAsyncHook(parent_env_->isolate(), disable);
-  } else if (pending_enable_async_hook_) {
-    CHECK(!pending_disable_async_hook_);
-    pending_enable_async_hook_ = false;
-  } else {
-    pending_disable_async_hook_ = true;
-  }
-}
+// Reconcile the state of the async hook used for async stack traces with the
+// state last requested by the protocol. The hook is set up in JS land,
+// (see inspector_async_hooks.js), which isn't safe to do when:
+// 1. We are in early bootstrap and the setup functions aren't registered in
+//    C++ yet.
+// 2. We are in a V8 interrupt requested by inspector protocol message
+//    dispatch e.g. from maxAsyncCallStackDepthChanged() notifications.
+// 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;
+    }
 
-void Agent::ToggleAsyncHook(Isolate* isolate, Local<Function> fn) {
-  // 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;
-  CHECK(parent_env_->has_run_bootstrapping_code());
-  HandleScope handle_scope(isolate);
-  CHECK(!fn.IsEmpty());
-  auto context = parent_env_->context();
-  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 Inspector's AsyncHook, please report this.");
+    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;
   }
 }
 

src/inspector_agent.h

@@ -90,8 +90,7 @@ class Agent {
   void RegisterAsyncHook(v8::Isolate* isolate,
     v8::Local<v8::Function> enable_function,
     v8::Local<v8::Function> disable_function);
-  void EnableAsyncHook();
-  void DisableAsyncHook();
+  void SetAsyncHookTrackingEnabled(bool enabled);
 
   void SetParentHandle(std::unique_ptr<ParentInspectorHandle> parent_handle);
   std::unique_ptr<ParentInspectorHandle> GetParentHandle(uint64_t thread_id,
@@ -132,7 +131,7 @@ class Agent {
   std::shared_ptr<NetworkResourceManager> GetNetworkResourceManager();
 
  private:
-  void ToggleAsyncHook(v8::Isolate* isolate, v8::Local<v8::Function> fn);
+  void SyncAsyncHookState();
   void ToggleNetworkTracking(v8::Isolate* isolate, v8::Local<v8::Function> fn);
 
   node::Environment* parent_env_;
@@ -150,8 +149,12 @@ class Agent {
   DebugOptions debug_options_;
   std::shared_ptr<ExclusiveAccess<HostPort>> host_port_;
 
-  bool pending_enable_async_hook_ = false;
-  bool pending_disable_async_hook_ = false;
+  // 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;
 
   bool network_tracking_enabled_ = false;
   bool pending_enable_network_tracking = false;

test/parallel/test-inspector-async-hook-after-done.js

@@ -34,8 +34,8 @@ function onAttachToWorker({ params: { sessionId } }) {
       session.once('NodeWorker.receivedMessageFromWorker', onMessageReceived);
       return;
     }
-    // Force a call to node::inspector::Agent::ToggleAsyncHook by changing the
-    // async call stack depth
+    // Force a call to node::inspector::Agent::SyncAsyncHookState by changing
+    // the async call stack depth
     postToWorkerInspector('Debugger.setAsyncCallStackDepth', { maxDepth: 1 });
     // This is were the original crash happened
     session.post('NodeWorker.detach', { sessionId }, () => {