bun:ffi: don't re-enter JS from a JSCallback while an exception is pending - #32780
bun:ffi: don't re-enter JS from a JSCallback while an exception is pending#32780robobun wants to merge 1 commit into
Conversation
|
Updated 9:08 AM PT - Jun 28th, 2026
❌ @robobun, your commit 6d754ea has some failures in 🧪 To try this PR locally: bunx bun-pr 32780That installs a local version of the PR into your bun-32780 --bun |
WalkthroughCentralizes FFI callback invocation in ChangesFFI callback exception flow
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/ffi/ffi.test.js`:
- Around line 993-997: Condense the explanatory comment near the JSCallback/FFI
test so it fits the repo’s 3-line comment limit while preserving the invariant.
Keep the essential points in the test around JSCallback and the enclosing FFI
call site: a thrown callback error must not remain pending in the VM, the error
should surface at the outer FFI call, and the callback must not be re-entered
during that native call.
- Around line 1029-1033: The subprocess test in ffi.test.js drains stderr but
never checks it, so unexpected fixture output can slip through; update the
qsort/FFI subprocess assertion to use the existing combined-object style by
capturing stdout, stderr, and exitCode together and asserting stderr explicitly
alongside the current stdout and exitCode checks. Keep the pattern consistent
with the surrounding bunEnv-based subprocess tests and the existing combined
result assertions in this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5923a2dd-8d95-4b55-9c65-2dc6a8b90ad6
📒 Files selected for processing (2)
src/jsc/bindings/JSFFIFunction.cpptest/js/bun/ffi/ffi.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/bun/ffi/ffi.test.js:1029-1033— nit:stderris read but never appears in any assertion, so when this test fails on an unfixed debug build the JSCASSERTION FAILED: !exception()abort message is silently dropped and the failure shows only an empty-stdout mismatch. Considerexpect({ stdout, stderr, exitCode }).toMatchObject({ stdout: "caught: cb-throw\ncalls: 1\ndone\n", exitCode: 0 })— same pattern ascc.test.ts:453— which surfaces stderr in the failure diff without asserting it empty (debug/ASAN noise).Extended reasoning...
What this is
The new subprocess test pipes
stderr, drains it concurrently into a local variable, and then never references that variable in any assertion:const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); expect(stdout).toBe("caught: cb-throw\ncalls: 1\ndone\n"); expect(exitCode).toBe(0);
The repo's CLAUDE.md (Tests reviewers reject → Subprocess tests, line 195) says: "Never assert stderr is exactly empty (ASAN/debug builds emit benign warnings); assert a combined
{ stdout, stderr, exitCode }object." The neighboringtest/js/bun/ffi/cc.test.ts:449-456shows the intended pattern:// stderr is included in the received object so failures show it, but is not // asserted empty: debug builds emit benign startup warnings. expect({ stdout: normalizeBunSnapshot(stdout), stderr, exitCode }).toMatchObject({ stdout: "OK 100", exitCode: 0, });
toMatchObjectonly checks the keys present in the expected object, sostderris part of the received value (and thus printed in the failure diff) without being constrained to"".Why it matters specifically for this test
This test's entire purpose is to catch a debug-build JSC assertion abort. Per the PR description, on unfixed
bun-debugthe child dies with:ASSERTION FAILED: Unexpected exception observed on thread ... !exception() JavaScriptCore/ExceptionScope.h(61) : void JSC::ExceptionScope::assertNoException()That diagnostic goes to stderr. With the current assertions, a regression produces only:
Expected: "caught: cb-throw\ncalls: 1\ndone\n" Received: ""…plus an exit-code mismatch — the actual reason (the JSC assertion text) is read into a dead variable and discarded. Whoever debugs the regression has to re-run the child manually to see what happened. Including
stderrin the received object puts the abort message directly in the test-failure diff.Step-by-step proof
- Revert
invokeFFICallback(or run on a build without this PR) and run the test underbun-debug. - Child process: first
qsortcomparison enters JS, throws, leaves the exception pending; second comparison hitsInterpreter::executeCallImpl→scope.assertNoException()→ abort. The abort banner is written to fd 2. - Parent:
proc.stdout.text()resolves to""(nothing was logged before the abort),proc.stderr.text()resolves to the multi-lineASSERTION FAILEDtext,proc.exitedresolves to a non-zero signal/exit code. expect(stdout).toBe(...)fails first; Jest's diff shows received""vs the expected three lines.stderris in scope but unreferenced, so its contents never appear in the test output.- With
expect({ stdout, stderr, exitCode }).toMatchObject({ stdout: ..., exitCode: 0 }), the same failure prints the full received object — including theASSERTION FAILED: !exception()text — in the diff.
Addressing the counter-argument
It's true that CLAUDE.md's own canonical subprocess-test example (lines 86-96) also reads
stderrwithout asserting on it, and that the pipe-draining requirement is satisfied here — so the test is not wrong. There is genuine tension between the example and the prose rule on line 195. Two things tip this toward worth-mentioning:- The "obvious fix" the counter-argument warns against (
expect(stderr).toBe("")) is not what's being suggested.toMatchObjectwithstderronly on the received side adds zero flake risk on ASAN/debug builds — it never compares stderr to anything. The siblingcc.test.tsin the same directory already does exactly this, with a comment explaining why. - The diagnostic gap is unusually concrete for this test: unlike most subprocess tests, its designed-for failure mode writes the only useful signal to stderr. That's precisely the case the combined-object guidance exists for.
Fix
Replace the two trailing assertions with:
expect({ stdout, stderr, exitCode }).toMatchObject({ stdout: "caught: cb-throw\ncalls: 1\ndone\n", exitCode: 0, });
This is a test-quality nit, not a functional bug — the test still correctly passes/fails on the right builds either way.
- Revert
|
Done in 7fce146: switched to the |
|
Final CI status for build 66372 (rebased commit Same agent (
I already re-rolled once and it landed on the same box, so I will stop pushing. A manual retry of the one |
There was a problem hiding this comment.
I didn't find any issues, but this changes ThrowScope placement and re-entry semantics at the JSC/FFI boundary, so it's worth a human glance before merging.
Extended reasoning...
Overview
This PR touches two files: src/jsc/bindings/JSFFIFunction.cpp and test/js/bun/ffi/ffi.test.js. The C++ change consolidates nine near-identical FFI_Callback_call* entry points into a shared invokeFFICallback helper, and adds an early-return guard that skips JS re-entry when scope.exception() is already set. The test adds a subprocess-based regression that drives qsort with a throwing comparator and asserts the error surfaces at the FFI call site with the callback body entered exactly once.
Security risks
None identified. This is exception-flow plumbing inside the FFI callback trampoline; it doesn't touch auth, permissions, parsing, or any external input surface. The new test only dlopens well-known system C libraries already used elsewhere in this test file.
Level of scrutiny
Moderate-to-high. While the diff is mostly deduplication (net -100 lines), the substantive change is to JSC exception-scope handling at the native↔JS re-entry boundary: DECLARE_THROW_SCOPE is now hoisted above profiledCall (previously declared only inside the post-call if (exception) branch), and a new pending-exception guard short-circuits the call. The reasoning in the PR description is sound and matches JSC's assertNoException() contract, but ThrowScope lifetime/placement in JSC bindings is subtle enough that someone familiar with the exception-check validator should confirm the hoisted scope doesn't introduce validator noise on other paths.
Other factors
The bug-hunting system found nothing, CodeRabbit's two style nits were addressed, and the test is well-constructed with cross-platform libc discovery. CI shows musl build failures and a node-tls-connect test failure that appear unrelated to this change. The author also flags an overlap with #32778 touching the same functions. Given the critical-path nature of JSC bindings rather than any specific concern with the implementation, I'm deferring rather than approving.
…32792) ### What Calling `worker.terminate()` while the worker thread is inside a `bun:ffi` `JSCallback` makes the FFI callback glue re-throw JSC's own TerminationException. Assertion-enabled builds abort on the worker thread: ``` ASSERTION FAILED: !isTerminationException(exception) || hasTerminationRequest() vendor/WebKit/Source/JavaScriptCore/runtime/VM.cpp(1072) : void JSC::VM::setException(Exception *) ``` Reproduces 5/5 with a debug build (no native library needed, `CFunction` over `cb.ptr` is enough): ```js // worker.js: a threadsafe JSCallback that signals the parent, then spins const callback = new JSCallback(() => { Atomics.store(flag, 0, 1); while (true) {} }, { returns: "void", args: [], threadsafe: true }); const fire = new CFunction({ ptr: callback.ptr, returns: "void", args: [] }); fire(); // main.js: wait for flag[0], then worker.terminate() ``` The racier sibling of the same bug shows up as `ASSERTION FAILED: !exception()` in `ExceptionScope.h(61)` when the next queued callback task enters `Interpreter::executeCallImpl` with the re-thrown exception still pending. ### Cause Every `FFI_Callback_*` entry point in `src/jsc/bindings/JSFFIFunction.cpp` called the JS function through the `NakedPtr<Exception>` overload of `profiledCall` (which clears the VM's exception) and then unconditionally re-threw it with `scope.throwException()`. A threadsafe `JSCallback` runs from a posted event-loop task, so its `profiledCall` is the outermost VM entry. When `worker.terminate()` lands inside that callback, JSC throws the TerminationException, and the outermost `VMEntryScope` teardown retires the termination request (`VM::clearHasTerminationRequest`). Re-installing the engine's own TerminationException after that violates `VM::setException`'s precondition, and in a no-assert build it leaves a half-terminated worker VM with a stale pending TerminationException that the next JS entry trips over. ### Fix The clear-and-rethrow round trip was an inlined, incorrect re-implementation of "leave the exception pending". Do that directly: all ten `FFI_Callback_*` entry points now share one `invokeFFICallback()` helper that uses the plain `profiledCall` overload and `RETURN_IF_EXCEPTION`, leaving whatever JSC threw (user exception or TerminationException) pending on the VM like any other host function. - Nested case (JS -> native -> callback): unchanged, the pending exception still surfaces at the enclosing FFI call site. - Top-of-event-loop case (threadsafe task): the task dispatcher already recognizes a pending TerminationException and stops the tick loop, which is the path every other task-based JS entry uses. ### Verification Two tests in `test/js/bun/ffi/ffi.test.js`: - `JSCallback tolerates worker.terminate() arriving inside the callback`: spawns the repro above as a child. Unfixed `bun-debug` aborts with the assertion (SIGABRT); fixed passes 10/10. - `JSCallback exceptions propagate out of the native call`: pins the normal-exception contract (a throwing callback still raises at the FFI call site), identical before and after. The assertion only exists in `ASSERT_ENABLED` JSC builds, so the release binary passes the first test before and after; debug/ASAN is where it distinguishes. Both tests are skipped on Windows ARM64, where TinyCC (and with it `JSCallback`/`CFunction`) is compiled out. The full existing `JSCallback`/`CFunction`/threadsafe-callback matrix in `ffi.test.js` passes with the change. Related: #32780 and #32778 fix independent `JSCallback` bugs in the same functions (re-entering JS while a user exception is pending; trampoline freed under a native caller). This one is about the TerminationException specifically and composes with both, but whichever lands later needs a small rebase.
…nding A JSCallback whose body throws leaves the exception installed in the VM while control is still inside the foreign caller, which cannot unwind it. If that caller invokes the callback again before returning (qsort calling its comparator), Bun entered JSC with the exception still pending and assertion-enabled builds aborted in ExceptionScope::assertNoException. invokeFFICallback now returns immediately, without entering JS, when an exception is already pending. The throw still surfaces at the enclosing FFI call site, as before.
9b255b1 to
6d754ea
Compare
There was a problem hiding this comment.
I didn't find any issues — the guard is the standard JSC ThrowScope idiom and the test is solid — but exception-scope handling at the FFI↔JS re-entry boundary is subtle enough that someone with JSC context should give it a look, especially given the noted overlap with #32778.
Extended reasoning...
Overview
This PR adds a 6-line early-return guard to invokeFFICallback in src/jsc/bindings/JSFFIFunction.cpp: if scope.exception() is already set when an FFI callback trampoline fires, return jsNull() immediately rather than calling JSC::profiledCall. This prevents debug/ASAN builds from aborting on ExceptionScope::assertNoException() when native code (e.g. qsort) re-invokes a JSCallback after the first invocation threw. A new subprocess test in test/js/bun/ffi/ffi.test.js exercises this via qsort with a throwing comparator and asserts the error is caught once at the enclosing FFI call site.
Security risks
None identified. The change only short-circuits re-entry into JS when an exception is already pending; it does not introduce new data flows, parsing, auth, or external input handling. The test dlopens the system C library, which is consistent with existing tests in this file.
Level of scrutiny
Moderate. The diff is tiny and follows the canonical JSC ThrowScope pattern (matching the existing RETURN_IF_EXCEPTION fallback value), but it sits at the native↔VM boundary where exception-scope semantics are subtle. The PR description argues release builds already effectively behave this way via VMTraps::NeedExceptionHandling, and verification under BUN_JSC_validateExceptionChecks=1 is reassuring — but a human familiar with Bun's JSC integration should confirm the approach, particularly since #32778 touches the same nine entry points and whichever lands second needs a rebase.
Other factors
CodeRabbit's two cosmetic nits (comment length, assertion shape) were addressed and resolved. The author's CI analysis attributes the red lanes to an unrelated darwin artifact-download timeout and a live-cert TLS test, with ffi.test.js itself passing where it ran. No CODEOWNERS cover these paths.
|
Closing this since #35246 (bun:ffi: use the engine-native FFI when available) merged and covers the same ground. Thank you @robobun for the PR — if there's a piece of this that #35246 didn't pick up, please say so and we'll take another look. (This comment was written by Claude, on behalf of the Bun team.) |
|
Thanks, and confirmed: #35246 covers it. The function this PR patched ( if (Exception* pendingException = vm.exceptionForInspection()) [[unlikely]] {
returnSlot = 0;
return { encodedJSUndefined(), pendingException };
}which is the same semantics this PR would have added, implemented more cleanly. I rebuilt One small observation, not a request: the stress suite's nearest case, |
2 participants
What
A
JSCallbackwhose JS body throws leaves the exception installed in the VM while control returns to the foreign caller, which cannot unwind it. When that caller invokes the callback again before returning (for exampleqsortcalling its comparator a second time), Bun re-enters JSC with the exception still pending and assertion-enabled builds abort:Repro (debug / ASAN builds):
Cause
The FFI callback glue leaves the exception pending so it surfaces once the enclosing FFI call returns to JS, which is the normal JSC host-function contract. What was missing is the other half of that contract: while an exception is pending, nothing may start a new JS execution. The native caller has no way to know a throw happened and keeps invoking the callback;
Interpreter::executeCallImplbegins withscope.assertNoException(), which is what aborts. Release builds happen to survive becauseVMTraps::NeedExceptionHandlingshort-circuits the callee before its body runs, but that is exactly the state JSC's assertion forbids.Fix
invokeFFICallback, the shared tail of everyFFI_Callback_*entry point, now returns immediately, without entering JS, when the VM already has a pending exception. Everything else is unchanged: the exception still stays pending and is raised at the enclosing FFI call site.Verification
New test in
test/js/bun/ffi/ffi.test.jsspawns a child thatqsorts 8 elements with a comparator that throws, and asserts the error is caught at theqsort()call site, the callback body ran exactly once, and the process exited 0.bun-debug(currentmain): the child aborts with the assertion above and the test failsbun-debug: the wholetest/js/bun/ffi/directory passes, including the two tests added by bun:ffi: don't re-throw JSC's TerminationException from a JSCallback #32792; also clean underBUN_JSC_validateExceptionChecks=1Rebase note
#32792 landed on
mainwhile this was open and performed the sameinvokeFFICallbackdeduplication of theFFI_Callback_*entry points, for an independent bug (re-throwing JSC's TerminationException). This PR is rebased on top of it, so the remainingsrc/change is just the pending-exception guard inside that helper. #32778 fixes a third independentJSCallbackbug in the same functions.