From 8d83d5f8ebb9960e75907cdfb5f749f61005e624 Mon Sep 17 00:00:00 2001
From: Valentin Shergin <shergin@fb.com>
Date: Wed, 16 Jan 2019 20:17:01 -0800
Subject: [PATCH] Fabric: Unifying retain/release logic in EventTarget

Summary:
I read my code and one thing stroke me: What if the same event emitter dispatches several events during the same event beat? In the previous implementation, only the first one will be delivered because the first `release` call consumes stored strong reference.
So, I changed the API a bit to make this more explicit: we have `retain` & `release` methods and we have a getter that works (multiple times) only if the object was successfully retained.

Reviewed By: sahrens

Differential Revision: D13668147

fbshipit-source-id: c00af5f15bc7e30aa704d46bd23584b918b6f75a
---
 ReactCommon/fabric/events/EventQueue.cpp          |  7 +++++++
 ReactCommon/fabric/events/EventTarget.cpp         | 13 +++++++++++--
 ReactCommon/fabric/events/EventTarget.h           | 13 +++++++++----
 ReactCommon/fabric/uimanager/UIManagerBinding.cpp |  2 +-
 4 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/ReactCommon/fabric/events/EventQueue.cpp b/ReactCommon/fabric/events/EventQueue.cpp
index f9f970b0c7a..01a9ef688e1 100644
--- a/ReactCommon/fabric/events/EventQueue.cpp
+++ b/ReactCommon/fabric/events/EventQueue.cpp
@@ -53,6 +53,13 @@ void EventQueue::onBeat(jsi::Runtime &runtime) const {
     eventPipe_(
         runtime, event.eventTarget.get(), event.type, event.payloadFactory);
   }
+
+  // No need to lock `EventEmitter::DispatchMutex()` here.
+  for (const auto &event : queue) {
+    if (event.eventTarget) {
+      event.eventTarget->release(runtime);
+    }
+  }
 }
 
 } // namespace react
diff --git a/ReactCommon/fabric/events/EventTarget.cpp b/ReactCommon/fabric/events/EventTarget.cpp
index 64dc2549056..112ca5688a8 100644
--- a/ReactCommon/fabric/events/EventTarget.cpp
+++ b/ReactCommon/fabric/events/EventTarget.cpp
@@ -42,11 +42,20 @@ void EventTarget::retain(jsi::Runtime &runtime) const {
   assert(!strongInstanceHandle_.isUndefined());
 }
 
-jsi::Value EventTarget::release(jsi::Runtime &runtime) const {
+void EventTarget::release(jsi::Runtime &runtime) const {
   // The method does not use `jsi::Runtime` reference.
   // It takes it only to ensure thread-safety (if the caller has the reference,
   // we are on a proper thread).
-  return std::move(strongInstanceHandle_);
+  strongInstanceHandle_ = jsi::Value::null();
+}
+
+jsi::Value EventTarget::getInstanceHandle(jsi::Runtime &runtime) const {
+  if (strongInstanceHandle_.isNull()) {
+    // The `instanceHandle` is not retained.
+    return jsi::Value::null();
+  }
+
+  return jsi::Value(runtime, strongInstanceHandle_);
 }
 
 Tag EventTarget::getTag() const {
diff --git a/ReactCommon/fabric/events/EventTarget.h b/ReactCommon/fabric/events/EventTarget.h
index 5a4cf0ae4cd..7567d994e9e 100644
--- a/ReactCommon/fabric/events/EventTarget.h
+++ b/ReactCommon/fabric/events/EventTarget.h
@@ -43,16 +43,21 @@ class EventTarget {
   void setEnabled(bool enabled) const;
 
   /*
-   * Creates a strong instance handle from a weak one and stores it inside the
-   * object.
+   * Retains an instance handler by creating a strong reference to it.
    * If the EventTarget is disabled, does nothing.
    */
   void retain(jsi::Runtime &runtime) const;
 
   /*
-   * Extract the stored strong instance handle from the object and returns it.
+   * Releases the instance handler by nulling a strong reference to it.
    */
-  jsi::Value release(jsi::Runtime &runtime) const;
+  void release(jsi::Runtime &runtime) const;
+
+  /*
+   * Creates and returns the `instanceHandle`.
+   * Returns `null` if the `instanceHandle` is not retained at this moment.
+   */
+  jsi::Value getInstanceHandle(jsi::Runtime &runtime) const;
 
   /*
    * Deprecated. Do not use.
diff --git a/ReactCommon/fabric/uimanager/UIManagerBinding.cpp b/ReactCommon/fabric/uimanager/UIManagerBinding.cpp
index cc3fe326bd4..0b787b74ddf 100644
--- a/ReactCommon/fabric/uimanager/UIManagerBinding.cpp
+++ b/ReactCommon/fabric/uimanager/UIManagerBinding.cpp
@@ -74,7 +74,7 @@ void UIManagerBinding::dispatchEvent(
 
   auto instanceHandle = eventTarget
     ? [&]() {
-      auto instanceHandle = eventTarget->release(runtime);
+      auto instanceHandle = eventTarget->getInstanceHandle(runtime);
       if (instanceHandle.isUndefined()) {
         return jsi::Value::null();
       }