Add VM::setDebuggerTrapCallback for runtime debugger activation - #287
Add VM::setDebuggerTrapCallback for runtime debugger activation#287robobun wants to merge 1 commit into
Conversation
WalkthroughChangesThe PR updates Bun debugger stepping, scope recovery, VM debugger-trap callbacks, trap entry points, and last-frontend inspector notifications. Debugger runtime updates
Inspector disconnect handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
53c8a52 to
e5af547
Compare
e5af547 to
b6b55b5
Compare
Preview Builds
|
b6b55b5 to
9af52a7
Compare
Port of #26867 to the Rust runtime with a simplified architecture: SIGUSR1 posts to an async-signal-safe semaphore; a dedicated thread fires notifyNeedDebuggerBreak on the main VM; JSC's SignalSender interrupts the VM (all tiers via InvalidationPoint patching) and VMTraps::handleTraps invokes a per-VM callback that activates the inspector and, when a pause is requested, enters Debugger::breakProgram(). Requires oven-sh/WebKit#287 (VM::setDebuggerTrapCallback + idempotent Debugger::attach + DebuggerCallFrame scope guard + disconnectFrontend ordering).
…45d) Main bumped WEBKIT_VERSION to 4895f45d in #34009. Re-rebase oven-sh/WebKit#287 onto that commit (previously it was c9ad5813 with the cmake fix cherry-picked) so the preview build matches main's WebKit plus just the 49-line setDebuggerTrapCallback patch. Resolved conflicts in serve-response-stream-sink-leak.test.ts by taking main's version (the threshold widen in this PR was reverted anyway).
9af52a7 to
d55f967
Compare
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
The trap callback is the right primitive. Recompiling with op_debug instead doesn't work because every codegen mode switch goes through whenIdle and already-linked functions keep their op_debug-less bytecode, so something has to be able to breakProgram() from a trap. But as placed the hook hands the callback frames it can't safely pause on, it has no re-entrancy/termination/exception contract, and two of the side hunks (isPauseAtNextOpportunitySet, idempotent attach) I think should just be dropped. Details inline, all from reading the code at d55f967 plus bun#37336, nothing here has been run.
Separately: six files change and nothing tests them. The consumer's suite passes with DebuggerCallFrame.cpp:159, the disconnectFrontend reorder, and the Debugger.cpp:180 early return each reverted, so none of the un-gated hunks are pinned by anything. I'll list the tests I'd want on the bun PR.
Things I checked that hold up: the disconnectFrontend reorder (no agent sends to the departing frontend during teardown, and PageInspectorController already does it this way), the old NoEvent race in handleTraps (already closed on base), m_globalObjects staying in sync with the idempotent attach, double scriptParsed on enable-after-preattach (doesn't happen), worker VMs getting the trap with no callback installed, and the function-pointer shape vs the other fork hooks.
| invalidateCodeBlocksOnStack(vm.topCallFrame); | ||
| #if USE(BUN_JSC_ADDITIONS) | ||
| if (auto callback = vm.debuggerTrapCallback()) | ||
| callback(vm); |
There was a problem hiding this comment.
blocking: this is not always a point where the top frame can be paused on. fireTrap poisons the trap-aware soft stack limit, so every LLInt function entry fails the check at LowLevelInterpreter.asm:1535 and lands in llint_check_stack_and_vm_traps (LLIntSlowPaths.cpp:543-563) with topCallFrame being the new callee at bc#0, before op_enter has run, before sp is lowered, and before the locals are zeroed (asm:1566-1580 does that after the slow call returns). For call-heavy code that is the main place this trap gets serviced. If the callback calls breakProgram() there and the block has since been relinked with debug opcodes (setBreakpointsActive plus one idle tick), DebuggerCallFrame::scope() takes the register path and reads an uninitialised stack slot as a JSScope*.
Two ways out that I can see, probably want both: make llint_check_stack_and_vm_traps service only VMTraps::NonDebuggerAsyncEvents (the mask VM::hasExceptionsAfterHandlingTraps already uses at VM.cpp:1078-1082 for the same reason) so the bit waits for the next op_check_traps at a loop back-edge, and make the scope() predicate refuse bc#0 (see the DebuggerCallFrame comment).
There was a problem hiding this comment.
Done in 1f04bd0: llint_check_stack_and_vm_traps services NonDebuggerAsyncEvents, and scope() now goes through CallFrame::scopeIfScopeRegisterIsLive(), which also refuses bytecode offset 0. One consequence I want to be explicit about: busy code that recurses without looping keeps the bit armed (and SignalSender busy) until it reaches a loop header or returns to the idle loop. If you would rather keep prologue servicing on the strength of the tightened predicate, it is a one-line change back.
| CONCURRENT_SAFE void notifyNeedDebuggerBreak() { traps().fireTrap(VMTraps::NeedDebuggerBreak); } | ||
| #if USE(BUN_JSC_ADDITIONS) | ||
| // Invoked from VMTraps::handleTraps for NeedDebuggerBreak after | ||
| // invalidateCodeBlocksOnStack, on the VM's owning thread at a safe point. |
There was a problem hiding this comment.
blocking: the wasm IPInt prologue is another caller where "safe point" doesn't hold. check_stack_and_vm_traps (WasmIPIntSlowPaths.cpp:1445-1461) goes straight to handleTrapsIfNeeded() with no frame tracer, and neither InPlaceInterpreter.asm nor the JS to wasm stub stores topCallFrame first, unlike every JS service site (LLINT_BEGIN, JITOperationPrologueCallFrameTracer at JITOperations.cpp:2979). So both the existing invalidateCodeBlocksOnStack(vm.topCallFrame) and the callback's breakProgram() walk a stale pointer. Upstream gets away with it because its handler never re-enters.
Either put a NativeCallFrameTracer on callFrame before handleTrapsIfNeeded there (callFrame is already trusted by setPrologueStopData on the line above), or mask NeedDebuggerBreak out of that site and say so in this comment.
There was a problem hiding this comment.
Done: the IPInt site is masked the same way, with the reason in a comment there and in the VM.h contract. Adding a frame tracer would still have left breakProgram() building a DebuggerCallFrame on a wasm top frame, which nothing here exercises, so masking seemed the honest option.
| @@ -484,6 +484,10 @@ bool VMTraps::handleTraps(VMTraps::BitField mask) | |||
| switch (event) { | |||
| case NeedDebuggerBreak: | |||
| invalidateCodeBlocksOnStack(vm.topCallFrame); | |||
There was a problem hiding this comment.
blocking: on !ENABLE(SIGNAL_BASED_VM_TRAPS) builds (Windows, since it has no HAVE_MACHINE_CONTEXT, PlatformHave.h:235 / PlatformEnable.h:986) and anywhere usePollingTraps is on, this line is {} (VMTraps.h:315-318) and DFG emits CheckTraps rather than InvalidationPoint (DFGByteCodeParser.cpp:7437-7440), modelled as touching only internal state. So the callback runs Runtime.evaluate or a whole nested pause loop from inside a live DFG/FTL frame and then returns into it with its hoisted checks intact. bun#37336 advertises _debugProcess on Windows so this is a shipping config.
invalidateCodeBlocksOnStack has no signal dependency in its body, so I'd un-gate it, and in polling mode either follow CheckTraps with an InvalidationPoint in handleCheckTraps or treat CheckTraps as clobbering the world under BUN_JSC_ADDITIONS. Until then the callback should only ever be reached from LLInt/baseline/C++ frames.
There was a problem hiding this comment.
Done: invalidateCodeBlocksOnStack is built whenever signals or BUN_JSC_ADDITIONS are on, handleDebuggerBreak() jettisons the stack unconditionally under Options::usePollingTraps() (which Options.cpp forces on for !SIGNAL_BASED_VM_TRAPS and no-JIT), and handleCheckTraps emits an InvalidationPoint after CheckTraps for linked plans so the frame exits right after the poll. I went with the invalidation point rather than making CheckTraps clobber the world, since the latter would be paid by every loop on Windows whether or not a debugger is involved.
| case NeedDebuggerBreak: | ||
| invalidateCodeBlocksOnStack(vm.topCallFrame); | ||
| #if USE(BUN_JSC_ADDITIONS) | ||
| if (auto callback = vm.debuggerTrapCallback()) |
There was a problem hiding this comment.
blocking: this is the first trap handler that re-enters the VM and it has no guard for that. takeTopPriorityTrap clears the bit with clearTrapWithoutCancellingThreadStop (:470) and the thread stop is only cancelled at scope exit (:476), so if the debugger thread re-fires NeedDebuggerBreak while callback(vm) is running (bun does this per queued CDP message), any JS the callback runs polls the bit at its next loop hint or prologue and recurses into the callback mid-dispatch. Message N+1 gets dispatched inside message N's BackendDispatcher::dispatch, and a Debugger.pause can land inside an injected-script evaluation.
It also has no termination or exception bracket. Interpreter::debug (Interpreter.cpp:1771) and pauseIfNeeded (Debugger.cpp:1069, :1213) both wrap debugger UI entry in DeferTermination plus assert-no-exception before and after. Here a termination that arrives during the CDP drain is swallowed, a stray exception is rethrown at whatever op_check_traps comes next (or trips throwScope.assertNoException() at LLIntSlowPaths.cpp:571 in debug), and under validateExceptionChecks a second callback in the same handleTraps loop asserts deterministically. The consumer's tests strip BUN_JSC_validateExceptionChecks from the inspectee which is why nobody has seen it.
Suggest: track "inside debugger trap callback" and mask &= ~NeedDebuggerBreak on entry when it's set (same shape as the isDeferringTermination() handling at :450) so a re-fire is coalesced by the outer while loop after return; wrap the call in { DeferTerminationForAWhile defer(vm); callback(vm); } followed by releaseAssertNoExceptionExceptTermination; and write the contract on setDebuggerTrapCallback (entered with no exception pending, must return that way, not re-entered).
There was a problem hiding this comment.
Done: VMTraps::handleDebuggerBreak() sets m_isHandlingDebuggerBreak, which handleTraps() uses to mask the bit on entry (same shape as the termination deferral), wraps the call in DeferTerminationForAWhile, and releaseAssertNoExceptionExceptTermination()s afterwards. The contract is on setDebuggerTrapCallback. The consumer keeps a guard of its own as well, and I am looking at whether the InjectedScript exception-check gaps are small enough to fix here so the inspectee can run with validateExceptionChecks on (see the bun PR).
| // Code compiled without CodeGenerationMode::Debugger may have its scope | ||
| // register repurposed by DFGStackLayoutPhase (needsScopeRegister() is | ||
| // false), leaving stale data in the slot. Fall through to callee->scope(). | ||
| else if (codeBlock && codeBlock->scopeRegister().isValid() && codeBlock->wasCompiledWithDebuggingOpcodes()) |
There was a problem hiding this comment.
should fix: the comment names the wrong mechanism and the predicate is still not tight enough. DFGStackLayoutPhase.cpp:169-170 sets the DFG CodeBlock's scopeRegister to invalid when needsScopeRegister() is false, so optimizing frames already fell through to callee->scope() before this change. What wasCompiledWithDebuggingOpcodes() is really standing in for is bytecode liveness: BytecodeUseDef.h:42-43 only keeps the scope register live (and only after op_enter) when the block has debug opcodes, so a baseline frame reached by OSR exit from non-debug DFG has jsUndefined or worse in that slot. That still leaves the bc#0 prologue case from the VMTraps comment, where the block does have debug opcodes but op_enter hasn't run.
ShadowChicken.cpp:336-342 walks the same frames one call earlier with a third rule (reads the slot as a JSValue, checks bytecodeIndex(), RELEASE_ASSERTs the type). I'd pull one helper both use that restates BytecodeUseDef faithfully: codeBlock && wasCompiledWithDebuggingOpcodes() && scopeRegister().isValid() && bytecodeIndex() is past op_enter, read as JSValue, dynamicDowncast, null falls through to callee->scope(). And reword the comment to name the two real hazards (dead scope local after OSR exit from non-debug DFG, trap serviced before op_enter).
There was a problem hiding this comment.
Done: CallFrame::scopeIfScopeRegisterIsLive() is the shared helper (debugging opcodes, valid register, offset past op_enter, read as JSValue, dynamicDowncast<JSScope>, null falls through), used by both DebuggerCallFrame::scope() and ShadowChicken::update(); the comment now describes the OSR-exit and prologue cases.
| // Enumerate source providers for scripts already loaded before the | ||
| // debugger attached and replay sourceParsed for each so observers | ||
| // (e.g. InspectorDebuggerAgent) can send scriptParsed events. | ||
| if (!canDispatchFunctionToObservers()) |
There was a problem hiding this comment.
consider: this early return isn't gated and isn't behaviour-neutral for the rest of the tree. sourceParsed is virtual and WebKitLegacy's WebScriptDebugger (WebScriptDebugger.mm:64-119) overrides it and calls attach() with zero observers, so its replay to WebScriptDebugDelegate silently stops happening. Doesn't matter for bun but it's the kind of thing that bites on the next upstream merge. Goes away if the attach hunk above is reverted.
There was a problem hiding this comment.
Gone along with the attach hunk.
|
|
||
| CONCURRENT_SAFE void notifyNeedDebuggerBreak() { traps().fireTrap(VMTraps::NeedDebuggerBreak); } | ||
| #if USE(BUN_JSC_ADDITIONS) | ||
| // Invoked from VMTraps::handleTraps for NeedDebuggerBreak after |
There was a problem hiding this comment.
should fix: running the callback after an unconditional whole-stack jettison means the app deopts on every CDP batch even when nothing pauses. requestThreadStopIfNeeded sets m_needToInvalidateCodeBlocks on every idle to pending transition (VMTraps.cpp:404) and invalidateCodeBlocksOnStack (:180-196) then jettisons every optimizing block on the stack. Upstream fires this once per inspector open; bun fires it per inbound CDP frame, so console autocomplete or getProperties against a busy target is a steady deopt storm, and the pause path already jettisons heap-wide through setSteppingMode anyway.
Under BUN_JSC_ADDITIONS, when a callback is installed I'd skip the pre-walk and let the callback decide, or make the signature bool (*)(VM&) and only invalidate when it returns true. Keep upstream behaviour when the callback is null.
There was a problem hiding this comment.
Done, split by trap mode: with a callback installed and signal based traps the pre-walk is skipped entirely (m_needToInvalidateCodeBlocks is just cleared), since the frame that was running optimized code has already exited via the breakpoints and the rest are at calls; with polling traps it has to happen for the reason in your other comment, so there it is unconditional. Null callback keeps upstream behaviour. The consumer also stops firing when its queue was already non-empty.
| // Invoked from VMTraps::handleTraps for NeedDebuggerBreak after | ||
| // invalidateCodeBlocksOnStack, on the VM's owning thread at a safe point. | ||
| using DebuggerTrapCallback = void (*)(VM&); | ||
| CONCURRENT_SAFE void setDebuggerTrapCallback(DebuggerTrapCallback cb) { m_debuggerTrapCallback.store(cb, std::memory_order_release); } |
There was a problem hiding this comment.
should fix (doc + consumer): nothing retires NeedDebuggerBreak for an embedder that is idle while holding the API lock. handleTraps is only reached from bytecode poll sites, C++ VM entry masks the bit out (NonDebuggerAsyncEvents, VM.cpp:1078-1082), and there is no other clearTrap(NeedDebuggerBreak). bun's idle path uses its own ACTIVATION_REQUESTED flag and never touches vm.traps(), and connect/disconnect re-fire the trap after activation, so on a mostly idle server (the headline kill -USR1 case) SignalSender keeps suspending and resuming the main thread every 1ms until some unrelated JS happens to run. The tests' idle fixture ticks a 1s timer which hides it.
Worth a line here saying an embedder that idles with the lock held must call vm.traps().handleTrapsIfNeeded(VMTraps::NeedDebuggerBreak) from its idle loop, and then bun can replace its flag re-check with exactly that so idle and busy activation share one path.
There was a problem hiding this comment.
Done: documented on the setter, and bun's idle path becomes traps().handleTrapsIfNeeded(NeedDebuggerBreak), driven off a flag every firing site sets, so connect/disconnect/message delivery on a quiet process retire the bit too.
| #if USE(BUN_JSC_ADDITIONS) | ||
| // Invoked from VMTraps::handleTraps for NeedDebuggerBreak after | ||
| // invalidateCodeBlocksOnStack, on the VM's owning thread at a safe point. | ||
| using DebuggerTrapCallback = void (*)(VM&); |
There was a problem hiding this comment.
should fix (doc): say what this does and doesn't buy. emitDebugHook is gated on CodeGenerationMode::Debugger (BytecodeGenerator.cpp:4152-4155), the only recompile is whenIdle (VM.cpp:1041-1050, RELEASE_ASSERT(!vm.entryScope) in Heap), and attach doesn't recompile. So for code that was already running at activation you get pause and evaluate, but not breakpoints, debugger; or stepping until that code is re-entered after an idle. In the headline while(true){} case idle never comes, setBreakpointByUrl reports resolved and never fires. Neither this comment, the PR text nor bun's docs ("matches Node.js") mention it.
There was a problem hiding this comment.
Done: the comment on the setter now says it buys pause and evaluate in running code, and that breakpoints, debugger and stepping wait for re-entry after an idle; bun's docs get the same paragraph.
| void clearBlackbox(); | ||
|
|
||
| bool isPaused() const { return m_isPaused; } | ||
| bool isStepping() const { return m_steppingMode == SteppingModeEnabled; } |
There was a problem hiding this comment.
should fix: stepping from a trap-initiated pause in op_debug-less code never completes and leaves stepping armed. stepOverStatement/stepOutOfFunction/stepNextExpression (Debugger.cpp:991-1031) arm m_pauseOnCallFrame as a raw CallFrame* that only the op_debug-driven return/didExecuteProgram events (:1430-1486) or unwind (:1509) ever clear, and the paused frame has no op_debug and won't get any until idle. So Step quietly becomes Continue, m_steppingMode stays Enabled (blocks DFG tier-up realm-wide), and the stale pointer can address-match some later frame. If the last frontend then disconnects from inside the callback, detach() runs unpaused and its only step cleanup is under m_isPaused (:244-248), while clearDebuggerRequests zeroes the per-block flags; on reconnect setSteppingMode(Enabled) is a no-op and step-over/into behave as resume until a plain pause+continue resets things.
Under BUN_JSC_ADDITIONS I'd have the step entry points notice m_currentCallFrame && codeBlock && !codeBlock->wasCompiledWithDebuggingOpcodes() and degrade to schedulePauseAtNextOpportunity() (the consumer re-fires the trap after runWhilePaused returns), and have detach reset m_steppingMode / immediate and eventual pause state / m_currentCallFrame when m_globalObjects empties regardless of m_isPaused.
There was a problem hiding this comment.
Done: next/over/out on a frame without debugging opcodes forward to stepIntoStatement() (so only m_pauseAtNextOpportunity is armed), and detach() resets m_currentCallFrame, the immediate/eventual/async pause state and m_steppingMode once m_globalObjects empties, regardless of m_isPaused. Pinned on the bun side by the stepInto-then-unrelated-message test and a reconnect-after-step test.
Lets an embedder service the NeedDebuggerBreak trap itself, so a debugger can be attached to a program that is already running and breakProgram() entered from code that was compiled without op_debug sites (bun uses this for SIGUSR1 / process._debugProcess). Recompiling with debug opcodes is not an alternative: code generation mode changes apply when the VM goes idle, and already linked functions keep their bytecode. VMTraps::handleDebuggerBreak() runs the callback with termination deferred (re-fired as a trap afterwards), asserts it leaves no exception behind, and masks NeedDebuggerBreak for the duration so JS run by the callback cannot nest another invocation; a re-fire is taken by the same handleTraps() loop once the callback returns. With signal based traps the whole-stack invalidation is skipped when a callback is installed (the frame that was running optimized code has already left it, the rest are parked at calls), since bun fires this trap once per CDP batch. With polling traps the callback can run inside a live DFG/FTL frame, so the stack is jettisoned unconditionally (invalidateCodeBlocksOnStack is now built in those configurations too) and the DFG emits an InvalidationPoint after each CheckTraps so that frame exits on return instead of continuing with hoisted state. The LLInt and IPInt prologue stack checks only service NonDebuggerAsyncEvents: the JS frame there is at bytecode 0 with op_enter still to run, and the wasm path has not stored topCallFrame, so neither can be paused on. The bit is taken at op_check_traps instead, or by the embedder calling handleTrapsIfNeeded(NeedDebuggerBreak) from its idle loop, which the VM.h comment now requires together with the rest of the contract and the limitations (pause and evaluate only, until running code is re-entered). DebuggerCallFrame::scope() and ShadowChicken::update() share CallFrame::scopeIfScopeRegisterIsLive(), which only reads the scope register under the conditions BytecodeUseDef keeps it live: debugging opcodes, a valid register, and a bytecode offset past op_enter. A pause entered through a trap can otherwise sit on a frame whose slot is dead (baseline reached by OSR exit from non-debug DFG) or unwritten. Step next/over/out issued while paused on a frame without debugging opcodes degrade to pausing at the next opportunity, since the hooks that would complete them never run on that frame, and Debugger::detach() resets the stepping and pause state once the last global object detaches, so a frontend disconnecting in that state does not leave the next one with stepping mode stuck on. JSGlobalObjectInspectorController::disconnectFrontend() tears the agents down only when the last frontend goes away, so a second frontend keeps working after the first disconnects.
d55f967 to
1f04bd0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
| addToGraph(CheckTraps); | ||
| // With polling traps the debugger trap callback (VMTraps::handleDebuggerBreak) runs | ||
| // arbitrary JS from inside this frame and jettisons the code blocks on the stack before | ||
| // returning; this is where the frame then leaves, instead of carrying on with whatever | ||
| // it had hoisted above the loop. | ||
| if (!m_graph.m_plan.isUnlinked()) | ||
| addToGraph(InvalidationPoint); |
There was a problem hiding this comment.
🔴 The InvalidationPoint emitted immediately after CheckTraps gets origin.exitOK=false (because CheckTraps writes InternalState and so clobbers exit state), while mayExit(InvalidationPoint)==Exits — this fails the DFGValidate.cpp:225 assertion on the very first DFG compilation whenever usePollingTraps() is on and asserts are enabled (Windows debug builds, or any debug build with JSC_usePollingTraps=1). Insert m_exitOK = true; addToGraph(ExitOK); between the two addToGraph calls, mirroring the emitExitOK() pattern at DFGByteCodeParser.cpp:7979.
Extended reasoning...
What the bug is
Under USE(BUN_JSC_ADDITIONS), the new handleCheckTraps() branch for Options::usePollingTraps() && !m_graph.m_plan.isUnlinked() emits two nodes back-to-back:
addToGraph(CheckTraps);
// ...
if (!m_graph.m_plan.isUnlinked())
addToGraph(InvalidationPoint);CheckTraps clobbers exit state, so the InvalidationPoint is created with origin.exitOK == false. But InvalidationPoint may exit, and DFG graph validation asserts that no node with mayExit() == Exits may have exitOK == false. The assertion fires immediately after bytecode parsing on every function that tiers up to DFG, before any optimization phase runs.
The specific code path
CheckTrapswritesInternalState— DFGClobberize.h:621-624 hascase CheckTraps: read(InternalState); write(InternalState); return;.- That counts as clobbering exit state — DFGClobbersExitState.cpp has no explicit case for
CheckTraps, so it falls through to the default at :113-127, which returnstruefor any write to a heap other thanSideStateorHeapObjectCount.InternalState(DFGAbstractHeap.h:85) is neither. addToGraph()therefore clearsm_exitOK— DFGByteCodeParser.cpp:871-872:if (clobbersExitState(m_graph, node)) m_exitOK = false;runs right after appendingCheckTraps.- The next
addToGraph(InvalidationPoint)inherits that —currentNodeOrigin()(DFGByteCodeParser.cpp:842-848) constructsNodeOrigin(..., m_exitOK), so theInvalidationPointat line 7447 getsorigin.exitOK = false. mayExit(InvalidationPoint) == Exits— DFGMayExit.cpp has no case forInvalidationPoint; it hits the defaultreturn Exits;at :449-451.- Validation fails — DFGValidate.cpp:225:
VALIDATE((node), !(mayExit(m_graph, node) == Exits && !node->origin.exitOK));fires.validate(dfg)runs immediately after parsing (DFGPlan.cpp:232-233) whenevervalidationEnabled(), which is unconditionallytrueunderASSERT_ENABLED(DFGCommon.h:88-94).
Step-by-step proof
Consider a Windows debug build (or any debug build run with JSC_usePollingTraps=1):
!ENABLE(SIGNAL_BASED_VM_TRAPS)on Windows (PlatformHave.h:235 / PlatformEnable.h:986), so Options.cpp:650-652 forcesOptions::usePollingTraps() = true.- Any function
fruns enough to tier up. DFG bytecode parsing begins for the linked plan (isUnlinked()is only true underOptions::forceUnlinkedDFG(), off by default). - Parsing reaches
op_enter(DFGByteCodeParser.cpp:7981) →handleCheckTraps(). TheusePollingTraps() && !isUnlinked()branch is taken. addToGraph(CheckTraps)runs: node appended,clobbersExitStatereturns true,m_exitOKis set tofalse.addToGraph(InvalidationPoint)runs:currentNodeOrigin()returns an origin withexitOK = false; the node is appended with that origin.- Parsing completes;
DFGPlan::compileInThreadImplcallsvalidate(dfg)at :232-233. Validate::validate()reaches theInvalidationPoint:mayExit(m_graph, node) == Exitsis true,node->origin.exitOKis false →VALIDATEfails →dataLogs the graph and callsCRASH().
The same happens at every op_check_traps (loop headers, DFGByteCodeParser.cpp:10492), so even if op_enter were somehow avoided, any loop would hit it.
Why nothing prevents it
The upstream (#else) branch emits either CheckTraps or InvalidationPoint, never both, so there is no exit-state ordering between them. The other place that emits back-to-back state-clobbering nodes before handleCheckTraps() — the op_enter handler — explicitly calls emitExitOK() (DFGByteCodeParser.cpp:7977-7979) to reset m_exitOK before the trap check. The new code does not.
Impact
- Debug/asserts-on builds with polling traps are unusable: the compiler thread crashes on the first DFG tier-up. Windows is a shipping config for Bun (the PR description and alii's review both call it out), and robobun's reply on the
attach()thread says the plan is to run "the runtime-inspector suite against the debug (asserts-on) build" — that run will crash immediately. - Release builds are unaffected at runtime:
exitOKis a compile-time invariant only enforced byvalidate(). The OSR exit itself would be sound (re-executingop_check_trapsafter the trap bit is cleared is idempotent), so no miscompilation occurs when validation is off.
Fix
Insert an ExitOK reset between the two nodes, exactly as op_enter does before calling handleCheckTraps():
addToGraph(CheckTraps);
if (!m_graph.m_plan.isUnlinked()) {
// CheckTraps wrote InternalState; re-executing the trap poll on OSR exit is idempotent.
m_exitOK = true;
addToGraph(ExitOK);
addToGraph(InvalidationPoint);
}(or equivalently emitExitOK(); — DFGByteCodeParser.cpp:372-375). The ExitOK node satisfies both DFGValidate.cpp:225 and the previous-node check at :229-234.
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| { | ||
| SetForScope handling(m_isHandlingDebuggerBreak, true); | ||
| // A termination that arrives while the debugger is being serviced is re-fired as a trap | ||
| // when this scope ends and taken by the handleTraps() loop that called us. | ||
| DeferTerminationForAWhile deferTermination(vm); | ||
| callback(vm); | ||
| } | ||
| scope.releaseAssertNoExceptionExceptTermination(); |
There was a problem hiding this comment.
🟡 Under JSC_validateExceptionChecks=1, a second handleDebuggerBreak() in the same handleTraps() loop still trips verifyExceptionCheckNeedIsSatisfied: releaseAssertNoExceptionExceptTermination() clears m_needExceptionCheck but does not release() the scope, so ~ThrowScope() re-simulates a throw and the next iteration's DECLARE_THROW_SCOPE asserts. Add scope.release() before the assert (or assert on the outer handleTraps() scope after the switch case instead of declaring a nested one here). This is opt-in-debug-only, but it is the exact "second callback in the same handleTraps loop" case @alii called out.
Extended reasoning...
What the bug is
handleDebuggerBreak() ends with:
auto scope = DECLARE_THROW_SCOPE(vm);
{
SetForScope handling(m_isHandlingDebuggerBreak, true);
DeferTerminationForAWhile deferTermination(vm);
callback(vm);
}
scope.releaseAssertNoExceptionExceptTermination();releaseAssertNoExceptionExceptTermination() (ExceptionScope.h:64) reads exception() → VM::exception(), which clears m_needExceptionCheck — but it does not set m_isReleased. The ThrowScope destructor (ThrowScope.cpp:44–72) then runs with m_isReleased == false, passes its own verifyExceptionCheckNeedIsSatisfied() (the flag was just cleared), computes previousScopeStackPosition = &S_ht (the outer scope in handleTraps()), finds previousScopeStackPosition > topEntryFrame false (S_ht sits below topEntryFrame when reached from op_check_traps; topEntryFrame is null when reached from the embedder idle loop), so willBeHandleByLLIntOrJIT = false and simulateThrow() sets m_needExceptionCheck = true again.
If NeedDebuggerBreak was re-fired while callback(vm) was running — the coalescing behaviour this PR explicitly relies on (VM.h: "a NeedDebuggerBreak fired while it runs is taken again once it returns") — the outer while (needHandling(mask)) loop calls handleDebuggerBreak() again. Nothing between the destructor and the second DECLARE_THROW_SCOPE reads vm.exception(), so the second ThrowScope constructor hits verifyExceptionCheckNeedIsSatisfied() with m_needExceptionCheck == true and RELEASE_ASSERT_NOT_REACHED_WITH_MESSAGE("...") fires.
Step-by-step trace
op_check_traps→operationHandleTraps→handleTraps(). Outerauto scope = DECLARE_THROW_SCOPE(vm)(call it S_ht).- Loop takes
NeedDebuggerBreak→handleDebuggerBreak(). InnerDECLARE_THROW_SCOPE(S_hdb); its ctor'sverifyExceptionCheckNeedIsSatisfied()passes. callback(vm)runs. During it another thread re-firesNeedDebuggerBreak(bun does this per queued CDP message; nestedhandleTraps()masks the bit viam_isHandlingDebuggerBreakso it stays set).scope.releaseAssertNoExceptionExceptTermination()callsVM::exception()→m_needExceptionCheck = false.- S_hdb destructs.
m_isReleased == false→ verify passes (flag cleared in step 4).previousScopeStackPosition = &S_ht; on a downward-growing stack S_ht is at a lower address thanvm.topEntryFrame(ortopEntryFrameis null from the idle loop), sowillBeHandleByLLIntOrJIT = false→simulateThrow()→m_needExceptionCheck = true. - Back in
handleTraps().needHandling(mask)(atomic load),takeTopPriorityTrap()(lock + bit ops) →NeedDebuggerBreakagain →handleDebuggerBreak(). - Inside:
vm.debuggerTrapCallback()(atomic load),Options::usePollingTraps()branch (Locker /jettisonOptimizedCodeBlocksOnStack— no exception scopes), thenDECLARE_THROW_SCOPE(vm). Ctor →verifyExceptionCheckNeedIsSatisfied()seesm_needExceptionCheck == true→ RELEASE_ASSERT.
Why the added ThrowScope doesn't close it
releaseAssertNoExceptionExceptTermination() clears the flag from any simulated throw inside callback(vm), but S_hdb's own destructor immediately re-simulates, and the outer handleTraps() loop never reads scope.exception() between iterations. Pre-PR the NeedDebuggerBreak handler had no nested ThrowScope at all, so this two-in-one-loop path is new to this PR — and is exactly the path the PR relies on for coalescing re-fires.
Impact
Only fires under Options::validateExceptionChecks() (off by default), so no production impact — it's a false positive in the validation machinery, not a real exception-handling bug. But it is the precise scenario @alii flagged ("under validateExceptionChecks a second callback in the same handleTraps loop asserts deterministically"), and robobun's reply says the goal is for the inspectee to run with validateExceptionChecks on, so leaving this in means that goal still can't be met on the coalescing path.
Fix
Easiest: add scope.release(); immediately before scope.releaseAssertNoExceptionExceptTermination(); — the destructor's m_isReleased branch then clears m_needExceptionCheck instead of verifying, and the outer S_ht is what the destructor's previousScope check considers anyway. Alternatively, drop the nested ThrowScope entirely and instead call scope.assertNoExceptionExceptTermination() on the outer handleTraps() scope right after handleDebuggerBreak() returns in the switch case.
| #if USE(BUN_JSC_ADDITIONS) | ||
| if (!currentCallFrameCanCompleteStep()) { | ||
| stepIntoStatement(); | ||
| return; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
🟡 currentCallFrameCanCompleteStep() inspects m_currentCallFrame, but stepOutOfFunction() arms m_pauseOnCallFrame at the caller — so when paused in a freshly-compiled callee H (has debug opcodes) whose caller F was compiled pre-attach (no debug opcodes), the guard passes and step-out targets F, which has no op_debug hooks to complete it. The step silently acts as continue, m_pauseOnCallFrame dangles once F returns, and m_steppingMode stays enabled. returnEvent()'s step-over→step-out retargeting (m_pauseOnCallFrame = callerFrame) has the same gap. Consider also checking the target frame's wasCompiledWithDebuggingOpcodes() and degrading to m_pauseAtNextOpportunity when it lacks them.
Extended reasoning...
What the guard checks vs. what it needs to check
currentCallFrameCanCompleteStep() (Debugger.cpp:993-998) tests whether m_currentCallFrame->codeBlock()->wasCompiledWithDebuggingOpcodes() is true. That is the correct question for stepNextExpression() and stepOverStatement(), which set m_pauseOnCallFrame = m_currentCallFrame — the frame being checked is the frame being targeted. But stepOutOfFunction() (Debugger.cpp:1057) sets m_pauseOnCallFrame = m_currentCallFrame->callerFrame(topEntryFrame): the target is one frame up. The guard says nothing about whether that caller can complete a step.
Why the mixed-opcode stack is reachable
This PR exists precisely to enable it. When the trap callback runs attach() → setBreakpointsActivated(true), hasInteractiveDebugger() flips immediately, so ScriptExecutable::defaultCodeGenerationMode() starts adding CodeGenerationMode::Debugger to any function compiled after attach. Meanwhile VM::deleteAllCode() defers via whenIdle, so already-linked functions on the stack keep their non-debug bytecode until the VM goes idle. During that window the stack is mixed: pre-attach frames have no op_debug, freshly-compiled callees do.
Step-by-step trace
- Trap-attach while F (compiled pre-attach, no debug opcodes) is running.
breakProgram()pauses in F. - User step-into.
stepIntoStatement()setsm_pauseAtNextOpportunity = true. F resumes; F has noop_debug, so nothing fires until F calls a function H that is freshly compiled (with debug opcodes). H'scallEvent/atStatementfires →pauseIfNeededseesm_pauseAtNextOpportunity→ pause in H.m_currentCallFrame = H. - User step-out.
currentCallFrameCanCompleteStep()reads H's code block →wasCompiledWithDebuggingOpcodes() == true→ returns true. Guard bypassed.stepOutOfFunction()setsm_pauseOnCallFrame = H->callerFrame() = F,m_pauseOnStepOut = true. - H resumes and returns.
returnEvent(H)fires (H hasop_debug). FirstupdateCallFrame(H, AttemptPause)→pauseIfNeededseesm_pauseOnCallFrame(F) != m_currentCallFrame(H)andm_pauseAtNextOpportunity == false→ no pause. Thenif (m_currentCallFrame == m_pauseOnCallFrame)isH == F→ false, so no retargeting. SecondupdateCallFrame(F, NoPause)setsm_currentCallFrame = F. - Control returns to F. F has no
op_debug, soatStatement/atExpression/returnEventnever fire on F.pauseIfNeededis never entered withm_currentCallFrame == F == m_pauseOnCallFrame. The step-out never completes;m_pauseOnCallFramedangles once F's stack slot is reused (can address-match a later frame → spurious pause);m_steppingModestaysEnabled(blocking DFG tier-up realm-wide) untildetach().
The second site: returnEvent()'s retarget
returnEvent() (Debugger.cpp:~1459-1462) does if (m_currentCallFrame == m_pauseOnCallFrame) { m_pauseOnCallFrame = callerFrame; m_pauseOnStepOut = true; } — treating step-over-off-the-end as step-out. If the user step-over in H instead, m_pauseOnCallFrame = H; when H returns, this branch retargets m_pauseOnCallFrame to callerFrame = F without checking whether F has debug opcodes. Same failure mode. unwindEvent() has the analogous retarget.
Impact and why this is a nit
The user-visible symptom is "step-out silently acts as continue" plus stuck stepping mode and a possible later spurious pause — a UX degradation in a specific interaction sequence, not a crash or data loss. The PR already documents on setDebuggerTrapCallback that stepping in trap-attached code is degraded and only works fully after re-entry post-idle, so user expectations are set. The detach() cleanup this PR adds (Debugger.cpp:242-257) resets m_pauseOnCallFrame/m_steppingMode on last-frontend disconnect, bounding the stale-state lifetime. The reviewer classified the parent issue as "should fix", not blocking, and the guard closes the primary case (stepping from a non-debug frame); this is the transitive case (stepping out into one).
Suggested fix
In stepOutOfFunction(), after computing the caller, check it too:
CallFrame* caller = m_currentCallFrame ? m_currentCallFrame->callerFrame(topEntryFrame) : nullptr;
if (caller && !caller->isNativeCalleeFrame()) {
if (CodeBlock* cb = caller->codeBlock(); cb && !cb->wasCompiledWithDebuggingOpcodes()) {
stepIntoStatement();
return;
}
}
m_pauseOnCallFrame = caller;And in returnEvent() / unwindEvent(), when retargeting m_pauseOnCallFrame = callerFrame, apply the same check on callerFrame and set m_pauseAtNextOpportunity = true instead when it lacks debug opcodes.
2 participants
Adds a per-VM callback for the
NeedDebuggerBreaktrap so an embedder can attach a debugger to a program that is already running and enterDebugger::breakProgram()from code that was compiled withoutop_debugsites (bun uses it forSIGUSR1/process._debugProcess, oven-sh/bun#37336). Recompiling is not an alternative: code generation mode changes apply when the VM goes idle and already linked functions keep their bytecode.Everything below is under
USE(BUN_JSC_ADDITIONS)unless noted.The callback (
VM.h,VMTraps):VMTraps::handleDebuggerBreak()runs it with termination deferred (re-fired as a trap afterwards), release-asserts it leaves no exception behind, and masksNeedDebuggerBreakwhile it is on the stack so JS run by the callback cannot nest another invocation; a re-fire is taken by the samehandleTraps()loop once it returns. The contract, the idle-loop requirement (handleTrapsIfNeeded(NeedDebuggerBreak), since nothing else retires the bit for an embedder that idles holding the API lock) and the limitations (pause and evaluate; breakpoints,debuggerand stepping only once the running code is re-entered) are documented onsetDebuggerTrapCallback.Where it may run:
llint_check_stack_and_vm_trapsand IPIntcheck_stack_and_vm_trapsserviceNonDebuggerAsyncEventsonly. The JS frame there is at bytecode 0 beforeop_enter, and the wasm path has not storedtopCallFrame; the bit is taken at the nextop_check_trapsor from the idle loop instead.Optimized frames: with signal based traps the whole-stack invalidation is skipped when a callback is installed (the frame that was running optimized code already left through the trap breakpoints, the rest are parked at calls); bun fires this trap once per CDP batch, so the per-delivery jettison mattered. With polling traps the callback can run inside a live DFG/FTL frame, so
invalidateCodeBlocksOnStackis now built in those configurations too, the callback path jettisons the stack unconditionally, andByteCodeParser::handleCheckTrapsemits anInvalidationPointafterCheckTrapsso the frame exits there on return rather than continuing with hoisted state. Upstream behaviour is unchanged when no callback is installed.Scope register:
DebuggerCallFrame::scope()andShadowChicken::update()shareCallFrame::scopeIfScopeRegisterIsLive(), which reads the slot only under the conditionsBytecodeUseDefkeeps it live (debugging opcodes, valid register, bytecode offset pastop_enter), as aJSValuewithdynamicDowncast, and otherwise falls through to the callee's scope. A trap pause can otherwise sit on a frame whose slot is dead (baseline reached by OSR exit from non-debug DFG) or unwritten.Stepping out of a trap pause: step next/over/out on a frame without debugging opcodes degrade to
stepIntoStatement()(pause at the next opportunity), since the hooks that complete them never run on that frame;Debugger::detach()resets stepping mode, the pause state andm_currentCallFramewhen the last global object detaches, so a frontend disconnecting in that state does not leave stepping stuck on for the next one.Inspector (not gated):
JSGlobalObjectInspectorController::disconnectFrontend()tears the agents down only when the last frontend disconnects, so a second frontend keeps working after the first goes away.Compared with the first revision,
Debugger::isPauseAtNextOpportunitySet()and the idempotentattach()are gone; bun readsInspectorDebuggerAgent::pauseOnNextStatementEnabled()and no longer pre-attaches. The consumer-side tests that pin these hunks are listed in oven-sh/bun#37336.Replaces #168.