Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Source/JavaScriptCore/debugger/Debugger.cpp`:
- Around line 166-183: Add an early-return guard at the start of
Debugger::notifySourceParsedForExistingCode to skip allocating sourceProviders
and walking the heap when there are no registered observers for sourceParsed;
check the debugger's observer registry (e.g., a
hasObservers()/m_observers.isEmpty() style check) and return immediately if
empty, otherwise proceed to create sourceProviders, call
m_vm.heap.forEachCodeBlock to collect providers, and then call sourceParsed as
before.
Preview Builds
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Source/JavaScriptCore/debugger/Debugger.h`:
- Around line 166-167: The accessor isPauseAtNextOpportunitySet() is a
Bun-specific API and must be guarded with the build flag; wrap its declaration
and corresponding implementation (the method in class Debugger and any inline
definition) with `#if` USE(BUN_JSC_ADDITIONS) ... `#endif` so it is only compiled
when BUN_JSC_ADDITIONS is enabled, similar to other Bun-specific members in
Debugger.h/Debugger.cpp.
In `@Source/JavaScriptCore/runtime/VMTraps.cpp`:
- Around line 497-513: The block using Bun event-loop hooks should be compiled
only when the Bun event loop is enabled: wrap the calls and checks to
Bun__drainQueuedCDPMessages and Bun__shouldBreakAfterMessageDrain inside a
USE(BUN_EVENT_LOOP) guard (in addition to the existing USE(BUN_JSC_ADDITIONS])
around the VMTraps.cpp snippet) so those symbols are only referenced when the
event loop is enabled; also add/mirror the same USE(BUN_EVENT_LOOP) guard around
the weak extern declarations for Bun__drainQueuedCDPMessages and
Bun__shouldBreakAfterMessageDrain near the top of the file so the declarations
and uses are consistently guarded.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Source/JavaScriptCore/runtime/VMTraps.cpp`:
- Around line 50-53: The extern "C" weak declarations
Bun__drainQueuedCDPMessages and Bun__shouldBreakAfterMessageDrain are only
guarded by USE(BUN_JSC_ADDITIONS) but the linker flags are added under
USE(BUN_EVENT_LOOP); update both the declarations (the block around
Bun__drainQueuedCDPMessages and Bun__shouldBreakAfterMessageDrain) and their
usage sites (the section handling message drain logic) to be guarded by both
USE(BUN_JSC_ADDITIONS) and USE(BUN_EVENT_LOOP) so the declarations and calls are
compiled only when the corresponding link-time symbols are available.
alii
changed the title
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Source/JavaScriptCore/runtime/VMTraps.cpp`:
- Around line 488-504: The CDP drain and break logic currently wrapped only by
USE(BUN_JSC_ADDITIONS) must also be guarded by USE(BUN_EVENT_LOOP); update the
preprocessor condition around the block that calls
Bun__drainQueuedCDPMessages(vm) and checks Bun__shouldBreakAfterMessageDrain(vm)
(the section that inspects vm.topCallFrame, calls lexicalGlobalObject(vm) and
invokes debugger->breakProgram()) so it is enclosed by both
USE(BUN_JSC_ADDITIONS) and USE(BUN_EVENT_LOOP) (e.g., combine the macros in the
`#if/`#endif) to match the extern declarations and ensure event-loop hooks are
only compiled when enabled.
- Around line 50-53: The weak extern declarations for
Bun__drainQueuedCDPMessages and Bun__shouldBreakAfterMessageDrain are event-loop
hooks and must be compiled only when both Bun JSC additions and the Bun event
loop are enabled; update the preprocessor guard around the declarations of
Bun__drainQueuedCDPMessages and Bun__shouldBreakAfterMessageDrain to require
USE(BUN_JSC_ADDITIONS) && USE(BUN_EVENT_LOOP) (i.e., change the existing `#if`
USE(BUN_JSC_ADDITIONS) to check both macros) so these symbols are only declared
when both features are present.
…h on jettisoned code
Previously, disconnectFrontend unconditionally called willDestroyFrontendAndBackend which detached the debugger and disabled all agents — even when other frontends were still connected. This caused CDP responses to be lost on client reconnection. Move the agent teardown after the hasFrontends() check so it only runs when the last frontend disconnects.
WalkthroughThe changes restructure JavaScriptCore's debugger attachment flow, refactoring the public attach method and introducing notifySourceParsedForExistingCode for pre-loaded code providers. Updates include scope guard validation in debugger call frames, deferred agent notification on inspector frontend disconnect, CDP message draining integration in VM trap handling, and expanded linker symbol flags. Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Source/JavaScriptCore/runtime/VMTraps.cpp (1)
50-53: Guard Bun event-loop hooks withUSE(BUN_EVENT_LOOP)in addition toUSE(BUN_JSC_ADDITIONS).The linker flags for these two symbols are gated behind
USE_BUN_EVENT_LOOPinWebKitCompilerFlags.cmake(Lines 259-282). The declarations (Lines 50-53) and usage (Lines 488-504) should match that guard to stay consistent, otherwise the symbols are referenced in builds where the linker doesn't expect them. As per coding guidelines, "Guard Bun-specific features with USE(BUN_JSC_ADDITIONS) and event-loop integration with USE(BUN_EVENT_LOOP)".Also applies to: 488-504
🤖 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 50 - 53, The declarations for Bun__drainQueuedCDPMessages and Bun__shouldBreakAfterMessageDrain and their usage must be wrapped with both feature guards; update the existing `#if` USE(BUN_JSC_ADDITIONS) blocks to `#if` USE(BUN_JSC_ADDITIONS) && USE(BUN_EVENT_LOOP) for the extern "C" weak declarations (Bun__drainQueuedCDPMessages, Bun__shouldBreakAfterMessageDrain) and mirror the same combined guard around the code that calls or references those symbols (the VM trap handling block that invokes Bun__drainQueuedCDPMessages/Bun__shouldBreakAfterMessageDrain) so declarations and usages match the linker flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp`:
- Around line 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.
In `@Source/JavaScriptCore/runtime/VMTraps.cpp`:
- Around line 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.
---
Duplicate comments:
In `@Source/JavaScriptCore/runtime/VMTraps.cpp`:
- Around line 50-53: The declarations for Bun__drainQueuedCDPMessages and
Bun__shouldBreakAfterMessageDrain and their usage must be wrapped with both
feature guards; update the existing `#if` USE(BUN_JSC_ADDITIONS) blocks to `#if`
USE(BUN_JSC_ADDITIONS) && USE(BUN_EVENT_LOOP) for the extern "C" weak
declarations (Bun__drainQueuedCDPMessages, Bun__shouldBreakAfterMessageDrain)
and mirror the same combined guard around the code that calls or references
those symbols (the VM trap handling block that invokes
Bun__drainQueuedCDPMessages/Bun__shouldBreakAfterMessageDrain) so declarations
and usages match the linker flags.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
Source/JavaScriptCore/debugger/Debugger.cppSource/JavaScriptCore/debugger/Debugger.hSource/JavaScriptCore/debugger/DebuggerCallFrame.cppSource/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cppSource/JavaScriptCore/runtime/VMTraps.cppSource/cmake/WebKitCompilerFlags.cmake
Comment on lines
+155
to
163
| // 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()); |
There was a problem hiding this comment.
🧹 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 hRepository: 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 -100Repository: oven-sh/WebKit
Length of output: 4204
🏁 Script executed:
#!/bin/bash
# Locate CodeBlock.h to inspect the method declaration
fd "CodeBlock.h" --type fRepository: 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.
Comment on lines
486
to
506
| 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; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check git history for VMTraps.cpp
cd Source/JavaScriptCore/runtime
git log --oneline -n 30 VMTraps.cppRepository: 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 -30Repository: 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 -10Repository: 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 3Repository: 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 5Repository: 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 2Repository: 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 -40Repository: 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 -80Repository: 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#168
Summary
Fixes for JSC's Debugger to support Bun's SIGUSR1 runtime inspector activation, where the debugger attaches to a VM that is already executing code (e.g.,
while(true){}).1. Idempotent
Debugger::attach()with source replayWhen
attach()is called on a GlobalObject that already has this debugger (pre-attach case), replaysourceParsedevents for all existing code blocks. Handles the case where the debugger was set on the GlobalObject before observers were registered — the initialsourceParsedcalls were no-ops becausecanDispatchFunctionToObservers()returned false.2.
notifySourceParsedForExistingCode()helperUses
Heap::forEachCodeBlock()to find allSourceProviders belonging to a GlobalObject viaownerExecutable()->source().provider(). Covers both function and module sources. Replaces the previous heap cell iteration approach.3.
wasCompiledWithDebuggingOpcodes()guard inDebuggerCallFrame::scope()Code compiled without
CodeGenerationMode::Debuggermay have its scope register repurposed by the DFG. Reading it crashes because the slot contains stale data (e.g., a VirtualRegister offset instead of a JSScope pointer). Added the same guard thatShadowChickenalready uses — falls through tocallee->scope()which is always valid.4.
isPauseAtNextOpportunitySet()accessorPublic accessor for
m_pauseAtNextOpportunity, used by Bun to check if aDebugger.pauseCDP command was dispatched during a message drain.5. VMTraps
NeedDebuggerBreakhandler improvementsneedHandling()returns true buttakeTopPriorityTrap()returnsNoEventdue to concurrent bit clearing. Previously hitRELEASE_ASSERT_NOT_REACHED.Bun__drainQueuedCDPMessages(weak symbol) dispatches queued CDP messages beforebreakProgram()so commands likeDebugger.pauseset the correct pause reason on the agent.breakProgram():Bun__shouldBreakAfterMessageDrain(weak symbol) checks if a pause was actually requested. Plain message delivery skips the pause loop.6. Only tear down agents on last frontend disconnect
Previously,
disconnectFrontendunconditionally calledwillDestroyFrontendAndBackendwhich detached the debugger and disabled all agents — even when other frontends were still connected. This caused CDP responses to be lost on client reconnection. Moved the agent teardown after thehasFrontends()check so it only runs when the last frontend disconnects.7. Linker flags
Added
-Wl,-U/-Wl,-uflags forBun__drainQueuedCDPMessagesandBun__shouldBreakAfterMessageDrainfor the standalone JSC build.