Do not create RuntimeExecutor on non-JSI executors (facebook#38125)

Summary: Pull Request resolved: facebook#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: 0542da1aca5d411964e3a0833f4c48428ed90c91
lunaleaps · Jun 30, 2023 · ea93ab5 · ea93ab5
1 parent 73293f2
commit ea93ab5
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 32 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
@@ -418,8 +418,11 @@ CatalystInstanceImpl::getNativeCallInvokerHolder() {
 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_;
 }
@@ -428,15 +431,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
@@ -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
@@ -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>
 
@@ -212,26 +213,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<CallInvoker> Instance::getDecoratedNativeCallInvoker(

diff --git a/packages/react-native/ReactCommon/cxxreact/React-cxxreact.podspec b/packages/react-native/ReactCommon/cxxreact/React-cxxreact.podspec
@@ -33,7 +33,7 @@ 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\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-runtimeexecutor/React_runtimeexecutor.framework/Headers\"",
+  s.pod_target_xcconfig    = { "HEADER_SEARCH_PATHS" => "\"$(PODS_ROOT)/boost\" \"$(PODS_ROOT)/RCT-Folly\" \"$(PODS_ROOT)/DoubleConversion\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-runtimeexecutor/React_runtimeexecutor.framework/Headers\" \"$(PODS_CONFIGURATION_BUILD_DIR)/React-debug/React_debug.framework/Headers\"",
                                "CLANG_CXX_LANGUAGE_STANDARD" => "c++17" }
   s.header_dir             = "cxxreact"
 
@@ -47,6 +47,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'