From c25d5948a56d68b7626463b00e86ac780f3038ce Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Thu, 13 Sep 2018 22:56:00 -0700 Subject: [PATCH] Fabric: Exposing EventEmitter's ownership model as a shared_ptr Summary: As we did in the previous diff, here we implemented `EventEmitter`'s ownership model as a `shared_ptr`. This change fixes problem with leaking `WeakObject`s which happens on hot-reload. So, in short: * `EventTargetWrapper` object owns `jsi::WeakObject` that can be converted to actual `jsi::Object` that represent event target in JavaScript realm; * `EventTargetWrapper` and `jsi::WeakObject` objects must be deallocated as soon as native part does not need them anymore; * `EventEmitter` objects retain `EventTarget` objects; * `EventEmitter` can loose event target object in case if assosiated `ShadowNode` got unmounted (not deallocated); in this case `EventEmitter` is loosing possibility to dispatch event even if some mounting-layer code is still retaining it. Reviewed By: mdvacca Differential Revision: D9762755 fbshipit-source-id: 96e989767a32914db9f4627fce51b044c71f257a --- .../componentdescriptor/ComponentDescriptor.h | 2 +- .../ConcreteComponentDescriptor.h | 4 ++-- ReactCommon/fabric/events/EventDispatcher.h | 1 + ReactCommon/fabric/events/EventEmitter.cpp | 13 ++++++++--- ReactCommon/fabric/events/EventEmitter.h | 23 +++++++++++++++---- ReactCommon/fabric/events/EventQueue.cpp | 7 +++--- ReactCommon/fabric/events/EventQueue.h | 2 +- ReactCommon/fabric/events/RawEvent.cpp | 20 ++++++++-------- ReactCommon/fabric/events/RawEvent.h | 14 +++++------ ReactCommon/fabric/events/primitives.h | 21 ++++------------- .../fabric/uimanager/FabricUIManager.cpp | 23 ++++++------------- .../fabric/uimanager/FabricUIManager.h | 9 +++----- ReactCommon/fabric/uimanager/ShadowTree.cpp | 6 ++++- ReactCommon/fabric/uimanager/ShadowTree.h | 2 ++ 14 files changed, 76 insertions(+), 71 deletions(-) diff --git a/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.h b/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.h index 0f4bb7899..bdc2d57fd 100644 --- a/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.h +++ b/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.h @@ -80,7 +80,7 @@ public: * shadow nodes. */ virtual SharedEventEmitter createEventEmitter( - const EventTarget &eventTarget, + SharedEventTarget eventTarget, const Tag &tag ) const = 0; }; diff --git a/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h b/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h index b01398b33..439e2c9f0 100644 --- a/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h +++ b/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h @@ -92,10 +92,10 @@ public: }; virtual SharedEventEmitter createEventEmitter( - const EventTarget &eventTarget, + SharedEventTarget eventTarget, const Tag &tag ) const override { - return std::make_shared(eventTarget, tag, eventDispatcher_); + return std::make_shared(std::move(eventTarget), tag, eventDispatcher_); } protected: diff --git a/ReactCommon/fabric/events/EventDispatcher.h b/ReactCommon/fabric/events/EventDispatcher.h index ee0b61144..9f409f3b6 100644 --- a/ReactCommon/fabric/events/EventDispatcher.h +++ b/ReactCommon/fabric/events/EventDispatcher.h @@ -18,6 +18,7 @@ namespace react { class EventDispatcher; using SharedEventDispatcher = std::shared_ptr; +using WeakEventDispatcher = std::weak_ptr; /* * Represents event-delivery infrastructure. diff --git a/ReactCommon/fabric/events/EventEmitter.cpp b/ReactCommon/fabric/events/EventEmitter.cpp index cc4ad591e..9d8b0ffbd 100644 --- a/ReactCommon/fabric/events/EventEmitter.cpp +++ b/ReactCommon/fabric/events/EventEmitter.cpp @@ -31,10 +31,14 @@ std::recursive_mutex &EventEmitter::DispatchMutex() { return mutex; } -EventEmitter::EventEmitter(const EventTarget &eventTarget, const Tag &tag, const std::shared_ptr &eventDispatcher): - eventTarget_(eventTarget), +EventEmitter::EventEmitter( + SharedEventTarget eventTarget, + Tag tag, + WeakEventDispatcher eventDispatcher +): + eventTarget_(std::move(eventTarget)), tag_(tag), - eventDispatcher_(eventDispatcher) {} + eventDispatcher_(std::move(eventDispatcher)) {} void EventEmitter::dispatchEvent( const std::string &type, @@ -73,6 +77,9 @@ void EventEmitter::dispatchEvent( void EventEmitter::setEnabled(bool enabled) const { enabled_ = enabled; + if (!enabled) { + eventTarget_ = nullptr; + } } bool EventEmitter::getEnabled() const { diff --git a/ReactCommon/fabric/events/EventEmitter.h b/ReactCommon/fabric/events/EventEmitter.h index f2e56da42..788f4c2cf 100644 --- a/ReactCommon/fabric/events/EventEmitter.h +++ b/ReactCommon/fabric/events/EventEmitter.h @@ -22,8 +22,16 @@ using SharedEventEmitter = std::shared_ptr; /* * Base class for all particular typed event handlers. - * Stores `InstanceHandle` identifying a particular component and the pointer - * to `EventDispatcher` which is responsible for delivering the event. + * Stores a pointer to `EventTarget` identifying a particular component and + * a weak pointer to `EventDispatcher` which is responsible for delivering the event. + * + * Note: Retaining an `EventTarget` does *not* guarantee that actual event target + * exists and/or valid in JavaScript realm. The `EventTarget` retains an `EventTargetWrapper` + * which wraps JavaScript object in `unsafe-unretained` manner. Retaining + * the `EventTarget` *does* indicate that we can use that to get an actual + * JavaScript object from that in the future *ensuring safety beforehand somehow*; + * JSI maintains `WeakObject` object as long as we retain the `EventTarget`. + * All `EventTarget` instances must be deallocated before stopping JavaScript machine. */ class EventEmitter: public std::enable_shared_from_this { @@ -37,7 +45,12 @@ class EventEmitter: public: static std::recursive_mutex &DispatchMutex(); - EventEmitter(const EventTarget &eventTarget, const Tag &tag, const std::shared_ptr &eventDispatcher); + EventEmitter( + SharedEventTarget eventTarget, + Tag tag, + WeakEventDispatcher eventDispatcher + ); + virtual ~EventEmitter() = default; /* @@ -59,9 +72,9 @@ protected: ) const; private: - EventTarget eventTarget_; + mutable SharedEventTarget eventTarget_; Tag tag_; - std::weak_ptr eventDispatcher_; + WeakEventDispatcher eventDispatcher_; mutable bool enabled_; // Protected by `DispatchMutex`. }; diff --git a/ReactCommon/fabric/events/EventQueue.cpp b/ReactCommon/fabric/events/EventQueue.cpp index 39ed94a15..7cd04c948 100644 --- a/ReactCommon/fabric/events/EventQueue.cpp +++ b/ReactCommon/fabric/events/EventQueue.cpp @@ -12,8 +12,8 @@ namespace facebook { namespace react { -EventQueue::EventQueue(const EventPipe &eventPipe, std::unique_ptr eventBeat): - eventPipe_(eventPipe), +EventQueue::EventQueue(EventPipe eventPipe, std::unique_ptr eventBeat): + eventPipe_(std::move(eventPipe)), eventBeat_(std::move(eventBeat)) { eventBeat_->setBeatCallback(std::bind(&EventQueue::onBeat, this)); } @@ -40,8 +40,9 @@ void EventQueue::onBeat() const { { std::lock_guard lock(EventEmitter::DispatchMutex()); for (const auto &event : queue) { + auto eventTarget = event.eventTarget.lock(); eventPipe_( - event.isDispachable() ? event.eventTarget : EmptyEventTarget, + eventTarget && event.isDispatchable() ? eventTarget.get() : nullptr, event.type, event.payload ); diff --git a/ReactCommon/fabric/events/EventQueue.h b/ReactCommon/fabric/events/EventQueue.h index 79cbc8d0e..c10a989d7 100644 --- a/ReactCommon/fabric/events/EventQueue.h +++ b/ReactCommon/fabric/events/EventQueue.h @@ -25,7 +25,7 @@ namespace react { class EventQueue { public: - EventQueue(const EventPipe &eventPipe, std::unique_ptr eventBeat); + EventQueue(EventPipe eventPipe, std::unique_ptr eventBeat); virtual ~EventQueue() = default; /* diff --git a/ReactCommon/fabric/events/RawEvent.cpp b/ReactCommon/fabric/events/RawEvent.cpp index 9ca30776c..876cdc762 100644 --- a/ReactCommon/fabric/events/RawEvent.cpp +++ b/ReactCommon/fabric/events/RawEvent.cpp @@ -11,18 +11,18 @@ namespace facebook { namespace react { RawEvent::RawEvent( - const std::string &type, - const folly::dynamic &payload, - const EventTarget &eventTarget, - const std::function &isDispatchable + std::string type, + folly::dynamic payload, + WeakEventTarget eventTarget, + RawEventDispatchable isDispatchable ): - type(type), - payload(payload), - eventTarget(eventTarget), - isDispachable_(isDispatchable) {} + type(std::move(type)), + payload(std::move(payload)), + eventTarget(std::move(eventTarget)), + isDispatchable_(std::move(isDispatchable)) {} -bool RawEvent::isDispachable() const { - return isDispachable_(); +bool RawEvent::isDispatchable() const { + return isDispatchable_(); } } // namespace react diff --git a/ReactCommon/fabric/events/RawEvent.h b/ReactCommon/fabric/events/RawEvent.h index 631748899..41a72ceda 100644 --- a/ReactCommon/fabric/events/RawEvent.h +++ b/ReactCommon/fabric/events/RawEvent.h @@ -23,25 +23,25 @@ public: using RawEventDispatchable = std::function; RawEvent( - const std::string &type, - const folly::dynamic &payload, - const EventTarget &eventTarget, - const RawEventDispatchable &isDispachable + std::string type, + folly::dynamic payload, + WeakEventTarget eventTarget, + RawEventDispatchable isDispatchable ); const std::string type; const folly::dynamic payload; - const EventTarget eventTarget; + const WeakEventTarget eventTarget; /* * Returns `true` if event can be dispatched to `eventTarget`. * Events that associated with unmounted or deallocated `ShadowNode`s * must not be dispatched. */ - bool isDispachable() const; + bool isDispatchable() const; private: - const RawEventDispatchable isDispachable_; + const RawEventDispatchable isDispatchable_; }; } // namespace react diff --git a/ReactCommon/fabric/events/primitives.h b/ReactCommon/fabric/events/primitives.h index 9ee1fac11..0d1ab5a8c 100644 --- a/ReactCommon/fabric/events/primitives.h +++ b/ReactCommon/fabric/events/primitives.h @@ -24,28 +24,17 @@ enum class EventPriority: int { Deferred = AsynchronousBatched }; -/* `InstanceHandler`, `EventTarget`, and `EventHandler` are all opaque - * raw pointers. We use `struct {} *` trick to differentiate them in compiler's - * eyes to ensure type safety. - * These structs must have names (and the names must be exported) - * to allow consistent template (e.g. `std::function`) instantiating - * across different modules. - */ -using EventTarget = struct EventTargetDummyStruct {} *; - /* * We need this types only to ensure type-safety when we deal with them. Conceptually, * they are opaque pointers to some types that derived from those classes. */ class EventHandler {}; +class EventTarget {}; +using SharedEventHandler = std::shared_ptr; +using SharedEventTarget = std::shared_ptr; +using WeakEventTarget = std::weak_ptr; -/* - * EmptyEventTarget is used when some event cannot be dispatched to an original - * event target but still has to be dispatched to preserve consistency of event flow. - */ -static const EventTarget EmptyEventTarget = nullptr; - -using EventPipe = std::function; +using EventPipe = std::function; } // namespace react } // namespace facebook diff --git a/ReactCommon/fabric/uimanager/FabricUIManager.cpp b/ReactCommon/fabric/uimanager/FabricUIManager.cpp index 69f703df9..a8ea221ba 100644 --- a/ReactCommon/fabric/uimanager/FabricUIManager.cpp +++ b/ReactCommon/fabric/uimanager/FabricUIManager.cpp @@ -105,20 +105,15 @@ void FabricUIManager::setDispatchEventToTargetFunction(std::function releaseEventTargetFunction) { - releaseEventTargetFunction_ = releaseEventTargetFunction; -} - -void FabricUIManager::dispatchEventToTarget(const EventTarget &eventTarget, const std::string &type, const folly::dynamic &payload) const { - if (eventTarget != EmptyEventTarget) { +void FabricUIManager::dispatchEventToTarget(const EventTarget *eventTarget, const std::string &type, const folly::dynamic &payload) const { + if (eventTarget) { dispatchEventToTargetFunction_( *eventHandler_, - eventTarget, + *eventTarget, const_cast(type), const_cast(payload) ); - } - else { + } else { dispatchEventToEmptyTargetFunction_( *eventHandler_, const_cast(type), @@ -127,11 +122,7 @@ void FabricUIManager::dispatchEventToTarget(const EventTarget &eventTarget, cons } } -void FabricUIManager::releaseEventTarget(const EventTarget &eventTarget) const { - releaseEventTargetFunction_(eventTarget); -} - -SharedShadowNode FabricUIManager::createNode(int tag, std::string viewName, int rootTag, folly::dynamic props, EventTarget eventTarget) { +SharedShadowNode FabricUIManager::createNode(int tag, std::string viewName, int rootTag, folly::dynamic props, SharedEventTarget eventTarget) { isLoggingEnabled && LOG(INFO) << "FabricUIManager::createNode(tag: " << tag << ", name: " << viewName << ", rootTag: " << rootTag << ", props: " << props << ")"; ComponentName componentName = componentNameByReactViewName(viewName); @@ -142,7 +133,7 @@ SharedShadowNode FabricUIManager::createNode(int tag, std::string viewName, int componentDescriptor->createShadowNode({ .tag = tag, .rootTag = rootTag, - .eventEmitter = componentDescriptor->createEventEmitter(eventTarget, tag), + .eventEmitter = componentDescriptor->createEventEmitter(std::move(eventTarget), tag), .props = componentDescriptor->cloneProps(nullptr, rawProps) }); @@ -246,7 +237,7 @@ void FabricUIManager::completeRoot(int rootTag, const SharedShadowNodeUnsharedLi void FabricUIManager::registerEventHandler(std::shared_ptr eventHandler) { isLoggingEnabled && LOG(INFO) << "FabricUIManager::registerEventHandler(eventHandler: " << eventHandler.get() << ")"; - eventHandler_ = eventHandler; + eventHandler_ = std::move(eventHandler); } } // namespace react diff --git a/ReactCommon/fabric/uimanager/FabricUIManager.h b/ReactCommon/fabric/uimanager/FabricUIManager.h index febab0de0..f98dd9430 100644 --- a/ReactCommon/fabric/uimanager/FabricUIManager.h +++ b/ReactCommon/fabric/uimanager/FabricUIManager.h @@ -19,7 +19,7 @@ namespace facebook { namespace react { using DispatchEventToEmptyTargetFunction = void (const EventHandler &eventHandler, std::string type, folly::dynamic payload); -using DispatchEventToTargetFunction = void (const EventHandler &eventHandler, EventTarget eventTarget, std::string type, folly::dynamic payload); +using DispatchEventToTargetFunction = void (const EventHandler &eventHandler, const EventTarget &eventTarget, std::string type, folly::dynamic payload); using ReleaseEventTargetFunction = void (EventTarget eventTarget); class FabricUIManager { @@ -44,16 +44,14 @@ public: */ void setDispatchEventToEmptyTargetFunction(std::function dispatchEventFunction); void setDispatchEventToTargetFunction(std::function dispatchEventFunction); - void setReleaseEventTargetFunction(std::function releaseEventTargetFunction); #pragma mark - Native-facing Interface - void dispatchEventToTarget(const EventTarget &eventTarget, const std::string &type, const folly::dynamic &payload) const; - void releaseEventTarget(const EventTarget &eventTarget) const; + void dispatchEventToTarget(const EventTarget *eventTarget, const std::string &type, const folly::dynamic &payload) const; #pragma mark - JavaScript/React-facing Interface - SharedShadowNode createNode(Tag reactTag, std::string viewName, Tag rootTag, folly::dynamic props, EventTarget eventTarget); + SharedShadowNode createNode(Tag reactTag, std::string viewName, Tag rootTag, folly::dynamic props, SharedEventTarget eventTarget); SharedShadowNode cloneNode(const SharedShadowNode &node); SharedShadowNode cloneNodeWithNewChildren(const SharedShadowNode &node); SharedShadowNode cloneNodeWithNewProps(const SharedShadowNode &node, folly::dynamic props); @@ -71,7 +69,6 @@ private: std::shared_ptr eventHandler_; std::function dispatchEventToEmptyTargetFunction_; std::function dispatchEventToTargetFunction_; - std::function releaseEventTargetFunction_; }; } // namespace react diff --git a/ReactCommon/fabric/uimanager/ShadowTree.cpp b/ReactCommon/fabric/uimanager/ShadowTree.cpp index c12341cd6..4693e70f1 100644 --- a/ReactCommon/fabric/uimanager/ShadowTree.cpp +++ b/ReactCommon/fabric/uimanager/ShadowTree.cpp @@ -18,7 +18,7 @@ namespace react { ShadowTree::ShadowTree(Tag rootTag): rootTag_(rootTag) { - const auto noopEventEmitter = std::make_shared(nullptr, rootTag, nullptr); + const auto noopEventEmitter = std::make_shared(nullptr, rootTag, std::shared_ptr()); rootShadowNode_ = std::make_shared( ShadowNodeFragment { .tag = rootTag, @@ -30,6 +30,10 @@ ShadowTree::ShadowTree(Tag rootTag): ); } +ShadowTree::~ShadowTree() { + complete(std::make_shared(SharedShadowNodeList {})); +} + Tag ShadowTree::getRootTag() const { return rootTag_; } diff --git a/ReactCommon/fabric/uimanager/ShadowTree.h b/ReactCommon/fabric/uimanager/ShadowTree.h index 22c134e56..2f7e94049 100644 --- a/ReactCommon/fabric/uimanager/ShadowTree.h +++ b/ReactCommon/fabric/uimanager/ShadowTree.h @@ -30,6 +30,8 @@ public: */ ShadowTree(Tag rootTag); + ~ShadowTree(); + /* * Returns the rootTag associated with the shadow tree (the tag of the * root shadow node).