From c80192c2ab6ab642ba64be86071711894f263090 Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Fri, 30 Aug 2019 18:21:59 -0700 Subject: [PATCH] Fabric: EventBeat::Owner to help with crashes Summary: The purpose of `EventBeat` is handling an asynchronous callback to itself which is being delivered on some different thread. That brings a challenge of ensuring that the `EventBeat` object stays valid during the timeframe of callback execution. The concept of Owner helps with that. The owner is a shared pointer that retains (probably indirectly) the `EventBeat` object. To ensure the correctness of the call, `EventBeat` retains the owner (practically creating a retain cycle) during executing the callback. In case if the pointer to the owner already null, `EventBeat` skips executing the callback. It's impossible to retain itself directly or refer to the shared pointer to itself from a constructor. `OwnerBox` is designed to work around this issue; it allows to store the pointer later, right after the creation of some other object that owns an `EventBeat`. Reviewed By: JoshuaGross Differential Revision: D17128549 fbshipit-source-id: 7ed34fd865430975157fd362f51c4a3d64214430 --- React/Fabric/RCTSurfacePresenter.mm | 8 ++--- React/Fabric/Utils/MainRunLoopEventBeat.h | 4 ++- React/Fabric/Utils/MainRunLoopEventBeat.mm | 10 ++++-- React/Fabric/Utils/RuntimeEventBeat.h | 4 ++- React/Fabric/Utils/RuntimeEventBeat.mm | 10 ++++-- .../react/fabric/jni/AsyncEventBeat.h | 2 ++ .../com/facebook/react/fabric/jni/Binding.cpp | 12 +++---- ReactCommon/fabric/core/events/EventBeat.cpp | 2 ++ ReactCommon/fabric/core/events/EventBeat.h | 31 +++++++++++++++++-- .../fabric/core/events/EventDispatcher.cpp | 19 ++++++------ .../fabric/core/events/EventDispatcher.h | 17 +++++----- ReactCommon/fabric/uimanager/Scheduler.cpp | 13 ++++---- .../fabric/uimanager/SchedulerToolbox.h | 4 +-- 13 files changed, 91 insertions(+), 45 deletions(-) diff --git a/React/Fabric/RCTSurfacePresenter.mm b/React/Fabric/RCTSurfacePresenter.mm index c3a1823b53d..87a078abe88 100644 --- a/React/Fabric/RCTSurfacePresenter.mm +++ b/React/Fabric/RCTSurfacePresenter.mm @@ -208,12 +208,12 @@ using namespace facebook::react; toolbox.componentRegistryFactory = componentRegistryFactory; toolbox.runtimeExecutor = runtimeExecutor; - toolbox.synchronousEventBeatFactory = [runtimeExecutor]() { - return std::make_unique(runtimeExecutor); + toolbox.synchronousEventBeatFactory = [runtimeExecutor](EventBeat::SharedOwnerBox const &ownerBox) { + return std::make_unique(ownerBox, runtimeExecutor); }; - toolbox.asynchronousEventBeatFactory = [runtimeExecutor]() { - return std::make_unique(runtimeExecutor); + toolbox.asynchronousEventBeatFactory = [runtimeExecutor](EventBeat::SharedOwnerBox const &ownerBox) { + return std::make_unique(ownerBox, runtimeExecutor); }; _scheduler = [[RCTScheduler alloc] initWithToolbox:toolbox]; diff --git a/React/Fabric/Utils/MainRunLoopEventBeat.h b/React/Fabric/Utils/MainRunLoopEventBeat.h index 8b78ffaa4fd..49eb4cb863e 100644 --- a/React/Fabric/Utils/MainRunLoopEventBeat.h +++ b/React/Fabric/Utils/MainRunLoopEventBeat.h @@ -19,7 +19,9 @@ namespace react { */ class MainRunLoopEventBeat final : public EventBeat { public: - MainRunLoopEventBeat(RuntimeExecutor runtimeExecutor); + MainRunLoopEventBeat( + EventBeat::SharedOwnerBox const &ownerBox, + RuntimeExecutor runtimeExecutor); ~MainRunLoopEventBeat(); void induce() const override; diff --git a/React/Fabric/Utils/MainRunLoopEventBeat.mm b/React/Fabric/Utils/MainRunLoopEventBeat.mm index 1585e28a2a5..2e158a6b5b3 100644 --- a/React/Fabric/Utils/MainRunLoopEventBeat.mm +++ b/React/Fabric/Utils/MainRunLoopEventBeat.mm @@ -11,8 +11,8 @@ namespace facebook { namespace react { -MainRunLoopEventBeat::MainRunLoopEventBeat(RuntimeExecutor runtimeExecutor) - : runtimeExecutor_(std::move(runtimeExecutor)) +MainRunLoopEventBeat::MainRunLoopEventBeat(EventBeat::SharedOwnerBox const &ownerBox, RuntimeExecutor runtimeExecutor) + : EventBeat(ownerBox), runtimeExecutor_(std::move(runtimeExecutor)) { mainRunLoopObserver_ = CFRunLoopObserverCreateWithHandler( NULL /* allocator */, @@ -51,9 +51,13 @@ void MainRunLoopEventBeat::induce() const void MainRunLoopEventBeat::lockExecutorAndBeat() const { + auto owner = ownerBox_->owner.lock(); + if (!owner) { + return; + } + // Note: We need the third mutex to get back to the main thread before // the lambda is finished (because all mutexes are allocated on the stack). - std::mutex mutex1; std::mutex mutex2; std::mutex mutex3; diff --git a/React/Fabric/Utils/RuntimeEventBeat.h b/React/Fabric/Utils/RuntimeEventBeat.h index 67ba74fdb59..446b798351d 100644 --- a/React/Fabric/Utils/RuntimeEventBeat.h +++ b/React/Fabric/Utils/RuntimeEventBeat.h @@ -20,7 +20,9 @@ namespace react { */ class RuntimeEventBeat : public EventBeat { public: - RuntimeEventBeat(RuntimeExecutor runtimeExecutor); + RuntimeEventBeat( + EventBeat::SharedOwnerBox const &ownerBox, + RuntimeExecutor runtimeExecutor); ~RuntimeEventBeat(); void induce() const override; diff --git a/React/Fabric/Utils/RuntimeEventBeat.mm b/React/Fabric/Utils/RuntimeEventBeat.mm index 77b6b14415b..3adb4d6a2c3 100644 --- a/React/Fabric/Utils/RuntimeEventBeat.mm +++ b/React/Fabric/Utils/RuntimeEventBeat.mm @@ -10,7 +10,8 @@ namespace facebook { namespace react { -RuntimeEventBeat::RuntimeEventBeat(RuntimeExecutor runtimeExecutor) : runtimeExecutor_(std::move(runtimeExecutor)) +RuntimeEventBeat::RuntimeEventBeat(EventBeat::SharedOwnerBox const &ownerBox, RuntimeExecutor runtimeExecutor) + : EventBeat(ownerBox), runtimeExecutor_(std::move(runtimeExecutor)) { mainRunLoopObserver_ = CFRunLoopObserverCreateWithHandler( NULL /* allocator */, @@ -41,7 +42,12 @@ void RuntimeEventBeat::induce() const } isBusy_ = true; - runtimeExecutor_([=](jsi::Runtime &runtime) mutable { + runtimeExecutor_([this, ownerBox = ownerBox_](jsi::Runtime &runtime) mutable { + auto owner = ownerBox->owner.lock(); + if (!owner) { + return; + } + this->beat(runtime); isBusy_ = false; }); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeat.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeat.h index cdd78cfb2e1..b0811831704 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeat.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeat.h @@ -25,9 +25,11 @@ class AsyncEventBeat : public EventBeat { friend class EventBeatManager; AsyncEventBeat( + EventBeat::SharedOwnerBox const &ownerBox, EventBeatManager* eventBeatManager, RuntimeExecutor runtimeExecutor, jni::global_ref javaUIManager) : + EventBeat(ownerBox), eventBeatManager_(eventBeatManager), runtimeExecutor_(std::move(runtimeExecutor)), javaUIManager_(javaUIManager) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp index 2a7dfede65a..a3ca8d369b3 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp @@ -206,16 +206,16 @@ void Binding::installFabricUIManager( // TODO: T31905686 Create synchronous Event Beat jni::global_ref localJavaUIManager = javaUIManager_; - EventBeatFactory synchronousBeatFactory = - [eventBeatManager, runtimeExecutor, localJavaUIManager]() { + EventBeat::Factory synchronousBeatFactory = + [eventBeatManager, runtimeExecutor, localJavaUIManager](EventBeat::SharedOwnerBox const &ownerBox) { return std::make_unique( - eventBeatManager, runtimeExecutor, localJavaUIManager); + ownerBox, eventBeatManager, runtimeExecutor, localJavaUIManager); }; - EventBeatFactory asynchronousBeatFactory = - [eventBeatManager, runtimeExecutor, localJavaUIManager]() { + EventBeat::Factory asynchronousBeatFactory = + [eventBeatManager, runtimeExecutor, localJavaUIManager](EventBeat::SharedOwnerBox const &ownerBox) { return std::make_unique( - eventBeatManager, runtimeExecutor, localJavaUIManager); + ownerBox, eventBeatManager, runtimeExecutor, localJavaUIManager); }; std::shared_ptr config = diff --git a/ReactCommon/fabric/core/events/EventBeat.cpp b/ReactCommon/fabric/core/events/EventBeat.cpp index d16cfa36c75..c99c5696473 100644 --- a/ReactCommon/fabric/core/events/EventBeat.cpp +++ b/ReactCommon/fabric/core/events/EventBeat.cpp @@ -10,6 +10,8 @@ namespace facebook { namespace react { +EventBeat::EventBeat(SharedOwnerBox const &ownerBox) : ownerBox_(ownerBox) {} + void EventBeat::request() const { isRequested_ = true; } diff --git a/ReactCommon/fabric/core/events/EventBeat.h b/ReactCommon/fabric/core/events/EventBeat.h index f0c88f15d04..13ffb90c31e 100644 --- a/ReactCommon/fabric/core/events/EventBeat.h +++ b/ReactCommon/fabric/core/events/EventBeat.h @@ -21,10 +21,36 @@ namespace react { */ class EventBeat { public: - virtual ~EventBeat() = default; + /* + * The concept of `Owner` + * The purpose of `EventBeat` is handling an asynchronous callback to itself + * which is being delivered on some different thread. That brings a challenge + * of ensuring that the `EventBeat` object stays valid during the timeframe of + * callback execution. The concept of Owner helps with that. The owner is a + * shared pointer that retains (probably indirectly) the `EventBeat` object. + * To ensure the correctness of the call, `EventBeat` retains the owner + * (practically creating a retain cycle) during executing the callback. In + * case if the pointer to the owner already null, `EventBeat` skips executing + * the callback. It's impossible to retain itself directly or refer to the + * shared pointer to itself from a constructor. `OwnerBox` is designed to work + * around this issue; it allows to store the pointer later, right after the + * creation of some other object that owns an `EventBeat`. + */ + using Owner = std::weak_ptr; + struct OwnerBox { + Owner owner; + }; + using SharedOwnerBox = std::shared_ptr; + + using Factory = + std::function(SharedOwnerBox const &ownerBox)>; using BeatCallback = std::function; + EventBeat(SharedOwnerBox const &ownerBox); + + virtual ~EventBeat() = default; + /* * Communicates to the Beat that a consumer is waiting for the coming beat. * A consumer must request coming beat after the previous beat happened @@ -57,10 +83,9 @@ class EventBeat { protected: BeatCallback beatCallback_; + SharedOwnerBox ownerBox_; mutable std::atomic isRequested_{false}; }; -using EventBeatFactory = std::function()>; - } // namespace react } // namespace facebook diff --git a/ReactCommon/fabric/core/events/EventDispatcher.cpp b/ReactCommon/fabric/core/events/EventDispatcher.cpp index 00c7dbd5a32..9edafb7c091 100644 --- a/ReactCommon/fabric/core/events/EventDispatcher.cpp +++ b/ReactCommon/fabric/core/events/EventDispatcher.cpp @@ -19,33 +19,34 @@ namespace facebook { namespace react { EventDispatcher::EventDispatcher( - const EventPipe &eventPipe, - const StatePipe &statePipe, - const EventBeatFactory &synchonousEventBeatFactory, - const EventBeatFactory &asynchonousEventBeatFactory) { + EventPipe const &eventPipe, + StatePipe const &statePipe, + EventBeat::Factory const &synchonousEventBeatFactory, + EventBeat::Factory const &asynchonousEventBeatFactory, + EventBeat::SharedOwnerBox const &ownerBox) { // Synchronous/Unbatched eventQueues_[(int)EventPriority::SynchronousUnbatched] = std::make_unique( - eventPipe, statePipe, synchonousEventBeatFactory()); + eventPipe, statePipe, synchonousEventBeatFactory(ownerBox)); // Synchronous/Batched eventQueues_[(int)EventPriority::SynchronousBatched] = std::make_unique( - eventPipe, statePipe, synchonousEventBeatFactory()); + eventPipe, statePipe, synchonousEventBeatFactory(ownerBox)); // Asynchronous/Unbatched eventQueues_[(int)EventPriority::AsynchronousUnbatched] = std::make_unique( - eventPipe, statePipe, asynchonousEventBeatFactory()); + eventPipe, statePipe, asynchonousEventBeatFactory(ownerBox)); // Asynchronous/Batched eventQueues_[(int)EventPriority::AsynchronousBatched] = std::make_unique( - eventPipe, statePipe, asynchonousEventBeatFactory()); + eventPipe, statePipe, asynchonousEventBeatFactory(ownerBox)); } void EventDispatcher::dispatchEvent( - const RawEvent &rawEvent, + RawEvent const &rawEvent, EventPriority priority) const { getEventQueue(priority).enqueueEvent(std::move(rawEvent)); } diff --git a/ReactCommon/fabric/core/events/EventDispatcher.h b/ReactCommon/fabric/core/events/EventDispatcher.h index 026ed66b68e..b07738cb543 100644 --- a/ReactCommon/fabric/core/events/EventDispatcher.h +++ b/ReactCommon/fabric/core/events/EventDispatcher.h @@ -27,19 +27,20 @@ class StateUpdate; */ class EventDispatcher { public: - using Shared = std::shared_ptr; - using Weak = std::weak_ptr; + using Shared = std::shared_ptr; + using Weak = std::weak_ptr; EventDispatcher( - const EventPipe &eventPipe, - const StatePipe &statePipe, - const EventBeatFactory &synchonousEventBeatFactory, - const EventBeatFactory &asynchonousEventBeatFactory); + EventPipe const &eventPipe, + StatePipe const &statePipe, + EventBeat::Factory const &synchonousEventBeatFactory, + EventBeat::Factory const &asynchonousEventBeatFactory, + EventBeat::SharedOwnerBox const &ownerBox); /* * Dispatches a raw event with given priority using event-delivery pipe. */ - void dispatchEvent(const RawEvent &rawEvent, EventPriority priority) const; + void dispatchEvent(RawEvent const &rawEvent, EventPriority priority) const; /* * Dispatches a state update with given priority. @@ -48,7 +49,7 @@ class EventDispatcher { const; private: - const EventQueue &getEventQueue(EventPriority priority) const; + EventQueue const &getEventQueue(EventPriority priority) const; std::array, 4> eventQueues_; }; diff --git a/ReactCommon/fabric/uimanager/Scheduler.cpp b/ReactCommon/fabric/uimanager/Scheduler.cpp index f52e1e93287..139ec13c147 100644 --- a/ReactCommon/fabric/uimanager/Scheduler.cpp +++ b/ReactCommon/fabric/uimanager/Scheduler.cpp @@ -28,6 +28,7 @@ Scheduler::Scheduler( ->at>("ReactNativeConfig"); auto uiManager = std::make_shared(); + auto eventOwnerBox = std::make_shared(); auto eventPipe = [=](jsi::Runtime &runtime, const EventTarget *eventTarget, @@ -45,19 +46,20 @@ Scheduler::Scheduler( stateTarget.getShadowNode().shared_from_this(), data); }; - auto eventDispatcher = std::make_shared( + eventDispatcher_ = std::make_shared( eventPipe, statePipe, schedulerToolbox.synchronousEventBeatFactory, - schedulerToolbox.asynchronousEventBeatFactory); + schedulerToolbox.asynchronousEventBeatFactory, + eventOwnerBox); - eventDispatcher_ = eventDispatcher; + eventOwnerBox->owner = eventDispatcher_; componentDescriptorRegistry_ = schedulerToolbox.componentRegistryFactory( - eventDispatcher, schedulerToolbox.contextContainer); + eventDispatcher_, schedulerToolbox.contextContainer); rootComponentDescriptor_ = - std::make_unique(eventDispatcher); + std::make_unique(eventDispatcher_); uiManager->setDelegate(this); uiManager->setComponentDescriptorRegistry(componentDescriptorRegistry_); @@ -73,7 +75,6 @@ Scheduler::Scheduler( componentDescriptorRegistry_)); delegate_ = delegate; - eventDispatcher_ = eventDispatcher; uiManager_ = uiManager; } diff --git a/ReactCommon/fabric/uimanager/SchedulerToolbox.h b/ReactCommon/fabric/uimanager/SchedulerToolbox.h index 9505572229d..bb4d868f003 100644 --- a/ReactCommon/fabric/uimanager/SchedulerToolbox.h +++ b/ReactCommon/fabric/uimanager/SchedulerToolbox.h @@ -39,8 +39,8 @@ struct SchedulerToolbox final { * Represent connections with the platform-specific run loops and general * purpose background queue. */ - EventBeatFactory asynchronousEventBeatFactory; - EventBeatFactory synchronousEventBeatFactory; + EventBeat::Factory asynchronousEventBeatFactory; + EventBeat::Factory synchronousEventBeatFactory; }; } // namespace react