From 3078097de7edf9a399044634f1d3034c56ce77b5 Mon Sep 17 00:00:00 2001
From: Pieter De Baets <pieterdb@meta.com>
Date: Fri, 30 Jun 2023 05:08:29 -0700
Subject: [PATCH] Do not create RuntimeExecutor on non-JSI executors (#38125)

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

On Android, when using the remote debugging feature (using legacy websockets), it's not safe to assume we can get a `jsi::Runtime` from `JSExecutor`.

Changelog: [General][Fixed] Android does't crash when using remote debugger

Reviewed By: NickGerleman

Differential Revision: D47124234

fbshipit-source-id: c178915af327604d12c48b1b472ad624a7b75e94
---
 .../jni/react/jni/CatalystInstanceImpl.cpp    | 26 ++++++-----
 .../ReactCommon/cxxreact/CMakeLists.txt       |  3 +-
 .../ReactCommon/cxxreact/Instance.cpp         | 44 +++++++++++--------
 .../cxxreact/React-cxxreact.podspec           |  7 ++-
 4 files changed, 47 insertions(+), 33 deletions(-)

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 53688d625d4a19..67c3fdfacfef60 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
@@ -398,8 +398,11 @@ CatalystInstanceImpl::getNativeMethodCallInvokerHolder() {
 jni::alias_ref<JRuntimeExecutor::javaobject>
 CatalystInstanceImpl::getRuntimeExecutor() {
   if (!runtimeExecutor_) {
-    runtimeExecutor_ = jni::make_global(
-        JRuntimeExecutor::newObjectCxxArgs(instance_->getRuntimeExecutor()));
+    auto executor = instance_->getRuntimeExecutor();
+    if (executor) {
+      runtimeExecutor_ =
+          jni::make_global(JRuntimeExecutor::newObjectCxxArgs(executor));
+    }
   }
   return runtimeExecutor_;
 }
@@ -408,15 +411,16 @@ jni::alias_ref<JRuntimeScheduler::javaobject>
 CatalystInstanceImpl::getRuntimeScheduler() {
   if (!runtimeScheduler_) {
     auto runtimeExecutor = instance_->getRuntimeExecutor();
-    auto runtimeScheduler = std::make_shared<RuntimeScheduler>(runtimeExecutor);
-
-    runtimeScheduler_ =
-        jni::make_global(JRuntimeScheduler::newObjectCxxArgs(runtimeScheduler));
-
-    runtimeExecutor([runtimeScheduler](jsi::Runtime &runtime) {
-      RuntimeSchedulerBinding::createAndInstallIfNeeded(
-          runtime, runtimeScheduler);
-    });
+    if (runtimeExecutor) {
+      auto runtimeScheduler =
+          std::make_shared<RuntimeScheduler>(runtimeExecutor);
+      runtimeScheduler_ = jni::make_global(
+          JRuntimeScheduler::newObjectCxxArgs(runtimeScheduler));
+      runtimeExecutor([scheduler =
+                           std::move(runtimeScheduler)](jsi::Runtime &runtime) {
+        RuntimeSchedulerBinding::createAndInstallIfNeeded(runtime, scheduler);
+      });
+    }
   }
 
   return runtimeScheduler_;
diff --git a/packages/react-native/ReactCommon/cxxreact/CMakeLists.txt b/packages/react-native/ReactCommon/cxxreact/CMakeLists.txt
index 2aa8c299dc1c38..d1316d7deaa023 100644
--- a/packages/react-native/ReactCommon/cxxreact/CMakeLists.txt
+++ b/packages/react-native/ReactCommon/cxxreact/CMakeLists.txt
@@ -27,4 +27,5 @@ target_link_libraries(reactnative
         jsinspector
         logger
         reactperflogger
-        runtimeexecutor)
+        runtimeexecutor
+        react_debug)
diff --git a/packages/react-native/ReactCommon/cxxreact/Instance.cpp b/packages/react-native/ReactCommon/cxxreact/Instance.cpp
index 7cf9d3a148eda6..8f75501e61d640 100644
--- a/packages/react-native/ReactCommon/cxxreact/Instance.cpp
+++ b/packages/react-native/ReactCommon/cxxreact/Instance.cpp
@@ -21,6 +21,7 @@
 #include <cxxreact/JSIndexedRAMBundle.h>
 #include <folly/MoveWrapper.h>
 #include <folly/json.h>
+#include <react/debug/react_native_assert.h>
 
 #include <glog/logging.h>
 
@@ -211,26 +212,31 @@ std::shared_ptr<CallInvoker> Instance::getJSCallInvoker() {
 }
 
 RuntimeExecutor Instance::getRuntimeExecutor() {
-  std::weak_ptr<NativeToJsBridge> weakNativeToJsBridge = nativeToJsBridge_;
+  // HACK: RuntimeExecutor is not compatible with non-JSIExecutor, we return
+  // a null callback, which the caller should handle.
+  if (!getJavaScriptContext()) {
+    return nullptr;
+  }
 
-  auto runtimeExecutor =
-      [weakNativeToJsBridge](
-          std::function<void(jsi::Runtime & runtime)> &&callback) {
-        if (auto strongNativeToJsBridge = weakNativeToJsBridge.lock()) {
-          strongNativeToJsBridge->runOnExecutorQueue(
-              [callback = std::move(callback)](JSExecutor *executor) {
-                jsi::Runtime *runtime =
-                    (jsi::Runtime *)executor->getJavaScriptContext();
-                try {
-                  callback(*runtime);
-                  executor->flush();
-                } catch (jsi::JSError &originalError) {
-                  handleJSError(*runtime, originalError, true);
-                }
-              });
-        }
-      };
-  return runtimeExecutor;
+  std::weak_ptr<NativeToJsBridge> weakNativeToJsBridge = nativeToJsBridge_;
+  return [weakNativeToJsBridge](
+             std::function<void(jsi::Runtime & runtime)> &&callback) {
+    if (auto strongNativeToJsBridge = weakNativeToJsBridge.lock()) {
+      strongNativeToJsBridge->runOnExecutorQueue(
+          [callback = std::move(callback)](JSExecutor *executor) {
+            // Assumes the underlying executor is a JSIExecutor
+            jsi::Runtime *runtime =
+                (jsi::Runtime *)executor->getJavaScriptContext();
+            try {
+              react_native_assert(runtime != nullptr);
+              callback(*runtime);
+              executor->flush();
+            } catch (jsi::JSError &originalError) {
+              handleJSError(*runtime, originalError, true);
+            }
+          });
+    }
+  };
 }
 
 std::shared_ptr<NativeMethodCallInvoker>
diff --git a/packages/react-native/ReactCommon/cxxreact/React-cxxreact.podspec b/packages/react-native/ReactCommon/cxxreact/React-cxxreact.podspec
index fe1b09b0a59362..fa7eb6125ee51c 100644
--- a/packages/react-native/ReactCommon/cxxreact/React-cxxreact.podspec
+++ b/packages/react-native/ReactCommon/cxxreact/React-cxxreact.podspec
@@ -33,8 +33,10 @@ Pod::Spec.new do |s|
   s.source_files           = "*.{cpp,h}"
   s.exclude_files          = "SampleCxxModule.*"
   s.compiler_flags         = folly_compiler_flags + ' ' + boost_compiler_flags
-  s.pod_target_xcconfig    = { "HEADER_SEARCH_PATHS" => "\"$(PODS_ROOT)/boost\" \"$(PODS_ROOT)/RCT-Folly\" \"$(PODS_ROOT)/DoubleConversion\"",
-                               "CLANG_CXX_LANGUAGE_STANDARD" => "c++17" }
+  s.pod_target_xcconfig    = {
+    "HEADER_SEARCH_PATHS" => "\"$(PODS_ROOT)/boost\" \"$(PODS_ROOT)/RCT-Folly\" \"$(PODS_ROOT)/DoubleConversion\" \"$(PODS_CONFIGURATION_BUILD_DIR)/React-debug/React_debug.framework/Headers\"",
+    "CLANG_CXX_LANGUAGE_STANDARD" => "c++17"
+  }
   s.header_dir             = "cxxreact"
 
   s.dependency "boost", "1.76.0"
@@ -47,6 +49,7 @@ Pod::Spec.new do |s|
   s.dependency "React-perflogger", version
   s.dependency "React-jsi", version
   s.dependency "React-logger", version
+  s.dependency "React-debug", version
 
   if ENV['USE_HERMES'] == nil || ENV['USE_HERMES'] == "1"
     s.dependency 'hermes-engine'