From e05acf19303a801b32ea27ba7e9489409e4f9247 Mon Sep 17 00:00:00 2001
From: Valentin Shergin <shergin@fb.com>
Date: Thu, 13 Sep 2018 22:56:09 -0700
Subject: [PATCH] Fabric: Managing an EventHandler as a unique_pointer

Summary:
I realized that instead of using shared_ptr's type-erasure feature, we can make the EventHandler's destructor virtual and this itself will allow safe deallocation by a pointer to a base class.
We cannot use the same technic for EventTarget thought because having a weak_ptr to this is another feature of shared_ptr that we need.

Reviewed By: mdvacca

Differential Revision: D9775742

fbshipit-source-id: 3c23a163827e8aa9ec731c89ce87051a93afe4ca
---
 ReactCommon/fabric/events/primitives.h           | 14 +++++++++++---
 ReactCommon/fabric/uimanager/FabricUIManager.cpp |  2 +-
 ReactCommon/fabric/uimanager/FabricUIManager.h   |  4 ++--
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/ReactCommon/fabric/events/primitives.h b/ReactCommon/fabric/events/primitives.h
index 0d1ab5a8c..c13a6a8fe 100644
--- a/ReactCommon/fabric/events/primitives.h
+++ b/ReactCommon/fabric/events/primitives.h
@@ -27,10 +27,18 @@ enum class EventPriority: int {
 /*
  * 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.
+ *
+ * `EventHandler` is managed as a `unique_ptr`, so it must have a *virtual*
+ * destructor to allow proper deallocation having only a pointer
+ * to the base (`EventHandler`) class.
+ *
+ * `EventTarget` is managed as a `shared_ptr`, so it does not need to have a virtual
+ * destructor because `shared_ptr` stores a pointer to destructor inside.
  */
-class EventHandler {};
-class EventTarget {};
-using SharedEventHandler = std::shared_ptr<const EventHandler>;
+struct EventHandler { virtual ~EventHandler() = default; };
+using UniqueEventHandler = std::unique_ptr<const EventHandler>;
+
+struct EventTarget {};
 using SharedEventTarget = std::shared_ptr<const EventTarget>;
 using WeakEventTarget = std::weak_ptr<const EventTarget>;
 
diff --git a/ReactCommon/fabric/uimanager/FabricUIManager.cpp b/ReactCommon/fabric/uimanager/FabricUIManager.cpp
index a8ea221ba..ff8456a09 100644
--- a/ReactCommon/fabric/uimanager/FabricUIManager.cpp
+++ b/ReactCommon/fabric/uimanager/FabricUIManager.cpp
@@ -235,7 +235,7 @@ void FabricUIManager::completeRoot(int rootTag, const SharedShadowNodeUnsharedLi
   }
 }
 
-void FabricUIManager::registerEventHandler(std::shared_ptr<EventHandler> eventHandler) {
+void FabricUIManager::registerEventHandler(UniqueEventHandler eventHandler) {
   isLoggingEnabled && LOG(INFO) << "FabricUIManager::registerEventHandler(eventHandler: " << eventHandler.get() << ")";
   eventHandler_ = std::move(eventHandler);
 }
diff --git a/ReactCommon/fabric/uimanager/FabricUIManager.h b/ReactCommon/fabric/uimanager/FabricUIManager.h
index f98dd9430..61ddbbdfe 100644
--- a/ReactCommon/fabric/uimanager/FabricUIManager.h
+++ b/ReactCommon/fabric/uimanager/FabricUIManager.h
@@ -60,13 +60,13 @@ public:
   SharedShadowNodeUnsharedList createChildSet(Tag rootTag);
   void appendChildToSet(const SharedShadowNodeUnsharedList &childSet, const SharedShadowNode &childNode);
   void completeRoot(Tag rootTag, const SharedShadowNodeUnsharedList &childSet);
-  void registerEventHandler(std::shared_ptr<EventHandler> eventHandler);
+  void registerEventHandler(UniqueEventHandler eventHandler);
 
 private:
 
   SharedComponentDescriptorRegistry componentDescriptorRegistry_;
   UIManagerDelegate *delegate_;
-  std::shared_ptr<EventHandler> eventHandler_;
+  UniqueEventHandler eventHandler_;
   std::function<DispatchEventToEmptyTargetFunction> dispatchEventToEmptyTargetFunction_;
   std::function<DispatchEventToTargetFunction> dispatchEventToTargetFunction_;
 };