fix: make static per-Runtime caches thread-safe (fatal "jsi::Function has already been deleted" crashes) - #1451
Open
jslok wants to merge 2 commits into
Open
Conversation
JSICache::_globalCache, Dispatcher::_globalCache, CommonGlobals::_cache, PropNameIDCache::_cache and HybridObjectPrototype::_prototypeCache are static unordered_maps keyed by jsi::Runtime* with no synchronization. They are read and mutated concurrently from every Runtime's thread (RN runtime + worklet runtimes), which is UB. Worst case: a racy find() miss in JSICache::getOrCreateCache makes a live Runtime create a DUPLICATE JSICache. In release, defineGlobal is a plain setProperty that silently replaces __nitroJsiCache (debug throws), so the orphaned cache gets GC'd and its destructor force-destroys every jsi::Function created through it - any later invocation of a stored callback then throws 'the underlying jsi::Function has already been deleted!' into the RuntimeScheduler and crashes the app. Each mutex is scoped to the map operations only and never held across a JSI call (getOrCreateCache is re-entrant via defineGlobal -> getGlobalFunction -> getOrCreateCache). For the nested caches only the outer slot acquisition is locked: the inner per-Runtime map is only ever touched from that Runtime's own thread, and unordered_map value references stay valid across rehash. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
If the underlying jsi::Function is force-deleted (JSICache/Runtime teardown) between scheduling and execution, the dispatched lambda in AsyncJSCallback::callAndForget threw a std::runtime_error that escalates to an uncaught fatal in the target Runtime's scheduler. callAndForget explicitly ignores results and completions, so add SyncJSCallback::isAlive() and log-and-skip instead of crashing. The Promise-returning call() path is unchanged (its throw surfaces as a catchable rejection). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
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
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
Nitro keeps five static
unordered_maps keyed byjsi::Runtime*with no locking:JSICache::_globalCache,Dispatcher::_globalCache,CommonGlobals::_cache,PropNameIDCache::_cache,HybridObjectPrototype::_prototypeCache. Apps running multiple Runtimes (the RN runtime + react-native-worklets runtimes, e.g. VisionCamera frame processors) read and mutate these concurrently from different threads — and concurrentunordered_mapmutation is UB. (HybridObjectPrototype::ensureInitializedalready has a mutex "in case we try to create HybridObjects in parallel Runtimes", but the maps themselves are unguarded.)We are seeing this crash in production (Android arm64, RN 0.86 bridgeless, release builds): fatal
Non-js exception: Cannot call SyncJSCallback<void()> - the underlying jsi::Function has already been deleted!(also thevoid(const std::string&)variant), always shortly after additional worklet runtimes spin up, plus occasional[libNitroModules.so]SIGSEGVs consistent with the same race.Mechanism (worst case)
JSICache::_globalCache, triggering a rehash, while thread B (main runtime) runsgetOrCreateCache—find()spuriously misses the live entry.JSICachefor the main runtime. In release,defineGlobalis a plainsetProperty, so__nitroJsiCacheis silently replaced (debug throws, so the race never reproduces in dev builds).~JSICache()force-destroys everyjsi::Functioncreated through it — while the runtime is alive and healthy.callAndForgetinvocation always holds a strongBorrowingReferencecopy, so cache destruction is the only way_functioncan be null under it.)Fix
Commit 1 — a mutex per static map, scoped strictly to the map operations and never held across a JSI call (
getOrCreateCacheis re-entrant viadefineGlobal→getGlobalFunction→getOrCreateCache). For the nested caches only the outermap[&runtime]slot acquisition is locked: the inner per-Runtime map is only ever touched from that Runtime's own thread, andunordered_mapvalue references are stable across rehash. Hot paths pay an uncontended lock around a lookup — behavior otherwise unchanged.Commit 2 (separable if you'd rather discuss it) —
callAndForgetchecks a newSyncJSCallback::isAlive()inside the dispatched lambda and logs-and-skips a force-deleted function instead of throwing an uncaught fatal. It is fire-and-forget — results/completions are ignored by contract — and this also covers the legitimate case of a queued invocation draining after Runtime teardown. The Promise-returningcall()path is unchanged (its throw surfaces as a catchable rejection).Not touched, audited as safe:
Prototype::get's cache (all mutations serialized byensureInitialized's mutex) andNitroTypeInfo::replaceRegex's regex cache (debug-only code path).Possibly related: #1104.
🤖 Generated with Claude Code