Treat SourceProviderSourceType::BunTranspiledModule as Module in debugger/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]

Walkthrough

The changes add conditional BunTranspiledModule handling to JavaScriptCore debugger parsing, source-provider caching, and completion source-type mapping.

Changes

Bun transpiled module support

Layer / File(s)	Summary
Runtime source-type routing Source/JavaScriptCore/runtime/CachedTypes.cpp, Source/JavaScriptCore/runtime/Completion.cpp	Source-provider encoding and decoding use the cached string-provider path. Completion maps BunTranspiledModule to JavaScript.
Debugger parse-data routing Source/JavaScriptCore/debugger/DebuggerParseData.cpp	Debugger parse-data gathering routes BunTranspiledModule through the module path.

Suggested reviewers: geoffreygaren, constellation

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description explains the problem and fix but omits the required bug title, Bugzilla URL, reviewer line, and changed-file/function list.	Add the required Bugzilla link, reviewer line, bug explanation in the template format, and a changed-file/function list.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes satisfy the WebKit compatibility requirements for issue #35605 by updating the three remaining Module-only switch sites; inspector handling already exists upstream.
Out of Scope Changes check	✅ Passed	All changes are limited to BunTranspiledModule handling in debugger parsing, source-provider caching, and completion, which directly supports the linked issue.
Title check	✅ Passed	The title clearly identifies the main change: treating BunTranspiledModule like Module in debugger, cache, and completion switches.

---

_{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
b7c69395	autobuild-preview-pr-345-b7c69395	2026-08-10 20:38:46 UTC
64b7e437	autobuild-preview-pr-345-64b7e437	2026-07-25 15:17:28 UTC

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[robobun]

Rebased onto current main (447082a) at b7c6939. The InspectorDebuggerAgent::didParseSource hunk is dropped: since the upstream merge in #383, scriptTypeForScript already maps BunTranspiledModule to ScriptType::Module. The remaining three sites (DebuggerParseData.cpp, CachedTypes.cpp encode/decode, Completion.cpp) are unchanged and are the only ::Module-only comparison sites left on main (git grep 'SourceProviderSourceType::Module\b').

oven-sh/bun#35605 (which turns BunTranspiledModule on for every runtime ESM module) is waiting on this; once it merges and autobuild-<sha> exists, that PR gets repinned to it.

[claude]

LGTM — mechanical addition of USE(BUN_JSC_ADDITIONS)-gated BunTranspiledModule case labels that fall through to the existing Module arms.

What was reviewed:

• Confirmed the three switch sites are the only remaining ::Module-only comparisons on main; scriptTypeForScript already handles it, so the dropped inspector hunk is correct.
• Checked CachedStringSourceProvider::encode/decode under BUN_JSC_ADDITIONS — only base-class SourceProvider members are touched, so the reinterpret_cast is no less safe for BunTranspiledModule than it already is for Module.
• The pre-existing clearInspectorBreakpointState issue noted earlier is unrelated to this diff and no longer in a touched file.

Extended reasoning...

Overview

This PR adds a case SourceProviderSourceType::BunTranspiledModule: label (guarded by #if USE(BUN_JSC_ADDITIONS)) at three switch sites so the case falls through to the existing SourceProviderSourceType::Module handling:

• debugger/DebuggerParseData.cpp — gatherDebuggerParseDataForSource now parses these providers as modules instead of returning false.
• runtime/CachedTypes.cpp — CachedSourceProvider::encode/decode route through CachedStringSourceProvider instead of hitting RELEASE_ASSERT_NOT_REACHED().
• runtime/Completion.cpp — getSourceType returns ScriptFetchParameters::Type::JavaScript instead of None.

After the rebase onto 447082a, the original fourth hunk in InspectorDebuggerAgent.cpp was dropped because scriptTypeForScript on main already maps BunTranspiledModule to ScriptType::Module — verified in the current tree.

Security risks

None. No new inputs, no parsing changes, no auth/crypto/permissions surface. The only effect is that an existing Bun-specific enum value takes the same code path as Module instead of the default/assert fallthrough. All additions are behind USE(BUN_JSC_ADDITIONS), so upstream/non-Bun builds are byte-identical.

Level of scrutiny

Low. This is a 12-line diff consisting purely of additional case labels with no new logic. The one non-trivial concern is the reinterpret_cast<const StringSourceProvider&> in CachedSourceProvider::encode: I checked CachedStringSourceProvider::encode and CachedSourceProviderShape::encode under USE(BUN_JSC_ADDITIONS) — they only invoke base-class SourceProvider methods (sourceOrigin(), sourceURL(), source().length(), etc.), so passing a non-StringSourceProvider here is exactly as safe as it already is for Module providers (Bun's Zig::SourceProvider is a direct SourceProvider subclass in either case). decode under Bun returns SourceProvider* and prefers the runtime-supplied provider, so no new subclass assumptions are introduced there either.

Other factors

• A grep for SourceProviderSourceType::Module\b confirms the PR description: the remaining references are constructors (jsc.cpp, JSScript.mm) or sites that already handle BunTranspiledModule (ModuleProgramExecutable.cpp, InspectorDebuggerAgent.cpp).
• The bug hunter found no issues in this diff. My earlier inline comment on this PR flagged a pre-existing inverted VM check in InspectorDebuggerAgent::clearInspectorBreakpointState; that file is no longer touched by the rebased diff and the issue is orthogonal to this change, so it does not block approval.
• Regression coverage lands on the Bun side with the WEBKIT_VERSION bump per the description.

[alii]

Superseded by #405 (this change rebased, plus the CachedStringSourceProvider::encode signature fix from review). Keeping this open until oven-sh/bun#35605 stops pinning autobuild-preview-pr-345-b7c69395.

[alii]

Landed as #405 (723cea6).