fix: bound JSICache slot growth, and give ReferenceState a real control block

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) {

packages/react-native-nitro-modules/cpp/jsi/JSICache.hpp

@@ -10,6 +10,8 @@
 #include "BorrowingReference.hpp"
 #include "NitroLogger.hpp"
 #include "WeakReference.hpp"
+#include <algorithm>
+#include <cstddef>
 #include <jsi/jsi.h>
 #include <memory>
 #include <mutex>
@@ -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 <typename T>
+  class WeakCache final {
+  public:
+    void push(WeakReference<T>&& reference) {
+      if (_references.size() >= _compactAt) [[unlikely]] {
+        compact();
+      }
+      _references.push_back(std::move(reference));
+    }
+
+    [[nodiscard]]
+    const std::vector<WeakReference<T>>& references() const {
+      return _references;
+    }
+
+  private:
+    void compact() {
+      _references.erase(
+          std::remove_if(_references.begin(), _references.end(), [](const WeakReference<T>& reference) { return reference.isDeleted(); }),
+          _references.end());
+      _compactAt = std::max(kMinCompactSize, _references.size() * 2);
+    }
+
+  private:
+    static inline constexpr size_t kMinCompactSize = 64;
+
+    std::vector<WeakReference<T>> _references;
+    size_t _compactAt{kMinCompactSize};
+  };
+
 private:
   std::mutex _mutex;
-  std::vector<WeakReference<jsi::Value>> _valueCache;
-  std::vector<WeakReference<jsi::Object>> _objectCache;
-  std::vector<WeakReference<jsi::Function>> _functionCache;
-  std::vector<WeakReference<jsi::WeakObject>> _weakObjectCache;
-  std::vector<WeakReference<jsi::PropNameID>> _propNameIDCache;
-  std::vector<WeakReference<jsi::ArrayBuffer>> _arrayBufferCache;
+  WeakCache<jsi::Value> _valueCache;
+  WeakCache<jsi::Object> _objectCache;
+  WeakCache<jsi::Function> _functionCache;
+  WeakCache<jsi::WeakObject> _weakObjectCache;
+  WeakCache<jsi::PropNameID> _propNameIDCache;
+  WeakCache<jsi::ArrayBuffer> _arrayBufferCache;
 
 private:
   static inline std::unordered_map<jsi::Runtime*, std::weak_ptr<JSICache>> _globalCache;
@@ -86,32 +137,32 @@ class JSICacheReference final {
 public:
   BorrowingReference<jsi::Value> makeShared(jsi::Value&& value) {
     BorrowingReference<jsi::Value> owning(new jsi::Value(std::move(value)));
-    _strongCache->_valueCache.push_back(owning.weak());
+    _strongCache->_valueCache.push(owning.weak());
     return owning;
   }
   BorrowingReference<jsi::Object> makeShared(jsi::Object&& value) {
     BorrowingReference<jsi::Object> owning(new jsi::Object(std::move(value)));
-    _strongCache->_objectCache.push_back(owning.weak());
+    _strongCache->_objectCache.push(owning.weak());
     return owning;
   }
   BorrowingReference<jsi::Function> makeShared(jsi::Function&& value) {
     BorrowingReference<jsi::Function> owning(new jsi::Function(std::move(value)));
-    _strongCache->_functionCache.push_back(owning.weak());
+    _strongCache->_functionCache.push(owning.weak());
     return owning;
   }
   BorrowingReference<jsi::WeakObject> makeShared(jsi::WeakObject&& value) {
     BorrowingReference<jsi::WeakObject> owning(new jsi::WeakObject(std::move(value)));
-    _strongCache->_weakObjectCache.push_back(owning.weak());
+    _strongCache->_weakObjectCache.push(owning.weak());
     return owning;
   }
   BorrowingReference<jsi::PropNameID> makeShared(jsi::PropNameID&& value) {
     BorrowingReference<jsi::PropNameID> owning(new jsi::PropNameID(std::move(value)));
-    _strongCache->_propNameIDCache.push_back(owning.weak());
+    _strongCache->_propNameIDCache.push(owning.weak());
     return owning;
   }
   BorrowingReference<jsi::ArrayBuffer> makeShared(jsi::ArrayBuffer&& value) {
     BorrowingReference<jsi::ArrayBuffer> owning(new jsi::ArrayBuffer(std::move(value)));
-    _strongCache->_arrayBufferCache.push_back(owning.weak());
+    _strongCache->_arrayBufferCache.push(owning.weak());
     return owning;
   }
 

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<T> -> BorrowingReference<T> Lock-constructor
-  explicit BorrowingReference(const WeakReference<T>& ref) : _value(ref._value), _state(ref._state) {
-    _state->strongRefCount++;
-  }
+  // WeakReference<T> -> BorrowingReference<T> Lock-constructor.
+  // The caller (`WeakReference<T>::lock()`) has already claimed the strong ref count via
+  // `tryIncrementStrongRefCount()`, so this must NOT increment it again.
+  explicit BorrowingReference(const WeakReference<T>& ref) : _value(ref._value), _state(ref._state) {}
 
 private:
   // BorrowingReference<C> -> BorrowingReference<T> 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() {

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

packages/react-native-nitro-modules/cpp/utils/WeakReference+Borrowing.hpp

@@ -26,6 +26,11 @@ BorrowingReference<T> WeakReference<T>::lock() const {
     // return nullptr
     return BorrowingReference<T>();
   }
+  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<T>();
+  }
 
   return BorrowingReference(*this);
 }

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<T> 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<T>;
 
 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<T>(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: