deps: re-land V8 API interrupt JS check

common.gypi

@@ -44,7 +44,7 @@
 
     # Reset this number to 0 on major V8 upgrades.
     # Increment by one for each non-official patch applied to deps/v8.
-    'v8_embedder_string': '-node.15',
+    'v8_embedder_string': '-node.16',
 
     ##### V8 defaults for Node.js #####
 
@@ -56,6 +56,8 @@
     # Refs: https://github.com/nodejs/node/issues/23167
     # Enable compiler warnings when using V8_DEPRECATED apis from V8 code.
     'v8_deprecation_warnings': 0,
+    # Check that JavaScript execution is disallowed in V8 API interrupts.
+    'v8_disallow_js_in_api_interrupts_is_checked': 1,
     # Enable compiler warnings when using V8_DEPRECATE_SOON apis from V8 code.
     'v8_imminent_deprecation_warnings': 0,
 
@@ -557,6 +559,9 @@
       ['v8_deprecation_warnings == 1', {
         'defines': ['V8_DEPRECATION_WARNINGS',],
       }],
+      ['v8_disallow_js_in_api_interrupts_is_checked == 1', {
+        'defines': ['V8_DISALLOW_JS_IN_API_INTERRUPTS_IS_CHECKED',],
+      }],
       ['v8_imminent_deprecation_warnings == 1', {
         'defines': ['V8_IMMINENT_DEPRECATION_WARNINGS',],
       }],

deps/v8/AUTHORS

@@ -102,6 +102,7 @@ Cheng Zhao <zcbenz@gmail.com>
 Chengzhong Wu <legendecas@gmail.com>
 Choongwoo Han <cwhan.tunz@gmail.com>
 Chris Nardi <hichris123@gmail.com>
+Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
 Christopher A. Taylor <chris@gameclosure.com>
 Christopher Nady <nady.christopher@gmail.com>
 Colin Ihrig <cjihrig@gmail.com>

deps/v8/src/inspector/v8-debugger.cc

@@ -524,6 +524,11 @@ void V8Debugger::handleProgramBreak(
   DCHECK(contextGroupId);
   m_pausedContextGroupId = contextGroupId;
 
+  // Breaks scheduled through an API interrupt (Debugger.pause, OOM) arrive
+  // here with JavaScript execution disallowed. Like the pause message loop,
+  // the paused and resumed notifications can reach an embedder channel that
+  // runs JavaScript.
+  v8::Isolate::AllowJavascriptExecutionScope allow_script(m_isolate);
   m_inspector->forEachSession(
       contextGroupId,
       [&pausedContext, &exception, &breakpointIds, &exceptionType, &isUncaught,
@@ -535,7 +540,6 @@ void V8Debugger::handleProgramBreak(
         }
       });
   {
-    v8::Isolate::AllowJavascriptExecutionScope allow_script(m_isolate);
     v8::Context::Scope scope(pausedContext);
 
     m_inspector->forEachSession(

deps/v8/test/unittests/inspector/inspector-unittest.cc

@@ -5,6 +5,8 @@
 #include <memory>
 #include <span>
 
+#include "include/v8-external.h"
+#include "include/v8-function.h"
 #include "include/v8-inspector.h"
 #include "include/v8-local-handle.h"
 #include "include/v8-primitive.h"
@@ -625,5 +627,94 @@ TEST_F(InspectorTest, WrapObjectWithCustomPreviewNoPreview) {
   EXPECT_FALSE(remote_object->hasCustomPreview());
   EXPECT_EQ(channel.triggerCount(), 0);
 }
+
+namespace {
+
+// Calls into JavaScript for every Debugger.paused and Debugger.resumed
+// notification, like an embedder that exposes an in-process session to JS.
+class JSCallbackChannel : public V8Inspector::Channel {
+ public:
+  JSCallbackChannel(v8::Isolate* isolate, v8::Local<v8::Context> context,
+                    v8::Local<v8::Function> callback)
+      : m_isolate(isolate),
+        m_context(isolate, context),
+        m_callback(isolate, callback) {}
+  ~JSCallbackChannel() override = default;
+
+  void sendResponse(int callId,
+                    std::unique_ptr<StringBuffer> message) override {}
+
+  void sendNotification(std::unique_ptr<StringBuffer> message) override {
+    String16 message_str = toString16(message->string());
+    if (message_str.find("Debugger.paused") == String16::kNotFound &&
+        message_str.find("Debugger.resumed") == String16::kNotFound) {
+      return;
+    }
+    v8::HandleScope handle_scope(m_isolate);
+    v8::Local<v8::Context> context = m_context.Get(m_isolate);
+    v8::Context::Scope context_scope(context);
+    m_callback.Get(m_isolate)
+        ->Call(context, v8::Undefined(m_isolate), 0, nullptr)
+        .ToLocalChecked();
+  }
+
+  void flushProtocolNotifications() override {}
+
+ private:
+  v8::Isolate* m_isolate;
+  v8::Global<v8::Context> m_context;
+  v8::Global<v8::Function> m_callback;
+};
+
+void DispatchPause(const v8::FunctionCallbackInfo<v8::Value>& info) {
+  static_cast<V8InspectorSession*>(info.Data().As<v8::External>()->Value(
+                                       v8::kExternalPointerTypeTagDefault))
+      ->dispatchProtocolMessage(
+          toStringView(R"({"id": 2, "method": "Debugger.pause"})"));
+}
+
+}  // namespace
+
+TEST_F(InspectorTest, ChannelCanRunJSWhenPausedOnInterrupt) {
+  v8::Isolate* isolate = v8_isolate();
+  v8::HandleScope handle_scope(isolate);
+
+  v8_inspector::V8InspectorClient default_client;
+  std::unique_ptr<V8Inspector> inspector =
+      V8Inspector::create(isolate, &default_client);
+  V8ContextInfo context_info(v8_context(), 1, toStringView(""));
+  inspector->contextCreated(context_info);
+
+  v8::Local<v8::Function> callback =
+      RunJS("globalThis.notifications = 0; () => notifications++")
+          .As<v8::Function>();
+  JSCallbackChannel channel(isolate, v8_context(), callback);
+  std::unique_ptr<V8InspectorSession> session = inspector->connect(
+      1, &channel, toStringView("{}"), v8_inspector::V8Inspector::kFullyTrusted,
+      v8_inspector::V8Inspector::kNotWaitingForDebugger);
+  session->dispatchProtocolMessage(
+      toStringView(R"({"id": 1, "method": "Debugger.enable"})"));
+
+  v8::Local<v8::Function> pause =
+      v8::Function::New(v8_context(), &DispatchPause,
+                        v8::External::New(isolate, session.get(),
+                                          v8::kExternalPointerTypeTagDefault))
+          .ToLocalChecked();
+  CHECK(v8_context()
+            ->Global()
+            ->Set(v8_context(), NewString("pause"), pause)
+            .FromJust());
+
+  // Debugger.pause while JavaScript runs schedules the break through an API
+  // interrupt, where JavaScript execution is disallowed. The loop spins until
+  // it takes that interrupt on a back edge. Taken on function entry instead,
+  // V8 would step in and pause at the next statement, outside the interrupt,
+  // so the count is read in the same script.
+  v8::Local<v8::Value> result = RunJS(
+      "pause();"
+      "for (let i = 0; i < 1e7 && notifications === 0; i++) {}"
+      "notifications");
+  CHECK_EQ(result->Int32Value(v8_context()).FromJust(), 2);
+}
 }  // namespace internal
 }  // namespace v8

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,