Fix callback and promise teardown races (#59670)

Summary:

## Context
- Native callback and promise cleanup can race JavaScript VM teardown. Construction-thread identity is unsafe because execution can temporarily move to another thread.

## This diff
- Keep weak native handles to callback and promise values rooted by the runtime.
- Always defer implicit release; permit inline release only with explicit, current exclusive runtime access.
- Release teardown roots and one-shot synchronous callbacks before their runtime-access scope ends.

## Compatibility
- Native handle construction requires a dispatch context, and typed calls require that context to remain live. Teardown must clear rooted values before destroying the runtime.

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 the legacy callback wrapper in favour of weak lifetime handles

Co-authored-by: HyunWoo Lee <l2hyunwoo@gmail.com>

Differential Revision: D128581452

Pull Request resolved: #59670

Author: javache

packages/react-native/ReactCommon/callinvoker/ReactCommon/tests/TestCallInvoker.h

@@ -30,8 +30,10 @@ class TestCallInvoker : public CallInvoker {
   void flushQueue()
   {
     while (!queue_.empty()) {
-      queue_.front()(runtime_);
+      auto job = std::move(queue_.front());
       queue_.pop_front();
+      job(runtime_);
+      job = {};
       runtime_.drainMicrotasks(); // Run microtasks every cycle.
     }
   }

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,18 @@ 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()) {
+      // The queued function owns the tuple; its lifetime is opaque to the analyzer.
+      // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
+      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,56 +81,46 @@ 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 &) {
+        if (!callback->function_.expired()) {
+          auto &rt = *callback->runtime_;
+          if (auto function = callback->function_.lockJSValue(rt)) {
+            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 with exclusive JavaScript runtime access. Use AsyncCallback otherwise.
 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)))
+      : runtime_(&rt), 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;
+  SyncCallback(SyncCallback &&) noexcept = default;
+  SyncCallback &operator=(SyncCallback &&) noexcept = default;
+  ~SyncCallback() = default;
 
-  // Allow move
-  SyncCallback(SyncCallback &&other) noexcept : wrapper_(std::move(other.wrapper_)) {}
-
-  SyncCallback &operator=(SyncCallback &&other) noexcept
+  void reset(jsi::Runtime &runtime) noexcept
   {
-    wrapper_ = std::move(other.wrapper_);
-    return *this;
-  }
-
-  ~SyncCallback()
-  {
-    if (auto wrapper = wrapper_.lock()) {
-      wrapper->destroy();
-    }
+    function_.reset(runtime);
   }
 
   R operator()(Args... args) const
@@ -138,21 +130,23 @@ class SyncCallback<R(Args...)> {
 
   R call(Args... args) const
   {
-    auto wrapper = wrapper_.lock();
+    auto jsInvoker = function_.lockInvoker();
+    auto function = jsInvoker ? function_.lockJSValue(*runtime_) : nullptr;
 
-    // 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 = *runtime_;
 
     if constexpr (std::is_void_v<R>) {
       callback.call(rt, bridging::toJs(rt, std::forward<Args>(args), jsInvoker)...);
@@ -177,8 +171,8 @@ 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_;
+  jsi::Runtime *runtime_;
+  LongLivedObjectHandle<jsi::Function> function_;
 };
 
 template <typename... Args>
@@ -191,7 +185,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 +199,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_.lockJSValue(rt)) {
+      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,35 @@ 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)};
-
-  // 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);
+  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>{},
+        // Lookup never dereferences or mutates the object.
+        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast)
+        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()) {
+      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_);
+  }
+}
+
+void LongLivedObjectCollection::clear(jsi::Runtime& /*runtime*/) {
+  clear();
 }
 
 size_t LongLivedObjectCollection::size() const {
@@ -64,4 +79,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