ffi: avoid copying the threadsafe callback wrapper on the calling thread#31332
Conversation
|
Updated 10:53 PM PT - May 24th, 2026
✅ @Jarred-Sumner, your commit 5469dde536d82c7fdf2eb396bced08c050e605ed passed in 🧪 To try this PR locally: bunx bun-pr 31332That installs a local version of the PR into your bun-31332 --bun |
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 26 minutes and 13 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core change looks sound, but this reworks cross-thread lifetime/refcounting of JSC Strong handles and changes close() semantics (pending invocations now outlive close), so it warrants a human pass on the concurrency edges.
Extended reasoning...
Overview
This PR changes FFICallbackFunctionWrapper from a plain heap object to ThreadSafeRefCounted, replaces the by-value capture in FFI_Callback_threadsafe_call with a Ref<> capture, caches the ScriptExecutionContextIdentifier at construction so the foreign thread never dereferences a Strong, and turns FFICallbackFunctionWrapper_destroy into a deref(). It also adds the first foreign-thread JSCallback coverage via a TinyCC-compiled pthread fixture in cc.test.ts.
Security risks
None in the traditional sense (no auth/input parsing). The risk surface is memory safety: this is a concurrency fix in a path that previously copy-constructed JSC::Strong handles off the JS thread. The new design must guarantee the wrapper's destructor (which tears down two Strongs) only ever runs on the JS thread — that holds when the posted task runs, but the edge where postTaskTo drops the lambda without executing it (context teardown) is worth a human sanity check.
Level of scrutiny
High. This is hand-written cross-thread lifetime management against JSC GC internals (HandleSet), and it intentionally changes user-visible semantics: JSCallback.close() now lets already-queued threadsafe invocations run instead of (previously) racing/UAF. The new tests pin that behavior. The pthread ABI is also hand-redeclared in the TinyCC fixture (unsigned long for pthread_t), which is platform-fragile enough to deserve a second look for the macOS/musl cases.
Other factors
The reasoning in the PR description and inline comments is thorough and the change is small and well-targeted; I didn't find correctness bugs in the C++ beyond the considerations above. The only inline finding is a test-diagnostics nit. Given the concurrency subtlety and the semantic change to close(), deferring to a human reviewer rather than auto-approving.
Jarred-Sumner
deleted the
claude/hardening-fix-r4-68-thread-safe-ffi-callback-copies-gc
branch
* oven/main (10 new commits): Optimize TextEncoder.encode: restore SIMD ASCII fast paths lost in the Rust port (oven-sh#31385) js_parser: sanitize auto-generated default export name for digit-named modules (oven-sh#31403) fetch: run checkServerIdentity before writing the request (oven-sh#31325) ffi: avoid copying the threadsafe callback wrapper on the calling thread (oven-sh#31332) install: gate the exit-callback cache teardown to the main thread (oven-sh#31376) fix(node:module): don't register native helpers as their own constructors (oven-sh#31393) css: escape custom pseudo-class/element names when printing (oven-sh#31404) Deepen the lots-of-for-loop fixture so the transpiler stack-overflow tests throw on Windows (oven-sh#31382) Hardening: input validation and bounds tightening across 36 subsystems (round 4) (oven-sh#31339) Speed up FormData multipart serialization (oven-sh#31379) Auto-merged: src/install/PackageManager.rs, src/runtime/cli/upgrade_command.rs, src/runtime/webcore/Blob.rs, src/sys/lib.rs
2 participants
FFI_Callback_threadsafe_callruns on whatever thread the C library invokes the callback from, and captured theFFICallbackFunctionWrapperby value into the event-loop task, copy-constructing its twoJSC::Strongmembers off the JS thread. This change makes the wrapperThreadSafeRefCounted, captures aRef<>in the task instead (an atomic increment), caches theScriptExecutionContextIdentifierat construction time so the calling thread never reads through aStrong, and turnsFFICallbackFunctionWrapper_destroyinto aderef()so pending tasks keep the wrapper alive acrossJSCallback.close(). The wrapper is now non-copyable, so the by-value capture cannot come back.Adds the first CI coverage for threadsafe
JSCallbacks actually invoked from a foreign thread (via a TinyCC-compiled pthread fixture incc.test.ts): one test delivers 200 invocations from a worker thread while the JS thread churns handle allocations, and one closes the callback while invocations are still queued and verifies they are all still delivered. These tests exercise and pin the new behavior; the underlying off-thread handle allocation was timing-dependent, so the change itself is verified by inspection (noJSC::Strongis constructed, copied, or destroyed off the JS thread on any reachable path) plus repeated local runs.