Skip to content

Treat SourceProviderSourceType::BunTranspiledModule as Module in debugger/inspector/cache/completion switches #345

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

Treat SourceProviderSourceType::BunTranspiledModule as Module in debugger/inspector/cache/completion switches #345

Show file tree
Hide file tree
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/debugger/DebuggerParseData.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,9 @@ bool gatherDebuggerParseDataForSource(VM& vm, SourceProvider* provider, Debugger
case SourceProviderSourceType::Program:
return gatherDebuggerParseData<Program>(vm, completeSource, debuggerParseData);
case SourceProviderSourceType::Module:
#if USE(BUN_JSC_ADDITIONS)
case SourceProviderSourceType::BunTranspiledModule:
#endif
return gatherDebuggerParseData<Module>(vm, completeSource, debuggerParseData);
default:
return false;
Expand Down
8 changes: 7 additions & 1 deletion Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1822,7 +1822,13 @@
String sourceURL = script.sourceURL;
String sourceMappingURL = sourceMapURLForScript(script);

m_frontendDispatcher->scriptParsed(scriptIDStr, script.url, script.startLine, script.startColumn, script.endLine, script.endColumn, script.isContentScript, sourceURL, sourceMappingURL, script.sourceProvider->sourceType() == JSC::SourceProviderSourceType::Module);
const auto providerSourceType = script.sourceProvider->sourceType();

Check notice on line 1825 in Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp

View check run for this annotation

Claude / Claude Code Review

[pre-existing] Inverted VM comparison in clearInspectorBreakpointState leaks InternalFunction debugger hooks 

**Pre-existing** (not touched by this PR, noticed while reading the file): in `clearInspectorBreakpointState()` the `replacedInternalFunctions().removeAllMatching` lambda checks `== &m_debugger.vm()` where the parallel `replacedThunks()` block just above and both blocks in `removeSymbolicBreakpoint()` use `!=`. The inversion means this-VM entries are never removed on `Debugger.disable` (the InternalFunction debugger hooks stay installed and the `Box<ReplacedInternalFunction>` entries leak), and

@claude claude Bot Jul 25, 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.

🟣 Pre-existing (not touched by this PR, noticed while reading the file): in clearInspectorBreakpointState() the replacedInternalFunctions().removeAllMatching lambda checks == &m_debugger.vm() where the parallel replacedThunks() block just above and both blocks in removeSymbolicBreakpoint() use !=. The inversion means this-VM entries are never removed on Debugger.disable (the InternalFunction debugger hooks stay installed and the Box<ReplacedInternalFunction> entries leak), and in a multi-VM process other-VM entries get their matchCount decremented by this agent's symbolic breakpoints. The actual line is ~2127; anchoring here because it's outside the diff.

Extended reasoning...

What the bug is

In InspectorDebuggerAgent::clearInspectorBreakpointState() (called from internalDisable() / willDestroyFrontendAndBackend()), the cleanup lambda for replacedInternalFunctions() has an inverted VM guard:

if (&replacedInternalFunction->internalFunction->vm() == &m_debugger.vm())
    return false;

Compare to the immediately preceding replacedThunks() block in the same function, and to both blocks in removeSymbolicBreakpoint(), which all use !=:

if (&replacedThunk->nativeExecutable->vm() != &m_debugger.vm())
    return false;

replacedInternalFunctions() is a process-global NeverDestroyed<Vector<Box<ReplacedInternalFunction>>> shared across all VMs/agents (guarded by s_replacedInternalFunctionsLock). The intent of the guard is: skip entries belonging to other VMs; only decrement/remove entries owned by this agent's VM. The == inverts that.

Step-by-step proof

Single-VM (typical Bun process):

  1. User connects an inspector and calls Debugger.addSymbolicBreakpoint with e.g. symbol: "fetch".
  2. addSymbolicBreakpoint() walks the heap, finds matching InternalFunctions, and didCreateInternalFunction() swaps their native function pointers to internalFunction{Call,Construct}WithDebuggerHook and appends a Box<ReplacedInternalFunction> to the global vector.
  3. User disconnects → willDestroyFrontendAndBackend() → internalDisable() → clearInspectorBreakpointState().
  4. The removeAllMatching lambda runs on each entry. For every entry, &entry->internalFunction->vm() == &m_debugger.vm() is true (there is only one VM), so the lambda returns false and the entry is kept.
  5. ~ReplacedInternalFunction() never runs, so the InternalFunction's native function pointers are never restored — every subsequent call still routes through internalFunctionWithDebuggerHook, which takes s_replacedInternalFunctionsLock, does a linear find(), and calls vm.forEachDebugger(...) before dispatching. The Box entries also leak for the process lifetime.

Multi-VM process:

  1. VM A's agent adds a symbolic breakpoint on "foo"; VM B's agent adds one on "foo" too. The global vector has entries for both VMs.
  2. VM A's agent is disabled → clearInspectorBreakpointState().
  3. For VM A's own entries, == &m_debugger.vm() is true → return false → kept (leak, as above).
  4. For VM B's entries, == &m_debugger.vm() is false → falls through to the m_symbolicBreakpoints loop. VM A's "foo" breakpoint matches, so VM B's entry has its matchCount decremented, potentially hitting 0 and being removed — restoring VM B's InternalFunction to its original pointer while VM B's debugger is still active and expects the hook.

Why nothing else prevents it

The only other place these entries are removed is removeSymbolicBreakpoint(), which is only called when the frontend explicitly issues Debugger.removeSymbolicBreakpoint. On disconnect / Debugger.disable, only clearInspectorBreakpointState() runs, and m_symbolicBreakpoints.clear() happens after the buggy block, so this is the sole opportunity to restore the hooks.

Fix

Flip == to != to match the three parallel sites:

if (&replacedInternalFunction->internalFunction->vm() != &m_debugger.vm())
    return false;

Relation to this PR

None. This PR only touches didParseSource() in this file to add a BunTranspiledModule case; it does not touch clearInspectorBreakpointState(), symbolic breakpoints, or the InternalFunction hook machinery. Flagged as pre-existing since we happened to be in this file.

const bool isModule = providerSourceType == JSC::SourceProviderSourceType::Module
#if USE(BUN_JSC_ADDITIONS)
|| providerSourceType == JSC::SourceProviderSourceType::BunTranspiledModule
#endif
;
m_frontendDispatcher->scriptParsed(scriptIDStr, script.url, script.startLine, script.startColumn, script.endLine, script.endColumn, script.isContentScript, sourceURL, sourceMappingURL, isModule);

m_scripts.set(sourceID, script);

Expand Down
6 changes: 6 additions & 0 deletions Source/JavaScriptCore/runtime/CachedTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1744,6 +1744,9 @@ class CachedSourceProvider : public VariableLengthObject<SourceProvider> {
switch (m_sourceType) {
case SourceProviderSourceType::Program:
case SourceProviderSourceType::Module:
#if USE(BUN_JSC_ADDITIONS)
case SourceProviderSourceType::BunTranspiledModule:
#endif
this->allocate<CachedStringSourceProvider>(encoder)->encode(encoder, reinterpret_cast<const StringSourceProvider&>(sourceProvider));
break;
#if ENABLE(WEBASSEMBLY)
Expand All @@ -1761,6 +1764,9 @@ class CachedSourceProvider : public VariableLengthObject<SourceProvider> {
switch (m_sourceType) {
case SourceProviderSourceType::Program:
case SourceProviderSourceType::Module:
#if USE(BUN_JSC_ADDITIONS)
case SourceProviderSourceType::BunTranspiledModule:
#endif
return this->buffer<CachedStringSourceProvider>()->decode(decoder, m_sourceType);
#if ENABLE(WEBASSEMBLY)
case SourceProviderSourceType::WebAssembly:
Expand Down
3 changes: 3 additions & 0 deletions Source/JavaScriptCore/runtime/Completion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,9 @@ static ScriptFetchParameters::Type getSourceType(const SourceCode& source)
case SourceProviderSourceType::WebAssembly:
return ScriptFetchParameters::Type::WebAssembly;
case SourceProviderSourceType::Module:
#if USE(BUN_JSC_ADDITIONS)
case SourceProviderSourceType::BunTranspiledModule:
#endif
return ScriptFetchParameters::Type::JavaScript;
default:
return ScriptFetchParameters::Type::None;
Expand Down
Loading