fix: free ReferenceState via a shared_ptr-style control block - #1468
Open
jslok wants to merge 2 commits into
Open
fix: free ReferenceState via a shared_ptr-style control block#1468jslok wants to merge 2 commits into
ReferenceState via a shared_ptr-style control block#1468jslok wants to merge 2 commits into
Conversation
…l-release lock() checked isDeleted and then unconditionally incremented the strong count, but ~BorrowingReference decrements the count BEFORE calling forceDestroyValue() and holds no mutex while doing so. A lock() landing in that window handed out a strong reference to a value whose final release was already in flight, and the resurrected reference's destructor then ran a second forceDestroyValue() concurrently with the releaser's. lock() now claims the strong count with an increment-if-not-zero compare-exchange loop, exactly like weak_ptr::lock(), and returns null if the count already reached zero. The private WeakReference -> BorrowingReference lock-constructor no longer increments, since lock() has already claimed the count. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both BorrowingReference::maybeDestroyState() and WeakReference::maybeDestroy() freed the state when strongRefCount == 0 && weakRefCount == 0. Those are two independent atomics read as a unit, so a last-strong releaser and a last-weak releaser running concurrently could BOTH observe (0, 0) - a double delete of the state - or one could read a counter out of a state the other had already freed. ReferenceState now uses shared_ptr's control-block scheme: weakRefCount starts at 1, representing one implicit weak reference collectively owned by the strong cohort, released by whichever strong reference performs the final strong release (after it destroyed the value). The state is freed by whoever brings weakRefCount to zero, decided by fetch_sub's return value alone - exactly one thread ever sees the 1 -> 0 transition, and weakRefCount cannot reach zero before the final strong release has completed. Depends on the lock() increment-if-not-zero fix: a lock() resurrecting a value mid final-release would run a second final strong release, releasing the implicit weak reference twice. 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
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.
Part 2 of 3 - #1464 split into atomic PRs as requested. Stacked on #1467, which must merge first - this branch contains #1467's commit, so only the last commit is new here. I'll rebase once #1467 lands. (It needs #1467 not just textually: without the non-resurrecting
lock(), a resurrected strong reference would perform a second final strong release and double-release the implicit weak reference introduced here.)The race
Both
BorrowingReference::maybeDestroyState()andWeakReference::maybeDestroy()freed the state whenstrongRefCount == 0 && weakRefCount == 0. Those are two independent atomics read as a unit, so a last-strong releaser and a last-weak releaser running concurrently can both observe(0, 0)- a doubledelete _state- or one can read a counter out of a state the other has already freed.The fix
ReferenceStatenow usesshared_ptr's control-block scheme:weakRefCountstarts at 1, representing one implicit weak reference collectively owned by the strong cohort, released by whichever strong reference performs the final strong release (after it destroyed the value). The state is freed by whoever bringsweakRefCountto zero, decided byfetch_sub's return value alone - so exactly one thread ever sees the1 -> 0transition, andweakRefCountcannot reach zero before the final strong release has completed.The "stale
_value" throw inWeakReference::maybeDestroy()goes away with it: under the control block, the weak count reaching zero implies the value was already destroyed by the final strong release.Testing
Has been running on-device (Android, release build) against a high-rate HybridObject conversion path as part of the combined #1464 change, with all Nitro-consuming modules rebuilt - no crashes or dispose warnings. Not exercised in isolation from #1467/part 3. Formatted to match
config/.clang-formatoutput in the surrounding code.🤖 Generated with Claude Code