From fe4dab970141adf63fdf184d1c05dad31974bace Mon Sep 17 00:00:00 2001 From: Lulu Wu Date: Wed, 13 Sep 2023 13:21:19 -0700 Subject: [PATCH] Implement "tickleJS" for Hermes (#39289) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/39289 "tickleJS" is a function in RN Hermes Debugger, we need to implement it for Bridgeless for feature correctness and keep parity with Bridge. Changelog: [Internal] - Implement "tickleJS" for Hermes in Bridgeless mode ### Summary of discussion: **Goal of this diff**: Implement tickleJS for Hermes debugger with Bridgeless, here is Bridge's implementation https://www.internalfb.com/code/fbsource/[7058fb4dae4f]/xplat/ReactNative/react/jsi/HermesExecutorFactory.cpp?lines=45-57 **The key problem to solve:** As you can see from Bridge's implementation, we need to dispatch the js call of "__tickleJs" to dedicated JS thread queue for thread safety, and it did cause a crash for me locally without dispatching to JS thread queue (see P818439922). For Bridgeless passing the JS message queue directly works, but then we would naturally want ask if RuntimeExecutor/RuntimeScheduler is a better choice because: 1, It should also work 2, It's the common way to run js code in native with Bridgeless 3, Can avoid potential race condition caused by dispatching to JS message queue directly 4, We're planning to expose RuntimeExecutor/RuntimeScheduler or a similar abstraction anyway. So the argument here is which solution should we take? **Why we decide to move forward with the JS msq solution?** - Time constraint: we need to ship this before 0.73 - After discussion there hasn't been any major concern over dispatching to JS msq directly so far - How to expose RuntimeExecutor/RuntimeScheduler is a bigger topic which need further research, we don't want to hold this diff for it For detailed discussion see the post https://fb.workplace.com/groups/react.technologies.discussions/permalink/3615044702060575/ Reviewed By: javache Differential Revision: D48952581 fbshipit-source-id: 3dfb05d35c422f3fd6a420e265e4efe8fa13f057 --- .../runtime/hermes/jni/JHermesInstance.cpp | 5 ++- .../runtime/hermes/jni/JHermesInstance.h | 4 +- .../jni/react/runtime/jni/JReactInstance.cpp | 2 +- .../main/jni/react/runtime/jsc/jni/OnLoad.cpp | 4 +- .../react/runtime/JSEngineInstance.h | 4 +- .../react/runtime/hermes/HermesInstance.cpp | 39 ++++++++++++++----- .../react/runtime/hermes/HermesInstance.h | 7 ++-- .../ios/ReactCommon/RCTHermesInstance.h | 4 +- .../ios/ReactCommon/RCTHermesInstance.mm | 6 ++- .../platform/ios/ReactCommon/RCTInstance.mm | 5 ++- .../platform/ios/ReactCommon/RCTJscInstance.h | 4 +- .../ios/ReactCommon/RCTJscInstance.mm | 3 +- 12 files changed, 61 insertions(+), 26 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/hermes/jni/JHermesInstance.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/hermes/jni/JHermesInstance.cpp index 4095d9cd605..1de9439dc87 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/hermes/jni/JHermesInstance.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/hermes/jni/JHermesInstance.cpp @@ -22,9 +22,10 @@ void JHermesInstance::registerNatives() { }); } -std::unique_ptr JHermesInstance::createJSRuntime() noexcept { +std::unique_ptr JHermesInstance::createJSRuntime( + std::shared_ptr msgQueueThread) noexcept { // TODO T105438175 Pass ReactNativeConfig to init Hermes with MobileConfig - return HermesInstance::createJSRuntime(); + return HermesInstance::createJSRuntime(nullptr, nullptr, msgQueueThread); } } // namespace facebook::react diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/hermes/jni/JHermesInstance.h b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/hermes/jni/JHermesInstance.h index fc93eea6190..db12cfc4d5f 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/hermes/jni/JHermesInstance.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/hermes/jni/JHermesInstance.h @@ -10,6 +10,7 @@ #include #include +#include #include #include #include @@ -29,7 +30,8 @@ class JHermesInstance static void registerNatives(); - std::unique_ptr createJSRuntime() noexcept; + std::unique_ptr createJSRuntime( + std::shared_ptr msgQueueThread) noexcept; ~JHermesInstance() {} diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactInstance.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactInstance.cpp index 05905b84121..832b8aa6f2c 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactInstance.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactInstance.cpp @@ -61,7 +61,7 @@ JReactInstance::JReactInstance( jBindingsInstaller_ = jni::make_global(jBindingsInstaller); instance_ = std::make_unique( - jsEngineInstance->cthis()->createJSRuntime(), + jsEngineInstance->cthis()->createJSRuntime(sharedJSMessageQueueThread), sharedJSMessageQueueThread, timerManager, std::move(jsErrorHandlingFunc)); diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jsc/jni/OnLoad.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jsc/jni/OnLoad.cpp index 0eab4312983..02e384cac71 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jsc/jni/OnLoad.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jsc/jni/OnLoad.cpp @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. */ +#include #include #include #include @@ -29,7 +30,8 @@ class JSCInstance : public jni::HybridClass { }); } - std::unique_ptr createJSRuntime() noexcept { + std::unique_ptr createJSRuntime( + std::shared_ptr msgQueueThread) noexcept { return jsc::makeJSCRuntime(); } diff --git a/packages/react-native/ReactCommon/react/runtime/JSEngineInstance.h b/packages/react-native/ReactCommon/react/runtime/JSEngineInstance.h index 2c8bfe03b08..30cf7025c96 100644 --- a/packages/react-native/ReactCommon/react/runtime/JSEngineInstance.h +++ b/packages/react-native/ReactCommon/react/runtime/JSEngineInstance.h @@ -8,6 +8,7 @@ #pragma once #include +#include #include namespace facebook::react { @@ -17,7 +18,8 @@ namespace facebook::react { */ class JSEngineInstance { public: - virtual std::unique_ptr createJSRuntime() noexcept = 0; + virtual std::unique_ptr createJSRuntime( + std::shared_ptr msgQueueThread) noexcept = 0; virtual ~JSEngineInstance() = default; }; diff --git a/packages/react-native/ReactCommon/react/runtime/hermes/HermesInstance.cpp b/packages/react-native/ReactCommon/react/runtime/hermes/HermesInstance.cpp index 0538d5e0434..670ec2f1d01 100644 --- a/packages/react-native/ReactCommon/react/runtime/hermes/HermesInstance.cpp +++ b/packages/react-native/ReactCommon/react/runtime/hermes/HermesInstance.cpp @@ -37,23 +37,43 @@ namespace facebook::react { // lead to a runtime crash. class HermesInstanceRuntimeAdapter : public inspector_modern::RuntimeAdapter { public: - HermesInstanceRuntimeAdapter(std::shared_ptr hermesRuntime) - : hermesRuntime_(hermesRuntime) {} + HermesInstanceRuntimeAdapter( + std::shared_ptr hermesRuntime, + std::shared_ptr msgQueueThread) + : hermesRuntime_(std::move(hermesRuntime)), + messageQueueThread_(std::move(msgQueueThread)) {} virtual ~HermesInstanceRuntimeAdapter() = default; HermesRuntime& getRuntime() override { return *hermesRuntime_; } + void tickleJs() override { + std::weak_ptr weakRuntime(hermesRuntime_); + messageQueueThread_->runOnQueue([weakRuntime]() { + auto runtime = weakRuntime.lock(); + if (!runtime) { + return; + } + jsi::Function func = + runtime->global().getPropertyAsFunction(*runtime, "__tickleJs"); + func.call(*runtime); + }); + } + private: std::shared_ptr hermesRuntime_; + std::shared_ptr messageQueueThread_; }; class DecoratedRuntime : public jsi::RuntimeDecorator { public: - DecoratedRuntime(std::unique_ptr runtime) + DecoratedRuntime( + std::unique_ptr runtime, + std::shared_ptr msgQueueThread) : RuntimeDecorator(*runtime), runtime_(std::move(runtime)) { - auto adapter = std::make_unique(runtime_); + auto adapter = std::make_unique( + runtime_, msgQueueThread); debugToken_ = inspector_modern::chrome::enableDebugging( std::move(adapter), "Hermes Bridgeless React Native"); @@ -70,13 +90,11 @@ class DecoratedRuntime : public jsi::RuntimeDecorator { #endif -std::unique_ptr HermesInstance::createJSRuntime() noexcept { - return createJSRuntime(nullptr, nullptr); -} - std::unique_ptr HermesInstance::createJSRuntime( std::shared_ptr reactNativeConfig, - std::shared_ptr<::hermes::vm::CrashManager> cm) noexcept { + std::shared_ptr<::hermes::vm::CrashManager> cm, + std::shared_ptr msgQueueThread) noexcept { + assert(msgQueueThread != nullptr); int64_t vmExperimentFlags = reactNativeConfig ? reactNativeConfig->getInt64("ios_hermes:vm_experiment_flags") : 0; @@ -113,7 +131,8 @@ std::unique_ptr HermesInstance::createJSRuntime( #ifdef HERMES_ENABLE_DEBUGGER std::unique_ptr decoratedRuntime = - std::make_unique(std::move(hermesRuntime)); + std::make_unique( + std::move(hermesRuntime), msgQueueThread); return decoratedRuntime; #endif diff --git a/packages/react-native/ReactCommon/react/runtime/hermes/HermesInstance.h b/packages/react-native/ReactCommon/react/runtime/hermes/HermesInstance.h index b176edc6820..63f3f73ba42 100644 --- a/packages/react-native/ReactCommon/react/runtime/hermes/HermesInstance.h +++ b/packages/react-native/ReactCommon/react/runtime/hermes/HermesInstance.h @@ -7,6 +7,7 @@ #pragma once +#include #include #include #include @@ -15,12 +16,10 @@ namespace facebook::react { class HermesInstance { public: - // This is only needed for Android. Consider removing. - static std::unique_ptr createJSRuntime() noexcept; - static std::unique_ptr createJSRuntime( std::shared_ptr reactNativeConfig, - std::shared_ptr<::hermes::vm::CrashManager> cm) noexcept; + std::shared_ptr<::hermes::vm::CrashManager> cm, + std::shared_ptr msgQueueThread) noexcept; }; } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHermesInstance.h b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHermesInstance.h index 24047b56616..0f372244394 100644 --- a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHermesInstance.h +++ b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHermesInstance.h @@ -7,6 +7,7 @@ #import +#import #import #import #import @@ -25,7 +26,8 @@ class RCTHermesInstance : public JSEngineInstance { std::shared_ptr reactNativeConfig, CrashManagerProvider crashManagerProvider); - std::unique_ptr createJSRuntime() noexcept override; + std::unique_ptr createJSRuntime( + std::shared_ptr msgQueueThread) noexcept override; ~RCTHermesInstance(){}; diff --git a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHermesInstance.mm b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHermesInstance.mm index 681114d9af7..04335ccd9c2 100644 --- a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHermesInstance.mm +++ b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHermesInstance.mm @@ -20,10 +20,12 @@ RCTHermesInstance::RCTHermesInstance( { } -std::unique_ptr RCTHermesInstance::createJSRuntime() noexcept +std::unique_ptr RCTHermesInstance::createJSRuntime( + std::shared_ptr msgQueueThread) noexcept { return _hermesInstance->createJSRuntime( - _reactNativeConfig, _crashManagerProvider ? _crashManagerProvider() : nullptr); + _reactNativeConfig, _crashManagerProvider ? _crashManagerProvider() : nullptr, msgQueueThread); } + } // namespace react } // namespace facebook diff --git a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTInstance.mm b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTInstance.mm index aa6fdb78d11..027e24b182b 100644 --- a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTInstance.mm +++ b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTInstance.mm @@ -218,7 +218,10 @@ void RCTInstanceSetRuntimeDiagnosticFlags(NSString *flags) // Create the React Instance _reactInstance = std::make_unique( - _jsEngineInstance->createJSRuntime(), _jsThreadManager.jsMessageThread, timerManager, jsErrorHandlingFunc); + _jsEngineInstance->createJSRuntime(_jsThreadManager.jsMessageThread), + _jsThreadManager.jsMessageThread, + timerManager, + jsErrorHandlingFunc); _valid = true; RuntimeExecutor bufferedRuntimeExecutor = _reactInstance->getBufferedRuntimeExecutor(); diff --git a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTJscInstance.h b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTJscInstance.h index 1a1e041c907..bcb1393519d 100644 --- a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTJscInstance.h +++ b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTJscInstance.h @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. */ +#import #import #import @@ -15,7 +16,8 @@ class RCTJscInstance : public JSEngineInstance { public: RCTJscInstance(); - std::unique_ptr createJSRuntime() noexcept override; + std::unique_ptr createJSRuntime( + std::shared_ptr msgQueueThread) noexcept override; ~RCTJscInstance(){}; }; diff --git a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTJscInstance.mm b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTJscInstance.mm index b270ba8d27e..168ddc3aa12 100644 --- a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTJscInstance.mm +++ b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTJscInstance.mm @@ -13,7 +13,8 @@ namespace react { RCTJscInstance::RCTJscInstance() {} -std::unique_ptr RCTJscInstance::createJSRuntime() noexcept +std::unique_ptr RCTJscInstance::createJSRuntime( + std::shared_ptr msgQueueThread) noexcept { return jsc::makeJSCRuntime(); }