Fix callback and promise teardown races

Summary:
Keep JavaScript callback and promise values rooted while native owners retain only weak lifetime handles. Dispatch release through the JavaScript executor so native invocation and destruction cannot retain these values across runtime teardown. Apply the same ownership policy to asynchronous callbacks and promises.

This builds on the investigation and reproducer contributed by HyunWoo Lee in #59579.

Changelog:
[General][Fixed] - Fix TurboModule callback and async promise cleanup racing runtime teardown
[General][Deprecated] - Deprecate CallbackWrapper in favour of LongLivedObjectHandle

Differential Revision: D128581452

Author: javache

packages/react-native/ReactCommon/react/bridging/CallbackWrapper.h

@@ -19,7 +19,10 @@ namespace facebook::react {
 
 class CallInvoker;
 
-// Helper for passing jsi::Function arg to other methods.
+/**
+ * @deprecated Use LongLivedObjectHandle<jsi::Function> instead.
+ * Access and release this legacy wrapper only on the JavaScript thread.
+ */
 class CallbackWrapper : public LongLivedObject {
  private:
   CallbackWrapper(jsi::Function &&callback, jsi::Runtime &runtime, std::shared_ptr<CallInvoker> jsInvoker)

packages/react-native/ReactCommon/react/bridging/Function.h

@@ -10,7 +10,7 @@
 #include <react/cxxstableapi/UmbrellaGuard.h>
 
 #include <react/bridging/Base.h>
-#include <react/bridging/CallbackWrapper.h>
+#include <react/bridging/CallbackWrapper.h> // Retain the legacy transitive include.
 
 #include <ReactCommon/CallInvoker.h>
 #include <ReactCommon/SchedulerPriority.h>
@@ -61,16 +61,16 @@ class AsyncCallback {
 
   void callWithArgs(std::optional<SchedulerPriority> priority, Args... args) const noexcept
   {
-    if (auto wrapper = callback_->wrapper_.lock()) {
-      auto fn = [callback = callback_,
+    auto callback = callback_;
+    if (auto jsInvoker = callback->function_.lockInvoker()) {
+      auto fn = [callback = std::move(callback),
                  argsPtr = std::make_shared<std::tuple<Args...>>(std::make_tuple(std::forward<Args>(args)...))](
                     jsi::Runtime &) { callback->apply(std::move(*argsPtr)); };
 
-      auto &jsInvoker = wrapper->jsInvoker();
       if (priority) {
-        jsInvoker.invokeAsync(*priority, std::move(fn));
+        jsInvoker->invokeAsync(*priority, std::move(fn));
       } else {
-        jsInvoker.invokeAsync(std::move(fn));
+        jsInvoker->invokeAsync(std::move(fn));
       }
     }
   }
@@ -79,57 +79,38 @@ class AsyncCallback {
       std::optional<SchedulerPriority> priority,
       std::function<void(jsi::Runtime &, jsi::Function &)> &&callImpl) const noexcept
   {
-    if (auto wrapper = callback_->wrapper_.lock()) {
-      // Capture callback_ and not wrapper_. If callback_ is deallocated or the
-      // JSVM is shutdown before the async task is scheduled, the underlying
-      // function will have been deallocated.
-      auto fn = [callback = callback_, callImpl = std::move(callImpl)](jsi::Runtime &rt) {
-        if (auto wrapper2 = callback->wrapper_.lock()) {
-          callImpl(rt, wrapper2->callback());
+    auto callback = callback_;
+    if (auto jsInvoker = callback->function_.lockInvoker()) {
+      // Retain native ownership until this task is destroyed, without retaining
+      // a JavaScript value while the task is queued or discarded.
+      auto fn = [callback = std::move(callback), callImpl = std::move(callImpl)](jsi::Runtime &rt) {
+        if (auto function = callback->function_.lockOnJS()) {
+          callImpl(rt, function.value());
         }
       };
 
-      auto &jsInvoker = wrapper->jsInvoker();
       if (priority) {
-        jsInvoker.invokeAsync(*priority, std::move(fn));
+        jsInvoker->invokeAsync(*priority, std::move(fn));
       } else {
-        jsInvoker.invokeAsync(std::move(fn));
+        jsInvoker->invokeAsync(std::move(fn));
       }
     }
   }
 };
 
-// You must ensure that when invoking this you're located on the JS thread, or
-// have exclusive control of the JS VM context. If you cannot ensure this, use
-// AsyncCallback instead.
+// Invoke only on the JavaScript thread. Use AsyncCallback from other threads.
 template <typename R, typename... Args>
 class SyncCallback<R(Args...)> {
  public:
   SyncCallback(jsi::Runtime &rt, jsi::Function function, std::shared_ptr<CallInvoker> jsInvoker)
-      : wrapper_(CallbackWrapper::createWeak(std::move(function), rt, std::move(jsInvoker)))
+      : function_(rt, std::move(function), std::move(jsInvoker))
   {
   }
 
-  // Disallow copying, as we can no longer safely destroy the callback
-  // from the destructor if there's multiple copies
   SyncCallback(const SyncCallback &) = delete;
   SyncCallback &operator=(const SyncCallback &) = delete;
-
-  // Allow move
-  SyncCallback(SyncCallback &&other) noexcept : wrapper_(std::move(other.wrapper_)) {}
-
-  SyncCallback &operator=(SyncCallback &&other) noexcept
-  {
-    wrapper_ = std::move(other.wrapper_);
-    return *this;
-  }
-
-  ~SyncCallback()
-  {
-    if (auto wrapper = wrapper_.lock()) {
-      wrapper->destroy();
-    }
-  }
+  SyncCallback(SyncCallback &&) noexcept = default;
+  SyncCallback &operator=(SyncCallback &&) noexcept = default;
 
   R operator()(Args... args) const
   {
@@ -138,21 +119,23 @@ class SyncCallback<R(Args...)> {
 
   R call(Args... args) const
   {
-    auto wrapper = wrapper_.lock();
+    auto function = function_.lockOnJS();
 
-    // If the wrapper has been deallocated, we can no longer provide a return
-    // value consistently, so our only option is to throw
-    if (!wrapper) {
+    // If the function has been deallocated, we can no longer provide a return
+    // value consistently, so our only option is to throw.
+    if (!function) {
       if constexpr (std::is_void_v<R>) {
         return;
       } else {
         throw std::runtime_error("Failed to call invalidated sync callback");
       }
     }
 
-    auto &callback = wrapper->callback();
-    auto &rt = wrapper->runtime();
-    auto jsInvoker = wrapper->jsInvokerPtr();
+    // JavaScript can destroy this native callback during the call; use only
+    // the retained value and local runtime/invoker references afterward.
+    auto &callback = function.value();
+    auto &rt = function.runtime();
+    auto jsInvoker = function.jsInvokerPtr();
 
     if constexpr (std::is_void_v<R>) {
       callback.call(rt, bridging::toJs(rt, std::forward<Args>(args), jsInvoker)...);
@@ -177,8 +160,7 @@ class SyncCallback<R(Args...)> {
     return call(std::move(std::get<Index>(args))...);
   }
 
-  // Held weakly so lifetime is managed by LongLivedObjectCollection.
-  std::weak_ptr<CallbackWrapper> wrapper_;
+  LongLivedObjectHandle<jsi::Function> function_;
 };
 
 template <typename... Args>
@@ -191,7 +173,7 @@ struct Bridging<AsyncCallback<Args...>> {
 
   static jsi::Function toJs(jsi::Runtime &rt, const AsyncCallback<Args...> &value)
   {
-    return value.callback_->function_.getFunction(rt);
+    return Bridging<SyncCallback<void(Args...)>>::toJs(rt, *value.callback_);
   }
 };
 
@@ -205,7 +187,10 @@ struct Bridging<SyncCallback<R(Args...)>> {
 
   static jsi::Function toJs(jsi::Runtime &rt, const SyncCallback<R(Args...)> &value)
   {
-    return value.function_.getFunction(rt);
+    if (auto function = value.function_.lockOnJS()) {
+      return function.value().getFunction(rt);
+    }
+    throw jsi::JSError(rt, "Failed to get invalidated callback");
   }
 };
 

packages/react-native/ReactCommon/react/bridging/LongLivedObject.cpp

@@ -6,6 +6,8 @@
  */
 
 #include "LongLivedObject.h"
+
+#include <ReactCommon/CallInvoker.h>
 #include <unordered_map>
 
 namespace facebook::react {
@@ -35,22 +37,28 @@ void LongLivedObjectCollection::add(std::shared_ptr<LongLivedObject> so) {
 }
 
 void LongLivedObjectCollection::remove(const LongLivedObject* o) {
-  std::scoped_lock lock(collectionMutex_);
-  // Empty owner: lookup-only alias, with no ownership or control block.
-  const std::shared_ptr<LongLivedObject> lookupKey{
-      std::shared_ptr<LongLivedObject>{}, const_cast<LongLivedObject*>(o)};
+  decltype(collection_)::node_type retired;
+  {
+    std::scoped_lock lock(collectionMutex_);
+    // Empty owner: lookup-only alias, with no ownership or control block.
+    const std::shared_ptr<LongLivedObject> lookupKey{
+        std::shared_ptr<LongLivedObject>{}, const_cast<LongLivedObject*>(o)};
 
-  // A shared_ptr hashes and compares by the pointer it stores, so a
-  // non-owning shared_ptr to `o` finds its entry without walking the set.
-  auto entry = collection_.find(lookupKey);
-  if (entry != collection_.end()) {
-    collection_.erase(entry);
+    // A shared_ptr hashes and compares by the pointer it stores, so a
+    // non-owning shared_ptr to `o` finds its entry without walking the set.
+    auto entry = collection_.find(lookupKey);
+    if (entry != collection_.end()) {
+      retired = collection_.extract(entry);
+    }
   }
 }
 
 void LongLivedObjectCollection::clear() {
-  std::scoped_lock lock(collectionMutex_);
-  collection_.clear();
+  decltype(collection_) retired;
+  {
+    std::scoped_lock lock(collectionMutex_);
+    retired.swap(collection_);
+  }
 }
 
 size_t LongLivedObjectCollection::size() const {
@@ -64,4 +72,22 @@ void LongLivedObject::allowRelease() {
   LongLivedObjectCollection::get(runtime_).remove(this);
 }
 
+void detail::releaseLongLivedObjectOnJS(
+    std::weak_ptr<LongLivedObject> object,
+    std::weak_ptr<CallInvoker> jsInvoker) noexcept {
+  if (object.expired()) {
+    return;
+  }
+  if (auto invoker = jsInvoker.lock()) {
+    invoker->invokeAsync([object = std::move(object)](jsi::Runtime&) {
+      if (auto locked = object.lock()) {
+        locked->allowRelease();
+      }
+    });
+  }
+  // If dispatch is dropped, runtime cleanup releases the collection. Never
+  // fall back to acquiring or releasing a strong reference on the caller
+  // thread.
+}
+
 } // namespace facebook::react