inspector: interrupt a busy JS loop so Debugger.pause is serviced

src/jsc/bindings/BunDebugger.cpp

@@ -6,7 +6,10 @@
 #include <JavaScriptCore/JSGlobalObjectDebuggable.h>
 #include <JavaScriptCore/JSGlobalObjectDebugger.h>
 #include <JavaScriptCore/Debugger.h>
+#include <JavaScriptCore/JSCConfig.h>
+#include <JavaScriptCore/VM.h>
 #include <wtf/Condition.h>
+#include <wtf/Scope.h>
 #include "ScriptExecutionContext.h"
 #include "debug-helpers.h"
 #include "BunInjectedScriptHost.h"
@@ -229,6 +232,30 @@ class BunInspectorConnection : public Inspector::FrontendChannel {
             connections.appendVector(inspectorConnections->get(global->scriptExecutionContext()->identifier()));
         }
 
+        // Mark these connections as paused so the debugger thread skips firing a VM
+        // trap for messages that runWhilePaused will service anyway (see
+        // interruptInspectedThreadIfBusy). A depth counter keeps nested pauses correct.
+        for (auto* connection : connections)
+            connection->runWhilePausedDepth.fetch_add(1);
+        auto clearPausedDepth = WTF::makeScopeExit([&] {
+            for (auto* connection : connections)
+                connection->runWhilePausedDepth.fetch_sub(1);
+
+            // A message (e.g. a Debugger.pause) that arrived after the resume batch
+            // was swapped out but before the depth dropped saw depth>0 and skipped
+            // the VM trap, and the resume path breaks out of the loop without
+            // re-draining. Arm the trap now if anything is still queued so it is
+            // serviced at the first post-resume safepoint instead of waiting for an
+            // event-loop tick that a resumed busy loop never reaches.
+            for (auto* connection : connections) {
+                Locker<Lock> locker(connection->jsThreadMessagesLock);
+                if (!connection->jsThreadMessages.isEmpty()) {
+                    global->vm().notifyNeedShellTimeoutCheck();
+                    break;
+                }
+            }
+        });
+
         for (auto* connection : connections) {
             if (connection->status == ConnectionStatus::Pending) {
                 connection->connect();
@@ -400,6 +427,7 @@ class BunInspectorConnection : public Inspector::FrontendChannel {
             ScriptExecutionContext::postTaskTo(scriptExecutionContextIdentifier, [connection = this](ScriptExecutionContext& context) {
                 connection->receiveMessagesOnInspectorThread(context, static_cast<Zig::GlobalObject*>(context.jsGlobalObject()), true);
             });
+            interruptInspectedThreadIfBusy();
         }
     }
 
@@ -416,9 +444,37 @@ class BunInspectorConnection : public Inspector::FrontendChannel {
             ScriptExecutionContext::postTaskTo(scriptExecutionContextIdentifier, [connection = this](ScriptExecutionContext& context) {
                 connection->receiveMessagesOnInspectorThread(context, static_cast<Zig::GlobalObject*>(context.jsGlobalObject()), true);
             });
+            interruptInspectedThreadIfBusy();
         }
     }
 
+    // The task posted above only runs when the inspected thread next turns its
+    // event loop. A thread spinning in JS that never yields (e.g. `while (true)
+    // {}`) would never drain it, so a Debugger.pause sent while JS is running
+    // would be ignored forever. Fire a VM trap so the inspected thread is
+    // interrupted at its next safepoint (a loop back-edge) and services the
+    // queued messages there via dispatchPendingMessagesOnVMInterrupt(). This is
+    // how V8/Node break a busy isolate (v8::Isolate::RequestInterrupt) and how
+    // WebKit's own WebInspectorInterruptDispatcher breaks into a running page.
+    void interruptInspectedThreadIfBusy()
+    {
+        auto* inspectedGlobalObject = this->globalObject;
+        if (!inspectedGlobalObject)
+            return;
+
+        // Already paused: runWhilePaused services messages once notifyPausedThread()
+        // wakes it, so the thread is parked in C++ rather than running JS. Firing a
+        // trap here would just leave the SignalSender polling (every 1ms) for a
+        // safepoint that won't be reached until the debugger resumes. This is a
+        // best-effort optimization (the posted task and notifyPausedThread() cover
+        // both states); the counter is atomic because it is written by the JS thread
+        // in runWhilePaused and read here on the debugger thread.
+        if (runWhilePausedDepth.load() > 0)
+            return;
+
+        inspectedGlobalObject->vm().notifyNeedShellTimeoutCheck();
+    }
+
     WTF::Vector<WTF::String, 12> debuggerThreadMessages;
     WTF::Lock debuggerThreadMessagesLock = WTF::Lock();
     std::atomic<uint32_t> debuggerThreadMessageScheduledCount { 0 };
@@ -433,11 +489,57 @@ class BunInspectorConnection : public Inspector::FrontendChannel {
 
     std::atomic<ConnectionStatus> status = ConnectionStatus::Pending;
 
+    // Nonzero while the inspected thread is parked in runWhilePaused. Written by
+    // the JS thread, read by the debugger thread in interruptInspectedThreadIfBusy.
+    std::atomic<uint32_t> runWhilePausedDepth { 0 };
+
     bool unrefOnDisconnect = false;
 
     bool hasEverConnected = false;
 };
 
+// Runs on the inspected JS thread when the NeedShellTimeoutCheck VM trap is
+// serviced at a safepoint (see interruptInspectedThreadIfBusy). It drains the
+// inspector messages queued from the debugger thread so that a Debugger.pause
+// (or any command) delivered while the thread is spinning in JS takes effect
+// without waiting for the event loop to tick. Dispatching here calls
+// InspectorDebuggerAgent::pause() -> Debugger::schedulePauseAtNextOpportunity(),
+// which enables stepping and deopts the running code so the pause lands at the
+// next statement.
+static void dispatchPendingMessagesOnVMInterrupt(JSC::VM& vm)
+{
+    Vector<BunInspectorConnection*, 8> connections;
+    {
+        Locker<Lock> locker(inspectorConnectionsLock);
+        if (!inspectorConnections)
+            return;
+        for (auto& entry : *inspectorConnections) {
+            for (auto* connection : entry.value) {
+                if (connection->globalObject && &connection->globalObject->vm() == &vm)
+                    connections.append(connection);
+            }
+        }
+    }
+
+    for (auto* connection : connections) {
+        ConnectionStatus status = connection->status.load();
+        if (status == ConnectionStatus::Disconnected || status == ConnectionStatus::Disconnecting)
+            continue;
+        auto* context = ScriptExecutionContext::getScriptExecutionContext(connection->scriptExecutionContextIdentifier);
+        if (!context)
+            continue;
+        connection->receiveMessagesOnInspectorThread(*context, static_cast<Zig::GlobalObject*>(connection->globalObject), true);
+    }
+}
+
+// Must run before the first VM is constructed: Config::finalize() freezes
+// g_jscConfig when a VM is created, after which this assignment would fault.
+// Called from JSCInitialize() (ZigGlobalObject.cpp) inside JSC::initialize().
+extern "C" void Bun__Debugger__initVMInterruptDispatch()
+{
+    g_jscConfig.shellTimeoutCheckCallback = dispatchPendingMessagesOnVMInterrupt;
+}
+
 JSC_DECLARE_HOST_FUNCTION(jsFunctionSend);
 JSC_DECLARE_HOST_FUNCTION(jsFunctionDisconnect);
 

src/jsc/bindings/ZigGlobalObject.cpp

@@ -271,6 +271,10 @@ extern "C" unsigned getJSCBytecodeCacheVersion()
 extern "C" void Bun__REPRL__registerFuzzilliFunctions(Zig::GlobalObject*);
 #endif
 
+// Defined in BunDebugger.cpp. Registers the inspector's VM-interrupt dispatch
+// callback; must run before the first VM freezes g_jscConfig.
+extern "C" void Bun__Debugger__initVMInterruptDispatch();
+
 extern "C" void JSCInitialize(const char* envp[], size_t envc, void (*onCrash)(const char* ptr, size_t length), bool evalMode, bool oneShotStartup)
 {
     static std::once_flag jsc_init_flag;
@@ -345,6 +349,12 @@ extern "C" void JSCInitialize(const char* envp[], size_t envc, void (*onCrash)(c
                 }
             }
             JSC::Options::assertOptionsAreCoherent();
+
+            // Register the inspector's VM-interrupt dispatch callback while
+            // g_jscConfig is still writable. Config::finalize() freezes it once
+            // the first VM is constructed. This lets Debugger.pause interrupt a
+            // busy JS loop instead of waiting for the event loop to tick.
+            Bun__Debugger__initVMInterruptDispatch();
         }); // end JSC::initialize lambda
     }); // end std::call_once lambda
 

test/no-validate-exceptions.txt

@@ -167,3 +167,8 @@ test/regression/issue/isArray-proxy-crash.test.ts
 
 # Third-party SDK with unchecked exception path in JSArray pushInline
 test/js/third_party/@azure/service-bus/azure-service-bus.test.ts
+
+# Inspector pause builds the Debugger.paused payload via WebKit's
+# JSJavaScriptCallFrame::scopeChain, which has a pre-existing unchecked exception
+# (a plain `debugger;` pause trips it too); unrelated to the fix under test.
+test/regression/issue/32548.test.ts

test/regression/issue/32548.test.ts

@@ -0,0 +1,216 @@
+// https://github.com/oven-sh/bun/issues/32548
+
+import { expect, test } from "bun:test";
+import { bunEnv, bunExe, isASAN, tempDir } from "harness";
+
+// This regression is a timing race: the busy loop must monopolize the JS thread
+// before the queued Debugger.pause is dispatched. On optimized non-ASAN release
+// builds the pause routinely wins the race and fires even without the fix, so a
+// fail-before there is flaky and the test would be an unreliable regression
+// guard. ASAN builds (local `bun bd` and the release-asan CI lanes) run slowly
+// enough to reproduce it deterministically, so the test gates on isASAN. Those
+// lanes also need the test/no-validate-exceptions.txt entry to dodge a
+// pre-existing unchecked exception in WebKit's JSJavaScriptCallFrame::scopeChain
+// (a plain `debugger;` pause hits it too; unrelated to this fix). The fix itself
+// is platform independent.
+test.skipIf(!isASAN)("Debugger.pause interrupts a busy loop and reports call frames", async () => {
+  using dir = tempDir("issue-32548", {
+    "index.js": `
+        let counter = 0;
+        console.log("busy-ready");
+        while (true) {
+          counter++;
+          if (counter === Number.MAX_SAFE_INTEGER) console.log(counter);
+        }
+      `,
+  });
+
+  await using proc = Bun.spawn({
+    cmd: [bunExe(), "--inspect-wait=ws://127.0.0.1:0/bun32548", "index.js"],
+    env: bunEnv,
+    cwd: String(dir),
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+
+  // Parse the inspector URL from the banner on stderr, and separately watch
+  // stdout for "busy-ready" so we know the loop is actually running before we
+  // ask the debugger to pause.
+  let stderrBuf = "";
+  let stderrLineBuf = "";
+  const { promise: urlPromise, resolve: urlResolve, reject: urlReject } = Promise.withResolvers<URL>();
+  let urlFound = false;
+  (async () => {
+    const decoder = new TextDecoder();
+    for await (const chunk of proc.stderr as ReadableStream<Uint8Array>) {
+      const text = decoder.decode(chunk);
+      stderrBuf += text;
+      if (!urlFound) {
+        stderrLineBuf += text;
+        const lines = stderrLineBuf.split("\n");
+        stderrLineBuf = lines.pop() ?? "";
+        for (const line of lines) {
+          const trimmed = line.trim();
+          if (!trimmed) continue;
+          try {
+            const u = new URL(trimmed);
+            if (u.protocol === "ws:" || u.protocol === "wss:") {
+              urlFound = true;
+              urlResolve(u);
+              break;
+            }
+          } catch {}
+        }
+      }
+    }
+    if (!urlFound) {
+      urlReject(new Error(`Inspector URL not found before child stderr closed: ${JSON.stringify(stderrBuf)}`));
+    }
+  })().catch(err => {
+    if (!urlFound) urlReject(err);
+  });
+
+  let stdoutBuf = "";
+  let busyReady = false;
+  const { promise: busyPromise, resolve: busyResolve, reject: busyReject } = Promise.withResolvers<void>();
+  // Sink: busyPromise is only awaited on the happy path; if an earlier await
+  // throws first, its rejection (child exited before "busy-ready") must not
+  // surface as an unhandled rejection on top of the real failure.
+  busyPromise.catch(() => {});
+  (async () => {
+    const decoder = new TextDecoder();
+    for await (const chunk of proc.stdout as ReadableStream<Uint8Array>) {
+      stdoutBuf += decoder.decode(chunk);
+      if (!busyReady && stdoutBuf.includes("busy-ready")) {
+        busyReady = true;
+        busyResolve();
+      }
+    }
+    if (!busyReady) {
+      busyReject(new Error(`child stdout closed before "busy-ready": ${JSON.stringify(stdoutBuf)}`));
+    }
+  })().catch(err => {
+    if (!busyReady) busyReject(err);
+  });
+
+  const url = await urlPromise;
+
+  const ws = new WebSocket(url);
+  try {
+    await new Promise<void>((resolve, reject) => {
+      ws.addEventListener("open", () => resolve(), { once: true });
+      ws.addEventListener("error", e => reject(new Error("WebSocket error", { cause: e })), { once: true });
+      ws.addEventListener("close", e => reject(new Error("WebSocket closed", { cause: e })), { once: true });
+    });
+
+    let nextId = 1;
+    type Waiter = { resolve: (value: any) => void; reject: (error: Error) => void };
+    const pending = new Map<number, Waiter>();
+    const eventWaiters = new Map<string, Waiter>();
+    let closeError: Error | undefined;
+
+    const failAll = (error: Error) => {
+      if (closeError) return;
+      closeError = error;
+      for (const w of pending.values()) w.reject(error);
+      pending.clear();
+      for (const w of eventWaiters.values()) w.reject(error);
+      eventWaiters.clear();
+    };
+    ws.addEventListener("error", e => failAll(new Error("WebSocket error", { cause: e })));
+    ws.addEventListener("close", e => failAll(new Error(`WebSocket closed (${e.code})`, { cause: e })));
+
+    ws.addEventListener("message", ev => {
+      const msg = JSON.parse(String(ev.data));
+      if (typeof msg.id === "number") {
+        const w = pending.get(msg.id);
+        if (w) {
+          pending.delete(msg.id);
+          w.resolve(msg);
+        }
+      } else if (typeof msg.method === "string") {
+        const w = eventWaiters.get(msg.method);
+        if (w) {
+          eventWaiters.delete(msg.method);
+          w.resolve(msg.params);
+        }
+      }
+    });
+
+    const send = (method: string, params: Record<string, unknown> = {}) =>
+      new Promise<any>((resolve, reject) => {
+        if (closeError) return reject(closeError);
+        const id = nextId++;
+        pending.set(id, { resolve, reject });
+        ws.send(JSON.stringify({ id, method, params }));
+      });
+
+    const waitForEvent = (method: string) =>
+      new Promise<any>((resolve, reject) => {
+        if (closeError) return reject(closeError);
+        eventWaiters.set(method, { resolve, reject });
+      });
+
+    // Attach before any user code runs so the busy loop is compiled with
+    // debug hooks (setBreakpointsActive / setPauseOnDebuggerStatements force
+    // op_debug insertion), then release --inspect-wait so the loop starts.
+    await Promise.all([
+      send("Inspector.enable"),
+      send("Runtime.enable"),
+      send("Debugger.enable"),
+      send("Debugger.setBreakpointsActive", { active: true }),
+      send("Debugger.setPauseOnDebuggerStatements", { enabled: true }),
+    ]);
+
+    const pausedPromise = waitForEvent("Debugger.paused");
+    // Sink: consumed via Promise.race below only if busyPromise resolves; if an
+    // earlier await throws, failAll() rejects this waiter on WS close, which must
+    // not become an unhandled rejection.
+    pausedPromise.catch(() => {});
+    send("Inspector.initialized").catch(() => {});
+
+    // Only ask to pause once the loop is provably running. With the bug the
+    // pause command is never even dispatched, so don't block on its response;
+    // the Debugger.paused event below is the signal that matters.
+    await busyPromise;
+    send("Debugger.pause").catch(() => {});
+
+    // With the bug, no Debugger.paused event ever arrives. Bound the wait so
+    // the failure is a clear assertion, and clear the timer either way so no
+    // stray timer/rejection outlives the test.
+    let pauseTimer: ReturnType<typeof setTimeout> | undefined;
+    const paused = await Promise.race([
+      pausedPromise,
+      new Promise<never>((_, reject) => {
+        pauseTimer = setTimeout(
+          () =>
+            reject(
+              new Error("Debugger.pause produced no Debugger.paused event within 4s (busy loop was never interrupted)"),
+            ),
+          4000,
+        );
+      }),
+    ]).finally(() => clearTimeout(pauseTimer));
+
+    expect(Array.isArray(paused.callFrames)).toBe(true);
+    expect(paused.callFrames.length).toBeGreaterThan(0);
+    const top = paused.callFrames[0];
+    expect(typeof top.functionName).toBe("string");
+    expect(typeof top.location?.scriptId).toBe("string");
+    expect(typeof top.location?.lineNumber).toBe("number");
+  } catch (err) {
+    const exitCode = proc.exitCode ?? proc.signalCode ?? "(running)";
+    throw new Error(
+      `${err instanceof Error ? err.message : String(err)}\n` +
+        `  child exit: ${exitCode}\n` +
+        `  child stdout: ${JSON.stringify(stdoutBuf)}\n` +
+        `  child stderr: ${JSON.stringify(stderrBuf)}`,
+      { cause: err },
+    );
+  } finally {
+    try {
+      ws.close();
+    } catch {}
+    // `await using proc` kills the child and awaits its exit on scope exit.
+  }
+});