From a43e666a341b118b7d3da3d069f297fa4738f1c4 Mon Sep 17 00:00:00 2001 From: Kevin Gozali Date: Fri, 29 Mar 2019 01:31:31 -0700 Subject: [PATCH] TM iOS: force flush message queue when calling into JS from native Summary: When calling into JS (e.g. promise resolve/reject, callback) in TurboModule, we bypass the bridge's message queue. At times this causes race condition, where there are a bunch of pending UI operations (in RCTUImanager) waiting to be flushed, but nothing adds calls to the message queue. Usually tapping the screen will trigger the flush because we're sending down touch events to JS. Reviewed By: JoshuaGross Differential Revision: D14656466 fbshipit-source-id: cb3a174e97542bf80f0a37b4170b6a8e6780fa35 --- React/CxxBridge/RCTCxxBridge.mm | 5 +++++ ReactCommon/cxxreact/Instance.cpp | 7 +++++++ ReactCommon/cxxreact/Instance.h | 2 ++ ReactCommon/cxxreact/JSExecutor.h | 2 ++ ReactCommon/cxxreact/NativeToJsBridge.h | 3 ++- ReactCommon/jsiexecutor/jsireact/JSIExecutor.h | 3 ++- ReactCommon/turbomodule/core/JSCallInvoker.cpp | 15 +++++++-------- ReactCommon/turbomodule/core/JSCallInvoker.h | 11 ++++++----- .../core/platform/ios/RCTTurboModule.h | 3 +++ .../core/platform/ios/RCTTurboModuleManager.mm | 2 +- 10 files changed, 37 insertions(+), 16 deletions(-) diff --git a/React/CxxBridge/RCTCxxBridge.mm b/React/CxxBridge/RCTCxxBridge.mm index 6f0aeed7c..cea1e0d84 100644 --- a/React/CxxBridge/RCTCxxBridge.mm +++ b/React/CxxBridge/RCTCxxBridge.mm @@ -210,6 +210,11 @@ struct RCTInstanceCallback : public InstanceCallback { return _jsMessageThread; } +- (std::shared_ptr)reactInstance +{ + return _reactInstance; +} + - (BOOL)isInspectable { return _reactInstance ? _reactInstance->isInspectable() : NO; diff --git a/ReactCommon/cxxreact/Instance.cpp b/ReactCommon/cxxreact/Instance.cpp index a9b6bd562..ad5069238 100644 --- a/ReactCommon/cxxreact/Instance.cpp +++ b/ReactCommon/cxxreact/Instance.cpp @@ -180,5 +180,12 @@ void Instance::handleMemoryPressure(int pressureLevel) { nativeToJsBridge_->handleMemoryPressure(pressureLevel); } +void Instance::invokeAsync(std::function&& func) { + nativeToJsBridge_->runOnExecutorQueue([func=std::move(func)](JSExecutor *executor) { + func(); + executor->flush(); + }); +} + } // namespace react } // namespace facebook diff --git a/ReactCommon/cxxreact/Instance.h b/ReactCommon/cxxreact/Instance.h index 72a5a7ee8..b72729660 100644 --- a/ReactCommon/cxxreact/Instance.h +++ b/ReactCommon/cxxreact/Instance.h @@ -71,6 +71,8 @@ public: void handleMemoryPressure(int pressureLevel); + void invokeAsync(std::function&& func); + private: void callNativeModules(folly::dynamic &&calls, bool isEndOfBatch); void loadApplication(std::unique_ptr bundleRegistry, diff --git a/ReactCommon/cxxreact/JSExecutor.h b/ReactCommon/cxxreact/JSExecutor.h index f7cfd9752..bc2eb7d4e 100644 --- a/ReactCommon/cxxreact/JSExecutor.h +++ b/ReactCommon/cxxreact/JSExecutor.h @@ -108,6 +108,8 @@ public: virtual void destroy() {} virtual ~JSExecutor() {} + virtual void flush() {} + static std::string getSyntheticBundlePath( uint32_t bundleId, const std::string& bundlePath); diff --git a/ReactCommon/cxxreact/NativeToJsBridge.h b/ReactCommon/cxxreact/NativeToJsBridge.h index cc30054ac..c80114230 100644 --- a/ReactCommon/cxxreact/NativeToJsBridge.h +++ b/ReactCommon/cxxreact/NativeToJsBridge.h @@ -83,9 +83,10 @@ public: * Synchronously tears down the bridge and the main executor. */ void destroy(); -private: + void runOnExecutorQueue(std::function task); +private: // This is used to avoid a race condition where a proxyCallback gets queued // after ~NativeToJsBridge(), on the same thread. In that case, the callback // will try to run the task on m_callback which will have been destroyed diff --git a/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h b/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h index 1eea5d5bc..39c692f97 100644 --- a/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h +++ b/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h @@ -103,10 +103,11 @@ class JSIExecutor : public JSExecutor { invokee(); } + void flush() override; + private: class NativeModuleProxy; - void flush(); void bindBridge(); void callNativeModules(const jsi::Value &queue, bool isEndOfBatch); jsi::Value nativeCallSyncHook(const jsi::Value *args, size_t count); diff --git a/ReactCommon/turbomodule/core/JSCallInvoker.cpp b/ReactCommon/turbomodule/core/JSCallInvoker.cpp index ec28ce1c3..848e9de90 100644 --- a/ReactCommon/turbomodule/core/JSCallInvoker.cpp +++ b/ReactCommon/turbomodule/core/JSCallInvoker.cpp @@ -7,20 +7,19 @@ #include "JSCallInvoker.h" -#include +#include namespace facebook { namespace react { -JSCallInvoker::JSCallInvoker(std::shared_ptr jsThread) - : jsThread_(jsThread) {} +JSCallInvoker::JSCallInvoker(std::shared_ptr reactInstance) + : reactInstance_(reactInstance) {} void JSCallInvoker::invokeAsync(std::function&& func) { - jsThread_->runOnQueue(std::move(func)); -} - -void JSCallInvoker::invokeSync(std::function&& func) { - jsThread_->runOnQueueSync(std::move(func)); + if (reactInstance_ == nullptr) { + return; + } + reactInstance_->invokeAsync(std::move(func)); } } // namespace react diff --git a/ReactCommon/turbomodule/core/JSCallInvoker.h b/ReactCommon/turbomodule/core/JSCallInvoker.h index 1f84bbc02..eff18fa87 100644 --- a/ReactCommon/turbomodule/core/JSCallInvoker.h +++ b/ReactCommon/turbomodule/core/JSCallInvoker.h @@ -13,25 +13,26 @@ namespace facebook { namespace react { -class MessageQueueThread; +class Instance; /** * A generic native-to-JS call invoker. It guarantees that any calls from any * thread are queued on the right JS thread. * - * For now, this is a thin-wrapper around existing MessageQueueThread. Eventually, + * For now, this is a thin-wrapper around existing bridge (`Instance`). Eventually, * it should be consolidated with Fabric implementation so there's only one * API to call JS from native, whether synchronously or asynchronously. + * Also, this class should not depend on `Instance` in the future. */ class JSCallInvoker { public: - JSCallInvoker(std::shared_ptr jsThread); + JSCallInvoker(std::shared_ptr reactInstance); void invokeAsync(std::function&& func); - void invokeSync(std::function&& func); + // TODO: add sync support private: - std::shared_ptr jsThread_; + std::shared_ptr reactInstance_; }; } // namespace react diff --git a/ReactCommon/turbomodule/core/platform/ios/RCTTurboModule.h b/ReactCommon/turbomodule/core/platform/ios/RCTTurboModule.h index da78e21e1..d63d69222 100644 --- a/ReactCommon/turbomodule/core/platform/ios/RCTTurboModule.h +++ b/ReactCommon/turbomodule/core/platform/ios/RCTTurboModule.h @@ -24,6 +24,8 @@ namespace facebook { namespace react { +class Instance; + /** * ObjC++ specific TurboModule base class. */ @@ -89,5 +91,6 @@ private: @interface RCTBridge () - (std::shared_ptr)jsMessageThread; +- (std::shared_ptr)reactInstance; @end diff --git a/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm b/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm index e6111517b..1a5e001ae 100644 --- a/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm +++ b/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm @@ -51,7 +51,7 @@ static Class getFallbackClassFromName(const char *name) { { if (self = [super init]) { _runtime = runtime; - _jsInvoker = std::make_shared(bridge.jsMessageThread); + _jsInvoker = std::make_shared(bridge.reactInstance); _delegate = delegate; _bridge = bridge;