From 95d09d32fee484cee3b75d1ae67f050302e84065 Mon Sep 17 00:00:00 2001
From: Pieter De Baets <pieterdb@meta.com>
Date: Fri, 13 Oct 2023 03:33:39 -0700
Subject: [PATCH] Fix unsafe async call in CatalystInstance callback (#40861)

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

Reference `jobj_` from the async callback is unsafe, as the Java counterpart may have been deallocated by the time it's executed. Instead move the async call to Java.

Note that this method doesn't actually do anything in Fabric, it's used by the old renderer only.

Changelog: [Internal]

Reviewed By: NickGerleman

Differential Revision: D50224974

fbshipit-source-id: c848b177753643febac1d6646d62a27ebace9238
---
 .../react/bridge/CatalystInstanceImpl.java    |  6 +++-
 .../jni/react/jni/CatalystInstanceImpl.cpp    | 28 +++++--------------
 .../main/jni/react/jni/CatalystInstanceImpl.h |  1 -
 3 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java
index b1e38b256d5..fb738344710 100644
--- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java
+++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java
@@ -152,6 +152,7 @@ public class CatalystInstanceImpl implements CatalystInstance {
     mJavaScriptContextHolder = new JavaScriptContextHolder(getJavaScriptContext());
   }
 
+  @DoNotStrip
   private static class BridgeCallback implements ReactCallback {
     // We do this so the callback doesn't keep the CatalystInstanceImpl alive.
     // In this case, the callback is held in C++ code, so the GC can't see it
@@ -166,7 +167,10 @@ public class CatalystInstanceImpl implements CatalystInstance {
     public void onBatchComplete() {
       CatalystInstanceImpl impl = mOuter.get();
       if (impl != null) {
-        impl.mNativeModuleRegistry.onBatchComplete();
+        impl.mNativeModulesQueueThread.runOnQueue(
+            () -> {
+              impl.mNativeModuleRegistry.onBatchComplete();
+            });
       }
     }
 
diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp
index 374e1c09c55..8c14b71293e 100644
--- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp
+++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp
@@ -7,12 +7,8 @@
 
 #include "CatalystInstanceImpl.h"
 
-#include <condition_variable>
 #include <fstream>
 #include <memory>
-#include <mutex>
-#include <sstream>
-#include <vector>
 
 #include <ReactCommon/CallInvokerHolder.h>
 #include <cxxreact/CxxNativeModule.h>
@@ -34,7 +30,6 @@
 
 #include <logger/react_native_log.h>
 
-#include "CxxModuleWrapper.h"
 #include "JReactCxxErrorHandler.h"
 #include "JReactSoftExceptionLogger.h"
 #include "JavaScriptExecutorHolder.h"
@@ -47,24 +42,16 @@ namespace facebook::react {
 
 namespace {
 
-class Exception : public jni::JavaClass<Exception> {
- public:
-};
-
 class JInstanceCallback : public InstanceCallback {
  public:
-  explicit JInstanceCallback(
-      alias_ref<ReactCallback::javaobject> jobj,
-      std::shared_ptr<JMessageQueueThread> messageQueueThread)
-      : jobj_(make_global(jobj)),
-        messageQueueThread_(std::move(messageQueueThread)) {}
+  explicit JInstanceCallback(alias_ref<ReactCallback::javaobject> jobj)
+      : jobj_(make_global(jobj)) {}
 
   void onBatchComplete() override {
-    messageQueueThread_->runOnQueue([this] {
-      static auto method = ReactCallback::javaClassStatic()->getMethod<void()>(
-          "onBatchComplete");
-      method(jobj_);
-    });
+    jni::ThreadScope guard;
+    static auto method =
+        ReactCallback::javaClassStatic()->getMethod<void()>("onBatchComplete");
+    method(jobj_);
   }
 
   void incrementPendingJSCalls() override {
@@ -86,7 +73,6 @@ class JInstanceCallback : public InstanceCallback {
 
  private:
   global_ref<ReactCallback::javaobject> jobj_;
-  std::shared_ptr<JMessageQueueThread> messageQueueThread_;
 };
 
 } // namespace
@@ -201,7 +187,7 @@ void CatalystInstanceImpl::initializeBridge(
       moduleMessageQueue_));
 
   instance_->initializeBridge(
-      std::make_unique<JInstanceCallback>(callback, moduleMessageQueue_),
+      std::make_unique<JInstanceCallback>(callback),
       jseh->getExecutorFactory(),
       std::make_unique<JMessageQueueThread>(jsQueue),
       moduleRegistry_);
diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h b/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h
index 7c88f0b6949..f1bbd95afce 100644
--- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h
+++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h
@@ -13,7 +13,6 @@
 #include <ReactCommon/RuntimeExecutor.h>
 #include <fbjni/fbjni.h>
 
-#include "CxxModuleWrapper.h"
 #include "JMessageQueueThread.h"
 #include "JRuntimeExecutor.h"
 #include "JRuntimeScheduler.h"