jsc: free owned FFI allocations when the post-call trap check returns Err

[robobun]

What

call_check_slow / from_js_host_call_generic run the FFI closure, then check for a pending exception (including VMTraps). If a worker's terminate() sets the NeedTermination trap in the window between the C++ function's own RETURN_IF_EXCEPTION and the Rust post-call check, the wrapper returns Err and the closure's return value is dropped. When that value is a raw heap pointer (or a +1'd BunString, which is Copy with no Drop), the drop is a no-op and the allocation leaks.

Same class of bug #34574 fixed for JSPropertyIterator, without the Ref<VM> cascade: here the leak is just the one allocation per occurrence, so ~VM still runs.

Cause

A sweep of every call_check_slow / from_js_host_call_generic call site turned up six whose closure returns an owned allocation from C++:

wrapper	returns	C++ alloc	destructor
URL::from_js	*mut URL	new WTF::URL (BunString.cpp)	URL__deinit
URL::href_from_js	BunString +1	toStringRef (BunString.cpp)	String::deref
FetchHeaders::create_from_js	*mut FetchHeaders	new WebCore::FetchHeaders (bindings.cpp:1813)	WebCore__FetchHeaders__deref
FetchHeaders::clone_this	*mut FetchHeaders	new WebCore::FetchHeaders (bindings.cpp:1860)	WebCore__FetchHeaders__deref
JSBigInt::to_string	BunString +1	toStringRef (JSBigIntBinding.cpp)	String::deref
JSValue::serialize	SerializedScriptValue*	leakRef() (Serialization.cpp)	Bun__SerializedScriptSlice__free

For FetchHeaders::create_from_js the window is wider than just the return instruction: the final C++ guard is a bare throwScope.exception() (no trap handling), so a trap set during headers->fill() is not observed until the Rust side checks, and fill() iterates every header entry.

Fix

Adds call_check_slow_owned(global, f, free) in TopExceptionScope.rs (with a from_js_host_call_owned alias in host_fn.rs): same shape as call_check_slow, but free runs on the closure's return before Err is propagated. The hazard is documented once on the helper and pointed at from call_check_slow_at's doc comment. All six sites become a single call with the matching destructor as the free closure.

Verification

New ASAN-only test terminate() during an allocating FFI wrapper does not leak the allocation in worker_threads.test.ts: a worker tight-loops new Response("", {headers: {200 entries}}) (drives FetchHeaders::create_from_js) while the parent terminate()s it, under Malloc=1 detect_leaks=1. 4 processes x 42 workers each; the parent fixture prints done once every chain finishes and the test asserts on that plus a null signalCode.

LSan fail-before stack (3/3 runs):

Direct leak of 336 byte(s) in 3 object(s) allocated from:
    #10 in WebCore::FetchHeaders::operator new(unsigned long) .../FetchHeaders.h:44:5
    #11 in WebCore__FetchHeaders__createFromJS .../bindings.cpp:1813:21
    #16 in <bun_jsc::fetch_headers::FetchHeaders>::create_from_js .../FetchHeaders.rs:147:9
    #17 in <bun_runtime::webcore::response::HeadersRef>::create_from_js .../Response.rs:90:12
    #18 in <bun_runtime::webcore::response::Init>::init .../Response.rs:1412:34
    #19 in <bun_runtime::webcore::response::Response>::constructor .../Response.rs:1239:28

Pass-after 3/3. worker_threads.test.ts 92 pass, headers.test.ts 99 pass, response.test.ts 23 pass.

Malloc=1 also surfaces pre-existing per-thread singletons (WebCore::eventNames, Bun.main's AtomString) that are normally hidden by bmalloc, so the test assertion is scoped to the six C++ function names above rather than a blanket LSan check.

A couple of sites use an &mut BunString out-param instead of a closure return (JSValue::get_name, jsonStringify); those have the same hazard at the caller and are left for a follow-up since the fix shape is different.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/worker_threads/worker_threads.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 6 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ffd23ed4-caf5-48d1-ba52-ae3e8c49a33f

📥 Commits

Reviewing files that changed from the base of the PR and between 6843b4c and 090ff8e.

📒 Files selected for processing (10)

• src/jsc/FetchHeaders.rs
• src/jsc/JSBigInt.rs
• src/jsc/JSValue.rs
• src/jsc/TopExceptionScope.rs
• src/jsc/URL.rs
• src/jsc/host_fn.rs
• src/jsc/lib.rs
• test/js/node/worker_threads/worker-terminate-ffi-alloc-parent-fixture.js
• test/js/node/worker_threads/worker-terminate-ffi-alloc-worker-fixture.js
• test/js/node/worker_threads/worker_threads.test.ts

---

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

[robobun]

^{Updated 4:36 AM PT - Jul 18th, 2026}

❌ @robobun, your commit 090ff8e has 1 failures in Build #75287 (All Failures):

• test/cli/run/no-orphans.test.ts - code 1 on 🍎 26 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 34577

That installs a local version of the PR into your bun-34577 executable, so you can run:

bun-34577 --bun

[github-actions]

Found 1 issue this PR may fix:

1. macOS Apple Silicon: memory invisible to RSS — bmalloc slabs, worker cleanup gaps, GC safety bugs #28318 - Umbrella issue tracking worker cleanup gaps and memory invisible to RSS; this PR fixes 6 FFI call sites that leak C++ heap allocations (FetchHeaders, URL, BunString, SerializedScriptValue) when worker termination fires mid-call

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #28318

🤖 Generated with Claude Code

[claude]

Beyond the inline nit, I checked that String::deref() is a no-op for the Dead tag (src/bun_core/string/mod.rs:539), so out.deref() in href_from_js/JSBigInt::to_string is safe when C++ threw before returning a +1 impl; and that each of the six destructor pairings (WebCore__FetchHeaders__deref, URL__deinit, Bun__SerializedScriptSlice__free, String::deref) matches the allocation named in the PR table, with null/DEAD guards on every Err path so nothing double-frees when C++ returned no allocation.

Extended reasoning...

Verified the Err-path cleanup at each of the six sites cannot free an allocation that was never made: the raw-pointer sites guard on non-null / NonNull::new, and the two BunString sites rely on String::deref() gating on tag == WTFStringImpl (checked in bun_core), so DEAD is a no-op. Success paths are byte-for-byte behavior-preserving. Leaving unapproved because this is FFI memory-ownership code across six JSC wrappers — worth a maintainer glance even though the pattern is mechanical.

[claude]

No issues found after the two fixture fixes — the failed gate in 090ff8e closes the last vacuous-pass path. Deferring to a human because this adds a new ownership primitive (call_check_slow_owned) to the exception-scope machinery and rewires six FFI sites' error-path destructors; worth a maintainer's eyes on the free-closure pairings.

What was reviewed:

• call_check_slow_owned: closure runs exactly once on both cfg paths in call_check_slow_at, so slot.expect(..) is sound; free(r) only on Err.
• Each of the six free closures null-guards before calling the destructor; BunString::deref() is a no-op on Dead/Empty tags, so the two string sites are safe when C++ returned a Dead-tagged value.
• Test fixture now gates done on !failed, so a missing worker fixture or worker error fails the stdout: "done" assertion.

Extended reasoning...

Overview

Adds call_check_slow_owned (and its from_js_host_call_owned alias) to TopExceptionScope.rs / host_fn.rs: same shape as call_check_slow, but takes a free: impl FnOnce(R) that runs on the closure's return before propagating Err. Six call sites whose C++ callee returns an owned heap allocation (URL::from_js, URL::href_from_js, FetchHeaders::create_from_js, FetchHeaders::clone_this, JSBigInt::to_string, JSValue::serialize) are converted to the new helper with matching destructor closures. An ASAN-only LSan test in worker_threads.test.ts drives FetchHeaders::create_from_js in a tight loop while the parent terminate()s the worker, and asserts none of the six symbol names appear in the leak report.

Security risks

None identified. This is a leak fix on the worker-termination path; no user-controlled data reaches a new sink, no auth/crypto/permissions code is touched.

Level of scrutiny

High. This is native memory-safety code at the Rust/C++ FFI boundary — the repo's most-blocked review category. The change introduces a new primitive that every future "owned-return FFI wrapper" will use, and each of the six converted sites pairs a C++ allocator with a specific destructor. A wrong pairing or a free on an already-freed value would be a use-after-free reachable from worker termination. I checked each pairing:

• FetchHeaders (both sites): NonNull::new → if let Some(mut p) { p.as_mut().deref() } — matches WebCore__FetchHeaders__deref; null-guarded.
• URL::from_js: raw *mut → if !p.is_null() { URL__deinit(p) } — null-guarded.
• URL::href_from_js / JSBigInt::to_string: BunString → s.deref(). Verified bun_core::String::deref() only acts on Tag::WTFStringImpl (src/bun_core/string/mod.rs:539), so a Dead-tagged return (which URL__getHrefFromJS produces on failure per its doc comment) is a no-op — no double-free.
• JSValue::serialize: SerializedScriptValueExternal → if !ext.handle.is_null() { Bun__SerializedScriptSlice__free(ext.handle) } — null-guarded, matches the existing Drop for SerializedScriptValue.

The helper itself smuggles R out via Option<R> and matches on call_check_slow_at's Result<()>; both cfg branches of call_check_slow_at run f() exactly once before the exception check, so the .expect(..) cannot fail.

Other factors

Two prior inline concerns from me (vacuous-pass test, then the residual missing-fixture gap) were both addressed — the parent fixture now prints done gated on !failed, and the test asserts {stdout: "done", signalCode: null}. The PR description documents a demonstrated 3/3 fail-before / 3/3 pass-after with the LSan stack. The test is ASAN-only and probabilistic (race-window based), which is unavoidable for this bug class but means non-ASAN CI won't exercise it.

I'm not approving because this adds a reusable ownership primitive to the JSC exception-scope layer and touches six FFI destructor pairings — a maintainer should sign off on the helper's shape and confirm the sweep didn't miss sibling sites (the PR description notes &mut BunString out-param sites are deferred to a follow-up).

[robobun]

CI builds #75210 and #75287 both show only pre-existing or flaky-retry failures (no-orphans.test.ts on macOS, various install/websocket/terminal flakes). None touch worker_threads.test.ts or any of the files in this diff; the new LSan test passed on the asan lane.

The diff is green. Ready for review.