InspectorDebuggerAgent: make Debugger.disable idempotent - #385
Conversation
Walkthrough
ChangesDebugger disable handling
🚥 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}
There was a problem hiding this comment.
LGTM — the guard mirrors enable() / willDestroyFrontendAndBackend() and matches the idempotent-disable pattern in InspectorConsoleAgent / InspectorHeapAgent. The inline note on clearInspectorBreakpointState is pre-existing and can be handled separately.
Extended reasoning...
Overview
This PR adds a 3-line early-return to InspectorDebuggerAgent::disable() so a second Debugger.disable on an already-disabled agent returns {} instead of re-entering internalDisable() and tripping ASSERT(!!m_client != !!client) in JSC::Debugger::setClient. Single file, single function, no signature changes.
Security risks
None. This is inspector/debugger protocol lifecycle code, not reachable without an attached inspector frontend, and the change only narrows the reachable path (skips teardown when already torn down). No new data exposure, auth, or memory-safety surface.
Level of scrutiny
Low. The change is mechanical and follows the exact pattern already used at the two other internalDisable() call sites: enable() guards with if (enabled()) and willDestroyFrontendAndBackend() guards with if (enabled()) internalDisable(...). It also matches sibling agents — InspectorConsoleAgent::disable() returns { } when !m_enabled, InspectorHeapAgent::disable() returns an error string. enabled() reads m_enabled, which internalEnable() sets true and internalDisable() sets false, so the guard is exact.
Other factors
The bug-hunting pass surfaced one pre-existing issue in clearInspectorBreakpointState() (inverted == vs != VM comparison in the replacedInternalFunctions() cleanup lambda). It predates this PR (from the upstream merge at 0424321), lives in code this diff does not touch, and is flagged inline as informational for a follow-up. It does not affect the correctness of the 3-line guard being added here.
| if (!enabled()) | ||
| return { }; | ||
|
|
||
| internalDisable(false); |
There was a problem hiding this comment.
🟣 Not introduced by this PR, but noting since it lives in the exact teardown path being gated here (disable() → internalDisable() → clearInspectorBreakpointState()): the replacedInternalFunctions() cleanup lambda at ~line 2160 checks == &m_debugger.vm() where the three parallel sites (both lambdas in removeSymbolicBreakpoint() and the replacedThunks() lambda immediately above) all use !=. The inversion means this VM's own InternalFunction debugger hooks are never uninstalled on disable, and in a multi-VM process it decrements matchCount on other VMs' entries instead. Pre-existing (from 0424321); feel free to leave for a follow-up.
Extended reasoning...
What the bug is
In InspectorDebuggerAgent::clearInspectorBreakpointState() — which internalDisable() calls, and whose entry point (disable()) this PR now guards — the replacedInternalFunctions() cleanup lambda has an inverted VM-ownership check:
if (&replacedInternalFunction->internalFunction->vm() == &m_debugger.vm())
return false;Compare with the three parallel sites, all of which use !=:
removeSymbolicBreakpoint(),replacedThunks()lambda:!= &m_debugger.vm()removeSymbolicBreakpoint(),replacedInternalFunctions()lambda:!= &m_debugger.vm()clearInspectorBreakpointState(),replacedThunks()lambda (immediately above):!= &m_debugger.vm()clearInspectorBreakpointState(),replacedInternalFunctions()lambda:== &m_debugger.vm()← inverted
Code path
replacedInternalFunctions() is a process-global Vector<Box<ReplacedInternalFunction>> shared across all VMs. Each entry records an InternalFunction whose native call/construct pointers were swapped for internalFunctionCallWithDebuggerHook / internalFunctionConstructWithDebuggerHook. The intended removeAllMatching pattern is:
- If the weak ref is dead → remove (return
true). - If the entry belongs to a different VM → skip, leave it alone (return
false). - Otherwise (this VM's entry) → decrement
matchCountper matching symbolic breakpoint; remove when it reaches zero.
With ==, step 2 fires for this VM's entries — they are unconditionally kept — and step 3 runs against other VMs' entries.
Why nothing else prevents it
The only other removal path is removeSymbolicBreakpoint(), which is per-breakpoint. clearInspectorBreakpointState() is the bulk-teardown path used on Debugger.disable / frontend disconnect, and it is the only place that clears m_symbolicBreakpoints wholesale. Since ~ReplacedInternalFunction() is what restores the original native function pointers, keeping the Box alive means the hook is never uninstalled.
Impact
- Single-VM (Bun's common case): on
Debugger.disable, everyReplacedInternalFunctionentry for this VM is kept. The hookedInternalFunctions continue callinginternalFunctionWithDebuggerHookafter the debugger is disabled, and the entries leak in the process-global vector across enable/disable cycles. - Multi-VM process: additionally, entries owned by other VMs get their
matchCountdecremented (and possibly removed) using this agent'sm_symbolicBreakpoints, which can prematurely uninstall another VM's hooks.
Step-by-step proof
Debugger.enable, thenDebugger.addSymbolicBreakpointmatching e.g.Array.didCreateInternalFunction()swaps the native pointer and appends aBox<ReplacedInternalFunction>withmatchCount = 1to the global vector.Debugger.disable→internalDisable(false)→clearInspectorBreakpointState().- In the
replacedInternalFunctions().removeAllMatchinglambda: the weak ref is live, and&internalFunction->vm() == &m_debugger.vm()is true →return false(keep).matchCountis never decremented;~ReplacedInternalFunction()never runs; the hook stays installed. m_symbolicBreakpoints.clear()runs afterward, so a subsequent enable starts with an empty breakpoint list but a non-empty globalreplacedInternalFunctions()— the entry is now orphaned.
Fix
Change == to != at line ~2160 to match the other three sites.
Provenance
git blame attributes line 2160 to 0424321c96 (2026-07-16 upstream merge), predating this PR. The PR only adds the if (!enabled()) return { }; guard and does not touch this code, so this is pre-existing — flagged only because it sits directly in the teardown path whose entry point the PR is modifying.
Preview Builds
|
A second Debugger.disable on an already-disabled agent reaches internalDisable() and calls m_debugger.setClient(nullptr) again, which trips ASSERT(!!m_client != !!client) in JSC::Debugger::setClient on an asserts build. Frontends send Debugger.disable liberally on teardown (detach + close, or two attached tools), so this is an ordinary sequence. enable() already guards with if (enabled()); willDestroyFrontendAndBackend() guards internalDisable() the same way. InspectorHeapAgent::disable() and InspectorConsoleAgent::disable() both early-return when already disabled. Mirror that here.
3a8fe8a to
10e1ab9
Compare
There was a problem hiding this comment.
LGTM — trivial idempotency guard mirroring the existing pattern.
Checked that enabled() reads m_enabled (toggled only by internalEnable/internalDisable), so the guard is exact.
Same if (enabled()) gate already wraps internalDisable() in willDestroyFrontendAndBackend(), and InspectorConsoleAgent/InspectorHeapAgent::disable() follow the same shape.
Returning { } (success) rather than an error matches the console agent and keeps redundant frontend teardown quiet.
The pre-existing ==/!= inversion noted inline is unrelated to this diff.
Extended reasoning...
Overview
Adds a 3-line early-return guard to InspectorDebuggerAgent::disable() so a redundant Debugger.disable no-ops instead of re-entering internalDisable() and tripping ASSERT(!!m_client != !!client) in JSC::Debugger::setClient. One file, one function, no behavior change on the enabled → disabled transition.
Security risks
None. This is inspector/debug-tooling control flow, gated behind an already-attached inspector frontend. The guard strictly narrows the reachable code (skips teardown when already torn down); it introduces no new state, allocation, or external input handling.
Level of scrutiny
Low. The change is mechanical and mirrors three existing precedents in-tree: enable() in the same class guards on if (enabled()), willDestroyFrontendAndBackend() guards internalDisable() identically, and InspectorConsoleAgent::disable() / InspectorHeapAgent::disable() both early-return when already disabled. enabled() is virtual but the only override (WebDebuggerAgent in WebCore) is irrelevant to Bun's JSCOnly build, and in any case the same virtual call already gates internalDisable() on the frontend-destroy path.
Other factors
The bug-hunting pass found no issues in the diff itself. The inline note I left previously about the inverted VM check in clearInspectorBreakpointState() is explicitly pre-existing (from 0424321) and out of scope here. Returning success rather than makeUnexpected on a redundant disable is the friendlier choice for frontends that disable liberally on detach, and matches InspectorConsoleAgent. Preview builds succeeded.
…uilds
InspectorDebuggerAgent::disable() called internalDisable() unconditionally,
so a Debugger.disable on an already-disabled agent (or before any enable)
re-ran m_debugger.setClient(nullptr) and tripped ASSERT(!!m_client != !!client)
in JSC::Debugger::setClient on debug/ASAN builds. Release answers {} and
continues. Frontends send Debugger.disable liberally on teardown (detach +
close, or two attached tools), so this is an ordinary sequence.
oven-sh/WebKit#385 adds the same if (!enabled()) return { }; guard that
enable(), willDestroyFrontendAndBackend(), InspectorHeapAgent::disable()
and InspectorConsoleAgent::disable() already have.
1 participant
A second
Debugger.disableon an already-disabled agent reachesinternalDisable()and callsm_debugger.setClient(nullptr)again, which tripsASSERT(!!m_client != !!client)inJSC::Debugger::setClient(Debugger.cpp) on an asserts build. Release just answers{}and continues.enable()already guards withif (enabled());willDestroyFrontendAndBackend()guardsinternalDisable()the same way.InspectorHeapAgent::disable()andInspectorConsoleAgent::disable()both early-return when already disabled. This mirrors that here soDebugger.disableis idempotent like the other domains.Repro over Bun's
--inspectWebSocket transport (single client):Debugger.enable,Debugger.disable,Debugger.disable→ SIGABRT on asserts build. A bareDebugger.disablebefore anyenablehits the same assert (m_clientstarts null). Also reachable with two frontends where A enables, B enables (→ "Debugger domain already enabled"), B disables, A disables. Frontends sendDebugger.disableliberally on teardown (detach + close, or two attached tools each disabling), so this is an ordinary sequence rather than an exotic one.Branched off
e6e37cda21(current BunWEBKIT_VERSION) so the preview build is a minimal bump.