From 43c55e98d493d60f9a05fb4f96efd54d1ea3bce8 Mon Sep 17 00:00:00 2001
From: Pieter De Baets <pieterdb@meta.com>
Date: Mon, 4 Mar 2024 04:11:36 -0800
Subject: [PATCH] Allow moving SyncCallback (#43268)

Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/43268

We previously restricted all copies and moves of SyncCallback, but that led to unsafe calling paths being added instead to AsyncCallback. Instead, allowing moving of SyncCallback, and document the need for the caller to invoke it safely, so can we remove the unsafe path from AsyncCallback.

Changelog: [General][Changed] Allow moving SyncCallback for advanced use-cases

Reviewed By: christophpurrer

Differential Revision: D54381734

fbshipit-source-id: 5fd797cd4541e507aa68f1e4e76a1be5cae20fbe
---
 .../ReactCommon/react/bridging/Function.h     | 28 +++++++++++--------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/packages/react-native/ReactCommon/react/bridging/Function.h b/packages/react-native/ReactCommon/react/bridging/Function.h
index 7fae7fc8575..0760e6abbc4 100644
--- a/packages/react-native/ReactCommon/react/bridging/Function.h
+++ b/packages/react-native/ReactCommon/react/bridging/Function.h
@@ -54,15 +54,6 @@ class AsyncCallback {
     callWithFunction(priority, std::move(callImpl));
   }
 
-  /// Invoke the function write-away as if it was a synchronous function
-  /// without any synchronization or delegating to JS context.
-  /// @note Caller is responsible for calling this from within JS context.
-  void unsafeCallSync(Args... args) const noexcept {
-    if (callback_) {
-      (*callback_)(std::forward<Args>(args)...);
-    }
-  }
-
  private:
   friend Bridging<AsyncCallback>;
 
@@ -110,6 +101,9 @@ class AsyncCallback {
   }
 };
 
+// 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.
 template <typename R, typename... Args>
 class SyncCallback<R(Args...)> {
  public:
@@ -122,9 +116,19 @@ class SyncCallback<R(Args...)> {
             rt,
             std::move(jsInvoker))) {}
 
-  // Disallow moving to prevent function from get called on another thread.
-  SyncCallback(SyncCallback&&) = delete;
-  SyncCallback& operator=(SyncCallback&&) = delete;
+  // 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()) {