Repository navigation
fix(runtime): never revive a JS block whose dispose has started - #500
Merged
Merged
Conversation
Re-marshals one JS function as a block while native code drops the cached block's last reference from a background queue. The releasing thread starts the block's dispose, which waits for the isolate's Locker held by the thread running JS; that thread's next cache hit Block_copies the dying block, and libclosure frees it once dispose returns while references taken in the meantime are still live. Covers release from a concurrent queue, a serial queue, and via the main NSOperationQueue, with JS on the main thread and on a GCD worker. Crashes under ASan (write-after-free on the freed block's flags) without the runtime fix.
A JS function passed as a block caches its block on itself, and a cache hit Block_copy'd that block. libclosure's increment ignores BLOCK_DEALLOCATING, so when another thread dropped the last reference and its dispose was waiting for the isolate's Locker, the Locker holder handed the dying block back to native code, which libclosure freed once dispose returned. In production this showed up as _Block_copy faulting at 0x8 under -[NSBlockOperation main] and as over-releases in dispatch's autorelease pool pops. The cache hit now takes a reference only while the block is live (a CAS on the same flags word libclosure uses) and otherwise builds a fresh block. Dispose always frees its own BlockWrapper and clears the slot only when the slot still holds it, so a wrapper superseded by a newer block is no longer leaked.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe runtime now checks cached JavaScript block liveness before retaining a block during marshalling or handle lookup. Block disposal and wrapper ownership handling have changed. Native callback helpers and tests cover asynchronous release, disposal, and worker teardown. ChangesJavaScript block lifetime
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable regression is established in this PR. It is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 5 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks each block’s refcount twice Comment |
Parks a cached block's dispose on the isolate's Locker deterministically (last reference dropped from a background queue while the JS turn sleeps) and checks that interop.handleof does not hand it out and that a new marshal of the function gets a working block. A worker that registers a function through interop.FunctionReference and gives native code a block built from it: the worker's teardown disposes the function, and the block's own dispose runs after the isolate is gone. Without the runtime fix both free the block's wrapper (ASan heap-use-after-free in the JSBlock dispose helper).
…ly live blocks ObjectManager::DisposeValue freed whatever wrapper sat in a disposed object's slot. A function registered through interop.FunctionReference and then passed as a block holds its JSBlock's wrapper there, so a worker's teardown freed that wrapper while native code still held the block, and the block's dispose freed it again once released. The JSBlock owns that wrapper; DisposeValue now leaves it alone. interop.handleof on a JS function returned its cached block unretained, including one whose dispose had already started. It now returns the block only while it is live, retained for the rest of the turn, and otherwise treats the function like one that was never marshalled. TryRetainJSBlock becomes an Interop member so HandleOf can share it.
edusperoni
marked this pull request as ready for review
October 9, 2026 17:28
edusperoni
added this pull request to stack #502
October 9, 2026 17:29
This was referenced Oct 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A JS function passed as a block parameter is converted to a
JSBlockonce, and aBlockWrapperis cached on the function. On a cache hit,SetFFIParamsdidCFAutorelease(Block_copy(block))with no liveness check.A
JSBlock's dispose runs on whichever thread drops the last native reference, and it takes the isolate'sv8::Locker. libclosure'slatching_incr_int(used by_Block_copy) does not checkBLOCK_DEALLOCATING. That allows this race:BLOCK_DEALLOCATINGand calls dispose, which blocks on the Locker because thread A is running JS.Block_copy"succeeds" on the dying block, and native code keeps it (for exampleNSOperationQueue.mainQueue.addOperationWithBlock, whichUtils.dispatchToMainThreaduses).Production signatures, from an Angular + zone.js app that runs JS callbacks on background queues and re-dispatches to the main queue during network-reconnect storms:
_Block_copyat0x8under-[NSBlockOperation main]on the main queue: the block's memory had been freed and zeroed, soflags == 0sent it down the stack-block branch through a NULL descriptor.objc_releasein_dispatch_last_resort_autorelease_pool_popon serial queues.Fix
Cache hit takes a reference only while the block is live.
TryRetainBlockCASes the refcount in the sameflagsword libclosure uses. It refuses whenBLOCK_DEALLOCATINGis set or the refcount is 0, and it respects the saturation latch. If it refuses, the call builds a fresh block and the slot is overwritten. Reading the cached block's flags is safe: while the isolate is valid, dispose clears the slot under the same Locker the cache-hit thread holds, and does so before libclosure frees the block.Native blocks keep
Block_copy. These are wrappers withOwnsBlock(), built inGetResultfor blocks returned from native code. The JS function's own copy keeps them alive.Dispose always frees its own
BlockWrapper. It clears the slot only when the slot still holds that wrapper. Previously a wrapper superseded in the slot was skipped and leaked, and with the first fix that case now happens whenever a refused cache hit refills the slot.ObjectManager::DisposeValueleaves JS-created block wrappers alone. These are the!OwnsBlock()wrappers, and theJSBlockowns them. Before, a function registered throughinterop.FunctionReferenceand then passed as a block had its block's wrapper freed by a worker's teardown (DisposeAllRegistered) while native code still held the block. The block's dispose then freed it again when native code released the block.interop.handleof(fn)returns the cached block only while it is live. It try-retains the block and autoreleases it, so the pointer stays valid for the rest of the turn, like a block passed to a native call. If the block is dying or already gone,fnis treated like a function that was never marshalled, which throws "Unknown type". Before, it returned the cached pointer unretained, including for a block whose dispose had already started.Tests
BlockCacheRaceTests.js, with fixtures inTNSTestNativeCallbacks:keepBlock:releaseMode:keeps the block and drops it 0–2 ms later from one of three places: a concurrent queue, a serial queue, or after also enqueueing it on the mainNSOperationQueue.repeat:pausingAfter:re-marshals the same function 600 times. Each step runs in its own autorelease pool, followed by a 0–3 ms sleep that stays inside the JS turn, so the last release lands while this thread still holds the Locker.More specs:
handleofdoesn't hand the dying block out, and that marshalling the function again (viaaddOperationWithBlock) gets a working block.fnwithinterop.FunctionReferenceand gives native code a block built from it, which native code keeps past the worker's teardown.Results under ASan (
./run_tests.sh -a):repeat:pausingAfter:, and a release-queue thread in_Block_release→JSBlockdispose →v8::Locker::Locker. The fault itself is a wild pointer in the fixture's__blockstorage. The freedJSBlockchunk was reused for that storage, and libclosure's latching decrement on the dead block'sflags(offset 8) corrupted the byref'sforwardingpointer. libsystem_blocks is uninstrumented, so ASan reports a SEGV rather than a heap-use-after-free.heap-use-after-freein theJSBlockdispose helper, on the block's wrapper, which the worker's teardown had already freed.Known gap (pre-existing, follow-up PR)
~Runtimedrops the isolate from the live registry, which makesIsValid()false, without holding the Locker. A foreign thread already running worker JS keeps going untilTerminateExecutionunwinds it. In that window, aJSBlockdispose on a third thread skips the Locker and frees the wrapper, and libclosure then frees the block, while the still-running JS can reach both through the cache slot.dispatch_syncs onto that queue. A deferred dispose was tried and reverted earlier (2ef5e50).Summary by CodeRabbit