Skip to content

InspectorDebuggerAgent: make Debugger.disable idempotent #385

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

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

InspectorDebuggerAgent: make Debugger.disable idempotent #385

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,9 @@ Protocol::ErrorStringOr<void> InspectorDebuggerAgent::enable()

Protocol::ErrorStringOr<void> InspectorDebuggerAgent::disable()
{
if (!enabled())
return { };

internalDisable(false);

@claude claude Bot Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟣 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:

  1. If the weak ref is dead → remove (return true).
  2. If the entry belongs to a different VM → skip, leave it alone (return false).
  3. Otherwise (this VM's entry) → decrement matchCount per 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, every ReplacedInternalFunction entry for this VM is kept. The hooked InternalFunctions continue calling internalFunctionWithDebuggerHook after 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 matchCount decremented (and possibly removed) using this agent's m_symbolicBreakpoints, which can prematurely uninstall another VM's hooks.

Step-by-step proof

  1. Debugger.enable, then Debugger.addSymbolicBreakpoint matching e.g. Array. didCreateInternalFunction() swaps the native pointer and appends a Box<ReplacedInternalFunction> with matchCount = 1 to the global vector.
  2. Debugger.disable → internalDisable(false) → clearInspectorBreakpointState().
  3. In the replacedInternalFunctions().removeAllMatching lambda: the weak ref is live, and &internalFunction->vm() == &m_debugger.vm() is true → return false (keep). matchCount is never decremented; ~ReplacedInternalFunction() never runs; the hook stays installed.
  4. m_symbolicBreakpoints.clear() runs afterward, so a subsequent enable starts with an empty breakpoint list but a non-empty global replacedInternalFunctions() — 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.


return { };
Expand Down
Loading