From 7aeacc4e05c52e2e0511098aa53cdfd753a5c240 Mon Sep 17 00:00:00 2001 From: Lulu Wu Date: Wed, 13 Sep 2023 06:06:16 -0700 Subject: [PATCH] Implement "tickleJS" for Hermes (#39289) Summary: "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 --- .../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 4095d9cd605113..1de9439dc87024 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 fc93eea6190726..db12cfc4d5fcb8 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 05905b84121013..832b8aa6f2ccf8 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 0eab4312983717..02e384cac712dd 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 2c8bfe03b08566..30cf7025c96eaf 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 0538d5e0434b3a..670ec2f1d01a0a 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 b176edc6820bcc..63f3f73ba42f84 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 24047b56616d87..0f3722443945dc 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 681114d9af7fc3..04335ccd9c2a13 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 @@ { } -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 aa6fdb78d1170b..027e24b182ba9b 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)_start // 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 1a1e041c9073fd..bcb1393519d1b8 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 b270ba8d27e2cd..168ddc3aa1228c 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 @@ RCTJscInstance::RCTJscInstance() {} -std::unique_ptr RCTJscInstance::createJSRuntime() noexcept +std::unique_ptr RCTJscInstance::createJSRuntime( + std::shared_ptr msgQueueThread) noexcept { return jsc::makeJSCRuntime(); }