Skip to content

[see #168] Changes to debugger for runtime inspector #161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alii wants to merge 8 commits into from
Closed

[see #168] Changes to debugger for runtime inspector #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
86c465c
try a third time
alii Feb 13, 2026
9284eb5
cleanup and make a notifySourceParsedForExistingCode()
alii Feb 13, 2026
f87ee89
debugger: skip heap walk in notifySourceParsedForExistingCode when no…
alii Feb 13, 2026
37ae99e
fix: stop SignalSender when polling traps are enabled to prevent cras…
alii Feb 13, 2026
1fdf65d
guard isPauseAtNextOpportunitySet with USE(BUN_JSC_ADDITIONS)
alii Feb 13, 2026
40d3a63
bisect: revert VMTraps.cpp to PR 159 version (keep Debugger changes)
alii Feb 13, 2026
608dcd0
restore NeedDebuggerBreak CDP drain handler and linker flags
alii Feb 14, 2026
802b48b
fix: only tear down agents on last frontend disconnect
alii Feb 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 33 additions & 16 deletions Source/JavaScriptCore/debugger/Debugger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,34 +163,51 @@ Debugger::~Debugger()
globalObject->setDebugger(nullptr);
}

void Debugger::attach(JSGlobalObject* globalObject)
void Debugger::notifySourceParsedForExistingCode(JSGlobalObject* globalObject)
{
ASSERT(!globalObject->debugger());
globalObject->setDebugger(this);
m_globalObjects.add(globalObject);

m_vm.setShouldBuildPCToCodeOriginMapping();
// Avoid heap walk when no observers are registered — sourceParsed will
// early-return for every entry anyway.
if (!canDispatchFunctionToObservers())
return;

// Call `sourceParsed` after iterating because it will execute JavaScript in Web Inspector.
// Enumerate all CodeBlocks to find source providers for scripts that were
// already loaded before the debugger attached. Fires sourceParsed for each
// one, notifying observers (e.g., InspectorDebuggerAgent sends scriptParsed).
UncheckedKeyHashSet<RefPtr<SourceProvider>> sourceProviders;
{
JSLockHolder locker(m_vm);
HeapIterationScope iterationScope(m_vm.heap);
m_vm.heap.objectSpace().forEachLiveCell(iterationScope, [&] (HeapCell* heapCell, HeapCell::Kind kind) {
if (isJSCellKind(kind)) {
auto* cell = static_cast<JSCell*>(heapCell);
if (auto* function = jsDynamicCast<JSFunction*>(cell)) {
if (function->scope()->globalObject() == globalObject && function->executable()->isFunctionExecutable() && !function->isHostOrBuiltinFunction())
sourceProviders.add(jsCast<FunctionExecutable*>(function->executable())->source().provider());
}
m_vm.heap.forEachCodeBlock([&](CodeBlock* codeBlock) {
if (codeBlock->globalObject() == globalObject) {
if (auto* provider = codeBlock->ownerExecutable()->source().provider())
sourceProviders.add(provider);
}
return IterationStatus::Continue;
});
}
for (auto& sourceProvider : sourceProviders)
sourceParsed(globalObject, sourceProvider.get(), -1, nullString());
}

void Debugger::attach(JSGlobalObject* globalObject)
{
if (globalObject->debugger() == this) {
#if USE(BUN_JSC_ADDITIONS)
// Debugger was pre-attached before observers were registered.
// Replay sourceParsed events now that observers exist.
if (canDispatchFunctionToObservers())
notifySourceParsedForExistingCode(globalObject);
#endif
return;
}
ASSERT(!globalObject->debugger());
globalObject->setDebugger(this);
m_globalObjects.add(globalObject);

m_vm.setShouldBuildPCToCodeOriginMapping();

// Call `sourceParsed` after iterating because it will execute JavaScript in Web Inspector.
notifySourceParsedForExistingCode(globalObject);
}

void Debugger::detach(JSGlobalObject* globalObject, ReasonForDetach reason)
{
// If we're detaching from the currently executing global object, manually tear down our
Expand Down
6 changes: 6 additions & 0 deletions Source/JavaScriptCore/debugger/Debugger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ class Debugger : public DoublyLinkedListNode<Debugger> {

JS_EXPORT_PRIVATE virtual void sourceParsed(JSGlobalObject*, SourceProvider*, int errorLineNumber, const WTF::String& errorMessage);

void notifySourceParsedForExistingCode(JSGlobalObject*);

void exception(JSGlobalObject*, CallFrame*, JSValue exceptionValue, bool hasCatchHandler);
void atStatement(CallFrame*);
void atExpression(CallFrame*);
Expand All @@ -161,6 +163,10 @@ class Debugger : public DoublyLinkedListNode<Debugger> {
void registerCodeBlock(CodeBlock*);
void forEachRegisteredCodeBlock(NOESCAPE const Function<void(CodeBlock*)>&);

#if USE(BUN_JSC_ADDITIONS)
bool isPauseAtNextOpportunitySet() const { return m_pauseAtNextOpportunity; }
#endif

void didCreateNativeExecutable(NativeExecutable&);
void willCallNativeExecutable(CallFrame*);

Expand Down
9 changes: 8 additions & 1 deletion Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,14 @@ DebuggerScope* DebuggerCallFrame::scope(VM& vm)
CodeBlock* codeBlock = m_validMachineFrame->isNativeCalleeFrame() ? nullptr : m_validMachineFrame->codeBlock();
if (isTailDeleted())
scope = m_shadowChickenFrame.scope;
else if (codeBlock && codeBlock->scopeRegister().isValid())
// Code compiled without CodeGenerationMode::Debugger may have its scope
// register repurposed by the DFG (DFGStackLayoutPhase invalidates it when
// needsScopeRegister() is false). Reading the scope register from such a
// frame crashes because the slot contains stale data (e.g., a VirtualRegister
// offset instead of a JSScope pointer). ShadowChicken has the same guard.
// Falls through to callee->scope() which is always valid.
else if (codeBlock && codeBlock->scopeRegister().isValid()
&& codeBlock->wasCompiledWithDebuggingOpcodes())
scope = m_validMachineFrame->scope(codeBlock->scopeRegister().offset());
Comment on lines +155 to 163
Copy link

@coderabbitai coderabbitai bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify wasCompiledWithDebuggingOpcodes is declared on CodeBlock and check its semantics
rg "wasCompiledWithDebuggingOpcodes" -C 3 --type cpp --type h

Repository: oven-sh/WebKit

Length of output: 6185

🏁 Script executed:

#!/bin/bash
# Check needsScopeRegister relationship with debugging opcodes
rg "needsScopeRegister" -C 3 --type cpp --type h | head -100

Repository: oven-sh/WebKit

Length of output: 4204

🏁 Script executed:

#!/bin/bash
# Locate CodeBlock.h to inspect the method declaration
fd "CodeBlock.h" --type f

Repository: oven-sh/WebKit

Length of output: 697

Core guard is correct — minor readability nit on condition ordering.

The fix is sound: guarding the scope-register read with wasCompiledWithDebuggingOpcodes() is the right predicate. DFGGraph::m_hasDebuggerEnabled is directly set based on wasCompiledWithDebuggingOpcodes(), and DFGStackLayoutPhase invalidates the scope register when debugger is not enabled. The fallback to callee->scope() is safe for the attach-to-running-code scenario even though it may miss any active with-scope layer.

One readability nit: the semantic primary guard is wasCompiledWithDebuggingOpcodes(), but it is checked last. Putting it first makes the intent immediately clear at the cost of one redundant virtual call if the scope register is invalid — not a correctness issue.

♻️ Optional reordering for clarity 
-        else if (codeBlock && codeBlock->scopeRegister().isValid()
-                 && codeBlock->wasCompiledWithDebuggingOpcodes())
+        else if (codeBlock && codeBlock->wasCompiledWithDebuggingOpcodes()
+                 && codeBlock->scopeRegister().isValid())
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Code compiled without CodeGenerationMode::Debugger may have its scope
// register repurposed by the DFG (DFGStackLayoutPhase invalidates it when
// needsScopeRegister() is false). Reading the scope register from such a
// frame crashes because the slot contains stale data (e.g., a VirtualRegister
// offset instead of a JSScope pointer). ShadowChicken has the same guard.
// Falls through to callee->scope() which is always valid.
else if (codeBlock && codeBlock->scopeRegister().isValid()
&& codeBlock->wasCompiledWithDebuggingOpcodes())
scope = m_validMachineFrame->scope(codeBlock->scopeRegister().offset());
// Code compiled without CodeGenerationMode::Debugger may have its scope
// register repurposed by the DFG (DFGStackLayoutPhase invalidates it when
// needsScopeRegister() is false). Reading the scope register from such a
// frame crashes because the slot contains stale data (e.g., a VirtualRegister
// offset instead of a JSScope pointer). ShadowChicken has the same guard.
// Falls through to callee->scope() which is always valid.
else if (codeBlock && codeBlock->wasCompiledWithDebuggingOpcodes()
&& codeBlock->scopeRegister().isValid())
scope = m_validMachineFrame->scope(codeBlock->scopeRegister().offset());
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp` around lines 155 - 163,
The condition order in DebuggerCallFrame.cpp should prioritize the semantic
guard wasCompiledWithDebuggingOpcodes() for readability: change the
if-expression so codeBlock->wasCompiledWithDebuggingOpcodes() is evaluated
first, then codeBlock->scopeRegister().isValid(), and finally use that to call
m_validMachineFrame->scope(codeBlock->scopeRegister().offset()); keep the
existing fallthrough to callee->scope() unchanged; this only reorders checks
(accept the negligible redundant virtual call when the scope register is
invalid) and retains the same symbols and behavior.

else if (JSCallee* callee = jsDynamicCast<JSCallee*>(m_validMachineFrame->jsCallee()))
scope = callee->scope();
Expand Down
5 changes: 2 additions & 3 deletions Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,6 @@ void JSGlobalObjectInspectorController::connectFrontend(FrontendChannel& fronten

void JSGlobalObjectInspectorController::disconnectFrontend(FrontendChannel& frontendChannel)
{
// FIXME: change this to notify agents which frontend has disconnected (by id).
m_agents.willDestroyFrontendAndBackend(DisconnectReason::InspectorDestroyed);

m_frontendRouter->disconnectFrontend(frontendChannel);

m_isAutomaticInspection = false;
Expand All @@ -161,6 +158,8 @@ void JSGlobalObjectInspectorController::disconnectFrontend(FrontendChannel& fron
if (!disconnectedLastFrontend)
return;

m_agents.willDestroyFrontendAndBackend(DisconnectReason::InspectorDestroyed);

#if ENABLE(INSPECTOR_ALTERNATE_DISPATCHERS)
if (m_augmentingClient)
m_augmentingClient->inspectorDisconnected();
Expand Down
25 changes: 25 additions & 0 deletions Source/JavaScriptCore/runtime/VMTraps.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "CallFrameInlines.h"
#include "CodeBlock.h"
#include "CodeBlockSet.h"
#include "Debugger.h"
#include "DFGCommonData.h"
#include "ExceptionHelpers.h"
#include "HeapInlines.h"
Expand All @@ -46,6 +47,11 @@
#include <wtf/ThreadMessage.h>
#include <wtf/threads/Signals.h>

#if USE(BUN_JSC_ADDITIONS)
extern "C" __attribute__((weak)) void Bun__drainQueuedCDPMessages(JSC::VM&);
extern "C" __attribute__((weak)) bool Bun__shouldBreakAfterMessageDrain(JSC::VM&);
#endif

namespace JSC {

#if ENABLE(SIGNAL_BASED_VM_TRAPS)
Expand Down Expand Up @@ -474,9 +480,28 @@ bool VMTraps::handleTraps(VMTraps::BitField mask)
bool didHandleTrap = false;
while (needHandling(mask)) {
auto event = takeTopPriorityTrap(mask);
if (event == NoEvent)
break;
switch (event) {
case NeedDebuggerBreak:
invalidateCodeBlocksOnStack(vm.topCallFrame);
#if USE(BUN_JSC_ADDITIONS)
// Drain queued CDP messages. If a command like Debugger.pause
// is dispatched, it sets m_javaScriptPauseScheduled on the agent.
if (Bun__drainQueuedCDPMessages)
Bun__drainQueuedCDPMessages(vm);
// Only enter breakProgram() if a pause was actually requested
// (bootstrap, Debugger.pause command, breakpoint). For plain
// message delivery, the drain above is sufficient.
if (!Bun__shouldBreakAfterMessageDrain || Bun__shouldBreakAfterMessageDrain(vm)) {
if (vm.topCallFrame) {
if (auto* globalObject = vm.topCallFrame->lexicalGlobalObject(vm)) {
if (auto* debugger = globalObject->debugger())
debugger->breakProgram();
}
}
}
#endif
didHandleTrap = true;
break;
Comment on lines 486 to 506
Copy link

@coderabbitai coderabbitai bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check git history for VMTraps.cpp
cd Source/JavaScriptCore/runtime
git log --oneline -n 30 VMTraps.cpp

Repository: oven-sh/WebKit

Length of output: 123

🏁 Script executed:

#!/bin/bash
# Get the detailed diff for the NeedDebuggerBreak case to see what changed
git show HEAD -- Source/JavaScriptCore/runtime/VMTraps.cpp | grep -A 40 -B 5 "NeedDebuggerBreak"

Repository: oven-sh/WebKit

Length of output: 1974

🏁 Script executed:

#!/bin/bash
# Check the previous version to see the original implementation
git show HEAD~1:Source/JavaScriptCore/runtime/VMTraps.cpp 2>/dev/null | sed -n '480,510p' || echo "Previous version not available or file path differs"

Repository: oven-sh/WebKit

Length of output: 40

🏁 Script executed:

#!/bin/bash
# Check the parent commit to see original code
git show HEAD~1:Source/JavaScriptCore/runtime/VMTraps.cpp | grep -A 20 "NeedDebuggerBreak" | head -30

Repository: oven-sh/WebKit

Length of output: 94

🏁 Script executed:

#!/bin/bash
# Also check if NeedDebuggerBreak existed at all before
git log --all -S "NeedDebuggerBreak" --oneline -- Source/JavaScriptCore/runtime/VMTraps.cpp | head -10

Repository: oven-sh/WebKit

Length of output: 424

🏁 Script executed:

#!/bin/bash
# Get more context - what does the current file look like in full context?
git show HEAD:Source/JavaScriptCore/runtime/VMTraps.cpp | sed -n '480,520p'

Repository: oven-sh/WebKit

Length of output: 1771

🏁 Script executed:

#!/bin/bash
# Find all references to NeedDebuggerBreak
rg "NeedDebuggerBreak" --type cpp --type h -B 3 -A 3

Repository: oven-sh/WebKit

Length of output: 6042

🏁 Script executed:

#!/bin/bash
# Check the enum definition for trap events
rg "enum.*Trap|NeedDebuggerBreak" Source/JavaScriptCore/runtime/VMTraps.h -B 5 -A 5

Repository: oven-sh/WebKit

Length of output: 1453

🏁 Script executed:

#!/bin/bash
# Search for any unconditional breakProgram calls in the trap handler
rg "breakProgram" Source/JavaScriptCore/runtime/VMTraps.cpp -B 2 -A 2

Repository: oven-sh/WebKit

Length of output: 621

🏁 Script executed:

#!/bin/bash
# Check how USE(BUN_JSC_ADDITIONS) is defined
rg "define.*USE.*BUN_JSC_ADDITIONS|#if.*USE.*BUN_JSC_ADDITIONS" -B 2 -A 2 | head -40

Repository: oven-sh/WebKit

Length of output: 2516

🏁 Script executed:

#!/bin/bash
# Check if this code is meant to work without Bun additions
rg "NeedDebuggerBreak" Source/WebKit -A 5 -B 5 | grep -A 10 -B 10 "notifyNeedDebuggerBreak"

Repository: oven-sh/WebKit

Length of output: 3260

🏁 Script executed:

#!/bin/bash
# Look for similar patterns - how do other trap handlers handle conditional code?
rg "case Need" Source/JavaScriptCore/runtime/VMTraps.cpp -A 15 | head -80

Repository: oven-sh/WebKit

Length of output: 2202

The entire breakProgram() handler path is conditionally compiled on USE(BUN_JSC_ADDITIONS), breaking debugger functionality when disabled.

WebKit's WebInspectorInterruptDispatcher unconditionally calls vm->notifyNeedDebuggerBreak() to pause execution during debugging. However, the handler only calls breakProgram() inside the #if USE(BUN_JSC_ADDITIONS) guard. When this macro is disabled, the NeedDebuggerBreak trap becomes a silent no-op—only invalidateCodeBlocksOnStack() runs, and all debugger pause requests are silently dropped.

Unlike other trap handlers (NeedShellTimeoutCheck, NeedWatchdogCheck, NeedTermination, NeedStopTheWorld, NeedExceptionHandling), which all execute unconditionally, this is the only handler with core functionality guarded. The Bun-specific parts (CDP message draining, the shouldBreakAfterMessageDrain check) can remain guarded, but the core breakProgram() invocation must execute unconditionally to preserve standard WebKit debugger behavior.

Add an #else clause that preserves the unconditional breakProgram() call:

Proposed fix 
             }
         }
 `#endif`
+#else
+            if (vm.topCallFrame) {
+                if (auto* globalObject = vm.topCallFrame->lexicalGlobalObject(vm)) {
+                    if (auto* debugger = globalObject->debugger())
+                        debugger->breakProgram();
+                }
+            }
+#endif
             didHandleTrap = true;
             break;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
case NeedDebuggerBreak:
invalidateCodeBlocksOnStack(vm.topCallFrame);
#if USE(BUN_JSC_ADDITIONS)
// Drain queued CDP messages. If a command like Debugger.pause
// is dispatched, it sets m_javaScriptPauseScheduled on the agent.
if (Bun__drainQueuedCDPMessages)
Bun__drainQueuedCDPMessages(vm);
// Only enter breakProgram() if a pause was actually requested
// (bootstrap, Debugger.pause command, breakpoint). For plain
// message delivery, the drain above is sufficient.
if (!Bun__shouldBreakAfterMessageDrain || Bun__shouldBreakAfterMessageDrain(vm)) {
if (vm.topCallFrame) {
if (auto* globalObject = vm.topCallFrame->lexicalGlobalObject(vm)) {
if (auto* debugger = globalObject->debugger())
debugger->breakProgram();
}
}
}
#endif
didHandleTrap = true;
break;
case NeedDebuggerBreak:
invalidateCodeBlocksOnStack(vm.topCallFrame);
`#if` USE(BUN_JSC_ADDITIONS)
// Drain queued CDP messages. If a command like Debugger.pause
// is dispatched, it sets m_javaScriptPauseScheduled on the agent.
if (Bun__drainQueuedCDPMessages)
Bun__drainQueuedCDPMessages(vm);
// Only enter breakProgram() if a pause was actually requested
// (bootstrap, Debugger.pause command, breakpoint). For plain
// message delivery, the drain above is sufficient.
if (!Bun__shouldBreakAfterMessageDrain || Bun__shouldBreakAfterMessageDrain(vm)) {
if (vm.topCallFrame) {
if (auto* globalObject = vm.topCallFrame->lexicalGlobalObject(vm)) {
if (auto* debugger = globalObject->debugger())
debugger->breakProgram();
}
}
}
`#else`
if (vm.topCallFrame) {
if (auto* globalObject = vm.topCallFrame->lexicalGlobalObject(vm)) {
if (auto* debugger = globalObject->debugger())
debugger->breakProgram();
}
}
`#endif`
didHandleTrap = true;
break;
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@Source/JavaScriptCore/runtime/VMTraps.cpp` around lines 486 - 506, The
NeedDebuggerBreak trap currently only calls debugger->breakProgram() inside the
`#if` USE(BUN_JSC_ADDITIONS) block, causing debugger pauses to be dropped when
that macro is disabled; update VMTraps.cpp so the core breakProgram() invocation
runs unconditionally: keep the Bun-specific CDP drain and
Bun__shouldBreakAfterMessageDrain check inside the `#if`, but add an `#else` path
that, when a pause is required, walks vm.topCallFrame -> lexicalGlobalObject(vm)
and invokes debugger->breakProgram() (or otherwise call breakProgram() directly)
so that invalidateCodeBlocksOnStack(vm.topCallFrame) is followed by the standard
unconditional breakProgram() behavior even when USE(BUN_JSC_ADDITIONS) is false,
referencing NeedDebuggerBreak, invalidateCodeBlocksOnStack, vm.topCallFrame,
lexicalGlobalObject, and debugger->breakProgram() to locate where to add the
`#else` branch.


Expand Down
8 changes: 6 additions & 2 deletions Source/cmake/WebKitCompilerFlags.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,9 @@ if (COMPILER_IS_GCC_OR_CLANG)
-Wl,-U,_WTFTimer__secondsUntilTimer
-Wl,-U,_WTFTimer__cancel
-Wl,-U,_Bun__errorInstance__finalize
-Wl,-U,_Bun__reportUnhandledError)
-Wl,-U,_Bun__reportUnhandledError
-Wl,-U,_Bun__drainQueuedCDPMessages
-Wl,-U,_Bun__shouldBreakAfterMessageDrain)
else()
WEBKIT_PREPEND_GLOBAL_COMPILER_FLAGS(-Wl,-u,_WTFTimer__create
-Wl,-u,_WTFTimer__update
Expand All @@ -274,7 +276,9 @@ if (COMPILER_IS_GCC_OR_CLANG)
-Wl,-u,_WTFTimer__secondsUntilTimer
-Wl,-u,_WTFTimer__cancel
-Wl,-u,_Bun__errorInstance__finalize
-Wl,-u,_Bun__reportUnhandledError)
-Wl,-u,_Bun__reportUnhandledError
-Wl,-u,_Bun__drainQueuedCDPMessages
-Wl,-u,_Bun__shouldBreakAfterMessageDrain)
endif()
endif ()

Expand Down
Loading