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

[robobun]

Problem

SourceProviderSourceType::BunTranspiledModule (added in #73) marks a SourceProvider that carries Bun's pre-computed module record so JSModuleLoader can build the JSModuleRecord without re-parsing. Four sites that branch on provider->sourceType() only match ::Module, so a BunTranspiledModule provider falls through:

Site	Fallthrough effect
debugger/DebuggerParseData.cpp gatherDebuggerParseDataForSource	returns false, Debugger::resolveBreakpoint gets empty pause positions, Debugger.setBreakpoint replies "Could not resolve breakpoint" for every location in the script, and Debugger.setBreakpointByUrl silently never resolves
inspector/agents/InspectorDebuggerAgent.cpp didParseSource	Debugger.scriptParsed sends module: false instead of true
runtime/CachedTypes.cpp CachedSourceProvider::encode / decode	RELEASE_ASSERT_NOT_REACHED() if JSC's bytecode cache ever encodes one of these providers
runtime/Completion.cpp getSourceType	returns ScriptFetchParameters::Type::None instead of JavaScript

Reachable in Bun today via bun test --isolate and bun build --compile standalone executables:

$ bun --inspect-wait=0 test --isolate x.test.ts  # + Debugger.enable over WebSocket
Debugger.scriptParsed: { url: ".../x.test.ts", module: false }
Debugger.setBreakpoint: { error: { message: "Could not resolve breakpoint" } }

The same file without --isolate (plain ::Module provider) reports module: true and the breakpoint resolves. oven-sh/bun#35605 would route every runtime ESM provider through this type and is blocked on this.

Fix

Add a USE(BUN_JSC_ADDITIONS)-gated case SourceProviderSourceType::BunTranspiledModule: at each site that falls through to the existing Module arm. These are the only four == ::Module / case ::Module: comparison sites on the enum; the remaining references construct providers of that type (jsc.cpp, JSScript.mm, WebCore) or already handle it (ModuleProgramExecutable.cpp, JSModuleLoader.cpp).

CachedStringSourceProvider::encode/decode under USE(BUN_JSC_ADDITIONS) only touch base-class SourceProvider members (source().length(), sourceOrigin(), sourceURL(), ...), so routing a non-StringSourceProvider subclass through the reinterpret_cast is as safe for BunTranspiledModule as it already is for Module (Bun's Zig::SourceProvider is a direct JSC::SourceProvider subclass either way).

The Bun-side regression test lands with the WEBKIT_VERSION bump.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5d96fd32-4f57-4ca0-87c2-9290f14db0a3

📥 Commits

Reviewing files that changed from the base of the PR and between b11589f and 64b7e43.

📒 Files selected for processing (4)

• Source/JavaScriptCore/debugger/DebuggerParseData.cpp
• Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp
• Source/JavaScriptCore/runtime/CachedTypes.cpp
• Source/JavaScriptCore/runtime/Completion.cpp

---

Walkthrough

Changes

Bun transpiled module support

Layer / File(s)	Summary
Runtime source-type routing Source/JavaScriptCore/runtime/CachedTypes.cpp, Source/JavaScriptCore/runtime/Completion.cpp	BunTranspiledModule uses existing cached string-provider encoding/decoding and maps to JavaScript completion handling.
Debugger and inspector module classification Source/JavaScriptCore/debugger/DebuggerParseData.cpp, Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp	Debugger parsing and inspector scriptParsed metadata recognize BunTranspiledModule as a module.

Suggested reviewers: constellation

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is detailed, but it does not follow the repository template or include the required Bugzilla link and commit-message-style fields.	Add the bug title, Bugzilla link, Reviewed by line, and the template's explanation and file sections, or confirm this repo accepts the current format.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the core change: treating BunTranspiledModule like Module in several JSC switches.
Linked Issues check	✅ Passed	The changes implement the linked issue's required JSC follow-up by handling BunTranspiledModule like Module in debugger, inspector, cache, and completion paths.
Out of Scope Changes check	✅ Passed	All shown code changes are directly tied to the BunTranspiledModule module-handling fix, with no unrelated edits visible in the summary.

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

LGTM — mechanical additions of a USE(BUN_JSC_ADDITIONS)-gated BunTranspiledModule fallthrough to the four existing Module switch arms. The inline finding is pre-existing (unrelated to this diff) and noted for follow-up, not a blocker.

Extended reasoning...

Overview

Adds case SourceProviderSourceType::BunTranspiledModule: (or the equivalent || clause) at four sites that branch on SourceProvider::sourceType() — gatherDebuggerParseDataForSource, InspectorDebuggerAgent::didParseSource, CachedSourceProvider::encode/decode, and getSourceType in Completion.cpp. Each new case falls through to the existing Module handling and is gated behind #if USE(BUN_JSC_ADDITIONS), matching the enum member's own gating in SourceProvider.h.

Security risks

None. No new inputs, allocations, or trust boundaries — the change only widens existing enum matches by one Bun-specific value. Upstream (non-Bun) builds compile identically.

Level of scrutiny

Low. These are one-line case labels routing a Bun-only enum value to the same code path Module already takes. I confirmed via grep that these four sites plus the already-handled ModuleProgramExecutable.cpp / JSModuleLoader.cpp are the complete set of comparison sites on this enum, matching the PR description's claim. For CachedTypes.cpp, I verified that under USE(BUN_JSC_ADDITIONS) CachedStringSourceProvider::encode only reads base-class SourceProvider members (source().length() plus Base::encode), so the reinterpret_cast is no less safe for BunTranspiledModule than it already is for Module with a Zig::SourceProvider.

Other factors

The one inline finding (inverted VM comparison in clearInspectorBreakpointState) is explicitly flagged as pre-existing — it lives ~300 lines away from the diff, in symbolic-breakpoint cleanup code this PR does not touch, and is not a regression introduced here. It should be filed/fixed separately and does not affect approval of this change.

[claude]

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

[github-actions]

Preview Builds

Commit	Release	Date
64b7e437	autobuild-preview-pr-345-64b7e437	2026-07-25 15:17:28 UTC