From d6b93795fdbf8a111c8f76af29081c37800f719a Mon Sep 17 00:00:00 2001 From: jslok Date: Fri, 31 Jul 2026 19:10:51 -0700 Subject: [PATCH] fix: bound JSICache slot growth, and give ReferenceState a real control block JSICache`s six weak-slot lists were append-only for the lifetime of a Runtime. Every makeShared() pushed a slot that was only freed in ~JSICache, so a caller that converts values to JS at a high rate grew them without bound long after the values themselves had been collected. They are now self-compacting: a slot whose value is definitively deleted is dropped, behind a doubling watermark that keeps this amortized O(1) per push. Compaction needs to probe liveness without resurrecting anything, so WeakReference gained isDeleted(), which reads the atomic flag instead of going through lock(). That probe exposed two lifetime bugs underneath: - ReferenceState was freed when "strong == 0 && weak == 0", reading two atomics non-atomically as a unit. A last-strong releaser and a last-weak releaser running concurrently could both observe (0, 0) and double-delete the state, or one could read a state the other had already freed. The strong cohort now owns one implicit weak reference, exactly like shared_ptr`s control block, so fetch_sub`s return value alone decides who frees the state and only one thread can ever see the 1 -> 0 transition. - WeakReference::lock() checked isDeleted and then incremented the strong count, but ~BorrowingReference decrements that count BEFORE setting isDeleted. A lock() landing in that window resurrected a dying value, and the resurrected reference then ran a second final release. lock() now uses an increment-if-not-zero, the same guard weak_ptr::lock() uses. Co-Authored-By: Claude Fable 5 --- .../cpp/jsi/JSICache.cpp | 14 ++-- .../cpp/jsi/JSICache.hpp | 75 ++++++++++++++++--- .../cpp/utils/BorrowingReference.hpp | 24 +++--- .../cpp/utils/ReferenceState.hpp | 29 ++++++- .../cpp/utils/WeakReference+Borrowing.hpp | 5 ++ .../cpp/utils/WeakReference.hpp | 43 +++++++---- 6 files changed, 146 insertions(+), 44 deletions(-) diff --git a/packages/react-native-nitro-modules/cpp/jsi/JSICache.cpp b/packages/react-native-nitro-modules/cpp/jsi/JSICache.cpp index 45739e3e8f..068691a44c 100644 --- a/packages/react-native-nitro-modules/cpp/jsi/JSICache.cpp +++ b/packages/react-native-nitro-modules/cpp/jsi/JSICache.cpp @@ -27,12 +27,14 @@ JSICache::~JSICache() { Logger::log(LogLevel::Info, TAG, "Destroying JSICache..."); std::unique_lock lock(_mutex); - destroyReferences(_valueCache); - destroyReferences(_objectCache); - destroyReferences(_functionCache); - destroyReferences(_weakObjectCache); - destroyReferences(_propNameIDCache); - destroyReferences(_arrayBufferCache); + // `.references()` - the caches self-compact dead slots as they grow (see `JSICache::WeakCache`), so this only + // ever walks slots that were still live at the last compaction. + destroyReferences(_valueCache.references()); + destroyReferences(_objectCache.references()); + destroyReferences(_functionCache.references()); + destroyReferences(_weakObjectCache.references()); + destroyReferences(_propNameIDCache.references()); + destroyReferences(_arrayBufferCache.references()); } JSICacheReference JSICache::getOrCreateCache(jsi::Runtime& runtime) { diff --git a/packages/react-native-nitro-modules/cpp/jsi/JSICache.hpp b/packages/react-native-nitro-modules/cpp/jsi/JSICache.hpp index 75785796aa..034e99f5a7 100644 --- a/packages/react-native-nitro-modules/cpp/jsi/JSICache.hpp +++ b/packages/react-native-nitro-modules/cpp/jsi/JSICache.hpp @@ -10,6 +10,8 @@ #include "BorrowingReference.hpp" #include "NitroLogger.hpp" #include "WeakReference.hpp" +#include +#include #include #include #include @@ -57,14 +59,63 @@ class JSICache final : public jsi::NativeState { private: friend class JSICacheReference; +public: + /** + * A list of weakly-held cache slots that drops dead slots as it grows. + * + * The cache exists so that `~JSICache` can force-destroy every value it handed out when the Runtime goes away. + * Slots were previously only ever appended, and freed as a whole in `~JSICache` - so a caller that converts a + * HybridObject to JS at a high rate grew these lists without bound for the lifetime of the Runtime, long after + * the values themselves had been collected. + * + * `compact()` only ever erases slots whose value is definitively deleted (`isDeleted()`), so it can never drop + * a slot that `~JSICache` still needs to destroy - it just removes the bookkeeping for values that no longer + * exist. The probe deliberately does NOT use `lock()`, which could resurrect a value whose final release is + * mid-flight on another Thread (see `WeakReference::isDeleted`); reading the atomic flag is one-sided-safe, so + * a racing release merely keeps the slot until the next compaction. + * + * The `_compactAt` watermark keeps this amortized O(1) per push: after compacting we only try again once the + * list has doubled, so a cache made mostly of LONG-LIVED values (where compaction reclaims nothing) does not + * re-scan on every insert. + */ + template + class WeakCache final { + public: + void push(WeakReference&& reference) { + if (_references.size() >= _compactAt) [[unlikely]] { + compact(); + } + _references.push_back(std::move(reference)); + } + + [[nodiscard]] + const std::vector>& references() const { + return _references; + } + + private: + void compact() { + _references.erase( + std::remove_if(_references.begin(), _references.end(), [](const WeakReference& reference) { return reference.isDeleted(); }), + _references.end()); + _compactAt = std::max(kMinCompactSize, _references.size() * 2); + } + + private: + static inline constexpr size_t kMinCompactSize = 64; + + std::vector> _references; + size_t _compactAt{kMinCompactSize}; + }; + private: std::mutex _mutex; - std::vector> _valueCache; - std::vector> _objectCache; - std::vector> _functionCache; - std::vector> _weakObjectCache; - std::vector> _propNameIDCache; - std::vector> _arrayBufferCache; + WeakCache _valueCache; + WeakCache _objectCache; + WeakCache _functionCache; + WeakCache _weakObjectCache; + WeakCache _propNameIDCache; + WeakCache _arrayBufferCache; private: static inline std::unordered_map> _globalCache; @@ -86,32 +137,32 @@ class JSICacheReference final { public: BorrowingReference makeShared(jsi::Value&& value) { BorrowingReference owning(new jsi::Value(std::move(value))); - _strongCache->_valueCache.push_back(owning.weak()); + _strongCache->_valueCache.push(owning.weak()); return owning; } BorrowingReference makeShared(jsi::Object&& value) { BorrowingReference owning(new jsi::Object(std::move(value))); - _strongCache->_objectCache.push_back(owning.weak()); + _strongCache->_objectCache.push(owning.weak()); return owning; } BorrowingReference makeShared(jsi::Function&& value) { BorrowingReference owning(new jsi::Function(std::move(value))); - _strongCache->_functionCache.push_back(owning.weak()); + _strongCache->_functionCache.push(owning.weak()); return owning; } BorrowingReference makeShared(jsi::WeakObject&& value) { BorrowingReference owning(new jsi::WeakObject(std::move(value))); - _strongCache->_weakObjectCache.push_back(owning.weak()); + _strongCache->_weakObjectCache.push(owning.weak()); return owning; } BorrowingReference makeShared(jsi::PropNameID&& value) { BorrowingReference owning(new jsi::PropNameID(std::move(value))); - _strongCache->_propNameIDCache.push_back(owning.weak()); + _strongCache->_propNameIDCache.push(owning.weak()); return owning; } BorrowingReference makeShared(jsi::ArrayBuffer&& value) { BorrowingReference owning(new jsi::ArrayBuffer(std::move(value))); - _strongCache->_arrayBufferCache.push_back(owning.weak()); + _strongCache->_arrayBufferCache.push(owning.weak()); return owning; } diff --git a/packages/react-native-nitro-modules/cpp/utils/BorrowingReference.hpp b/packages/react-native-nitro-modules/cpp/utils/BorrowingReference.hpp index 2100469af2..5f6a5985fe 100644 --- a/packages/react-native-nitro-modules/cpp/utils/BorrowingReference.hpp +++ b/packages/react-native-nitro-modules/cpp/utils/BorrowingReference.hpp @@ -56,8 +56,8 @@ class BorrowingReference final { bool shouldDestroy = _state->decrementStrongRefCount(); if (shouldDestroy) { forceDestroyValue(); + releaseImplicitWeakRef(); } - maybeDestroyState(); } _value = ref._value; @@ -71,10 +71,10 @@ class BorrowingReference final { } private: - // WeakReference -> BorrowingReference Lock-constructor - explicit BorrowingReference(const WeakReference& ref) : _value(ref._value), _state(ref._state) { - _state->strongRefCount++; - } + // WeakReference -> BorrowingReference Lock-constructor. + // The caller (`WeakReference::lock()`) has already claimed the strong ref count via + // `tryIncrementStrongRefCount()`, so this must NOT increment it again. + explicit BorrowingReference(const WeakReference& ref) : _value(ref._value), _state(ref._state) {} private: // BorrowingReference -> BorrowingReference Cast-constructor @@ -97,8 +97,8 @@ class BorrowingReference final { bool shouldDestroy = _state->decrementStrongRefCount(); if (shouldDestroy) { forceDestroyValue(); + releaseImplicitWeakRef(); } - maybeDestroyState(); } public: @@ -192,12 +192,16 @@ class BorrowingReference final { } private: - void maybeDestroyState() { - if (_state->strongRefCount == 0 && _state->weakRefCount == 0) { - // free the full memory if there are no more references at all + /** + * Releases the strong cohort's implicit weak reference (see `ReferenceState`). Called exactly once per state, + * by whichever strong reference performed the final strong release - the value is already destroyed at this + * point. Frees the state if no `WeakReference` is left holding it either. + */ + void releaseImplicitWeakRef() { + if (_state->weakRefCount.fetch_sub(1) == 1) { delete _state; - _state = nullptr; } + _state = nullptr; } void forceDestroyValue() { diff --git a/packages/react-native-nitro-modules/cpp/utils/ReferenceState.hpp b/packages/react-native-nitro-modules/cpp/utils/ReferenceState.hpp index 7bc6316946..1b3d6aeff1 100644 --- a/packages/react-native-nitro-modules/cpp/utils/ReferenceState.hpp +++ b/packages/react-native-nitro-modules/cpp/utils/ReferenceState.hpp @@ -34,7 +34,34 @@ struct ReferenceState { return oldRefCount <= 1; } - explicit ReferenceState() : strongRefCount(1), weakRefCount(0), isDeleted(false) {} + /** + * Increments the strong ref count by one, but only if it is not already zero, and returns whether it did. + * + * A zero strong count means the final strong release is already under way, so the value is about to be + * destroyed even if `isDeleted` has not been set yet - `~BorrowingReference` decrements the count BEFORE + * calling `forceDestroyValue()`. Handing out a strong reference in that window would resurrect a dying value, + * and the resurrected reference would then run a second final release. This is `weak_ptr::lock()`'s + * increment-if-not-zero. + */ + inline bool tryIncrementStrongRefCount() { + size_t count = strongRefCount.load(); + while (count != 0) { + if (strongRefCount.compare_exchange_weak(count, count + 1)) { + return true; + } + } + return false; + } + + // `weakRefCount` starts at 1: the strong cohort collectively owns one implicit weak reference, 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. + // + // This is the same shape as `shared_ptr`'s control block, and it exists because the previous scheme ("delete + // the state when `strong == 0 && weak == 0`") read the two counters non-atomically as a unit: a last-strong + // releaser and a last-weak releaser running concurrently could BOTH observe (0, 0) and double-delete the + // state - or one could read a state the other had already freed. + explicit ReferenceState() : strongRefCount(1), weakRefCount(1), isDeleted(false) {} }; } // namespace margelo::nitro diff --git a/packages/react-native-nitro-modules/cpp/utils/WeakReference+Borrowing.hpp b/packages/react-native-nitro-modules/cpp/utils/WeakReference+Borrowing.hpp index a97d7be369..d4aab617be 100644 --- a/packages/react-native-nitro-modules/cpp/utils/WeakReference+Borrowing.hpp +++ b/packages/react-native-nitro-modules/cpp/utils/WeakReference+Borrowing.hpp @@ -26,6 +26,11 @@ BorrowingReference WeakReference::lock() const { // return nullptr return BorrowingReference(); } + if (!_state->tryIncrementStrongRefCount()) { + // The last strong reference is mid-release and this value is about to be destroyed - it just hasn't + // flagged `isDeleted` yet. Resurrecting it here would cause a second final release. + return BorrowingReference(); + } return BorrowingReference(*this); } diff --git a/packages/react-native-nitro-modules/cpp/utils/WeakReference.hpp b/packages/react-native-nitro-modules/cpp/utils/WeakReference.hpp index 22ca5c77fb..d439d03812 100644 --- a/packages/react-native-nitro-modules/cpp/utils/WeakReference.hpp +++ b/packages/react-native-nitro-modules/cpp/utils/WeakReference.hpp @@ -49,8 +49,7 @@ class WeakReference final { return *this; if (_state != nullptr) { - _state->weakRefCount--; - maybeDestroy(); + releaseWeakRef(); } _value = ref._value; @@ -67,8 +66,7 @@ class WeakReference final { if (_state != nullptr) { // destroy previous pointer - _state->weakRefCount--; - maybeDestroy(); + releaseWeakRef(); } _value = ref._value; @@ -83,8 +81,7 @@ class WeakReference final { ~WeakReference() { if (_state != nullptr) { - _state->weakRefCount--; - maybeDestroy(); + releaseWeakRef(); } } @@ -94,21 +91,37 @@ class WeakReference final { [[nodiscard]] BorrowingReference lock() const; + /** + * Returns whether the referenced value has already been deleted. + * + * Unlike `lock()`, this never materializes a strong reference, so it is safe to call while the value's final + * release may be running concurrently on another Thread: `lock()` checks `isDeleted` and then increments the + * strong count, but `~BorrowingReference` decrements the count BEFORE setting `isDeleted` (and without holding + * the state mutex), so a `lock()` landing in that window resurrects the dying value - and the resurrected + * temporary's destructor then runs a second `forceDestroyValue()` concurrently with the releaser's. A plain + * read of the atomic flag cannot resurrect anything; during such a race it merely still reports the value as + * alive, which callers must treat as "maybe alive". + */ + [[nodiscard]] + bool isDeleted() const { + return _state == nullptr || _state->isDeleted.load(); + } + public: friend class BorrowingReference; private: - void maybeDestroy() { - if (_state->strongRefCount == 0 && _state->weakRefCount == 0) { - // free the full memory if there are no more references at all - if (!_state->isDeleted) [[unlikely]] { - std::string typeName = TypeInfo::getFriendlyTypename(true); - throw std::runtime_error("WeakReference<" + typeName + "> encountered a stale `_value` - BorrowingReference<" + typeName + - "> should've already deleted this!"); - } + /** + * Releases this weak reference's count on the state, freeing the state if it was the last reference overall. + * The strong cohort owns one implicit weak reference (see `ReferenceState`), so the count can only reach zero + * after the final strong release already destroyed the value, and `fetch_sub`'s return value alone decides + * who frees the state. + */ + void releaseWeakRef() { + if (_state->weakRefCount.fetch_sub(1) == 1) { delete _state; - _state = nullptr; } + _state = nullptr; } private: