Fabric: Removing RuntimeExecutor from EventBeatManager

Summary:
This diff changes the implementation a little bit:
* We don't use RuntimeExecutor there anymore (because it's already been used in EventBeat). The actual purpose of EventBeatManager was distilled to "provide a tick synchronized with the main run loop";
* Now we use dedicated EventBeatManagerObserver interface to decouple this functionality from EventBeat-intrinsic functionality;
* A bunch of small clean-ups.
* Now we ensure threading and capturing ownership in a single function in AsyncEventBeat class. Before this change, the EventBeatManager called `beat` function of the EventBeat directly bypassing AsyncEventBeat class, and effectively bypassing the check that underlying infra still exists (aka EventBeat::OwnerBox feature). That alone might fix some C++ crashes that we see.

Changelog: [Internal] Fabric-specific internal change.

Reviewed By: sammy-SC

Differential Revision: D18307655

fbshipit-source-id: 3b582cb71085ed99ee94f8e6d575196c2082557b

ReactAndroid/src/main/java/com/facebook/react/fabric/jni/AsyncEventBeat.h

@@ -16,34 +16,25 @@
 namespace facebook {
 namespace react {
 
-namespace {
-
-class AsyncEventBeat : public EventBeat {
- private:
-  EventBeatManager* eventBeatManager_;
-  RuntimeExecutor runtimeExecutor_;
-  jni::global_ref<jobject> javaUIManager_;
-
+class AsyncEventBeat final : public EventBeat, public EventBeatManagerObserver {
  public:
-  friend class EventBeatManager;
-
   AsyncEventBeat(
       EventBeat::SharedOwnerBox const &ownerBox,
-      EventBeatManager* eventBeatManager,
+      EventBeatManager *eventBeatManager,
       RuntimeExecutor runtimeExecutor,
-      jni::global_ref<jobject> javaUIManager) :
-      EventBeat(ownerBox),
-      eventBeatManager_(eventBeatManager),
-      runtimeExecutor_(std::move(runtimeExecutor)),
-      javaUIManager_(javaUIManager) {
-    eventBeatManager->registerEventBeat(this);
+      jni::global_ref<jobject> javaUIManager)
+      : EventBeat(ownerBox),
+        eventBeatManager_(eventBeatManager),
+        runtimeExecutor_(runtimeExecutor),
+        javaUIManager_(javaUIManager) {
+    eventBeatManager->addObserver(*this);
   }
 
   ~AsyncEventBeat() {
-    eventBeatManager_->unregisterEventBeat(this);
+    eventBeatManager_->removeObserver(*this);
   }
 
-  void induce() const override {
+  void tick() const override {
     runtimeExecutor_([this, ownerBox = ownerBox_](jsi::Runtime &runtime) {
       auto owner = ownerBox->owner.lock();
       if (!owner) {
@@ -54,6 +45,10 @@ class AsyncEventBeat : public EventBeat {
     });
   }
 
+  void induce() const override {
+    tick();
+  }
+
   void request() const override {
     bool alreadyRequested = isRequested_;
     EventBeat::request();
@@ -65,9 +60,12 @@ class AsyncEventBeat : public EventBeat {
       onRequestEventBeat(javaUIManager_);
     }
   }
-};
 
-} // namespace
+ private:
+  EventBeatManager *eventBeatManager_;
+  RuntimeExecutor runtimeExecutor_;
+  jni::global_ref<jobject> javaUIManager_;
+};
 
 } // namespace react
 } // namespace facebook

ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp

@@ -214,8 +214,6 @@ void Binding::installFabricUIManager(
             });
       };
 
-  eventBeatManager->setRuntimeExecutor(runtimeExecutor);
-
   // TODO: T31905686 Create synchronous Event Beat
   jni::global_ref<jobject> localJavaUIManager = javaUIManager_;
   EventBeat::Factory synchronousBeatFactory =

ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.cpp

@@ -21,29 +21,23 @@ jni::local_ref<EventBeatManager::jhybriddata> EventBeatManager::initHybrid(
   return makeCxxInstance(jhybridobject);
 }
 
-void EventBeatManager::setRuntimeExecutor(RuntimeExecutor runtimeExecutor) {
-  runtimeExecutor_ = runtimeExecutor;
-}
-
-void EventBeatManager::registerEventBeat(EventBeat* eventBeat) const {
+void EventBeatManager::addObserver(
+    EventBeatManagerObserver const &observer) const {
   std::lock_guard<std::mutex> lock(mutex_);
-
-  registeredEventBeats_.insert(eventBeat);
+  observers_.insert(&observer);
 }
 
-void EventBeatManager::unregisterEventBeat(EventBeat* eventBeat) const {
+void EventBeatManager::removeObserver(
+    EventBeatManagerObserver const &observer) const {
   std::lock_guard<std::mutex> lock(mutex_);
-
-  registeredEventBeats_.erase(eventBeat);
+  observers_.erase(&observer);
 }
 
 void EventBeatManager::tick() {
   std::lock_guard<std::mutex> lock(mutex_);
 
-  for (const auto eventBeat : registeredEventBeats_) {
-    runtimeExecutor_([=](jsi::Runtime &runtime) mutable {
-      eventBeat->beat(runtime);
-    });
+  for (auto observer : observers_) {
+    observer->tick();
   }
 }
 

ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventBeatManager.h

@@ -17,33 +17,44 @@
 namespace facebook {
 namespace react {
 
+class EventBeatManagerObserver {
+ public:
+  /*
+   * Called by `EventBeatManager` on the main thread signaling that this is a
+   * good time to flush an event queue.
+   */
+  virtual void tick() const = 0;
+
+  virtual ~EventBeatManagerObserver() noexcept = default;
+};
+
 class EventBeatManager : public jni::HybridClass<EventBeatManager> {
  public:
-  constexpr static const char* const kJavaDescriptor =
+  constexpr static const char *const kJavaDescriptor =
       "Lcom/facebook/react/fabric/events/EventBeatManager;";
 
   static void registerNatives();
 
-  void setRuntimeExecutor(RuntimeExecutor runtimeExecutor);
-
-  void registerEventBeat(EventBeat* eventBeat) const;
-
-  void unregisterEventBeat(EventBeat* eventBeat) const;
-
   EventBeatManager(jni::alias_ref<EventBeatManager::jhybriddata> jhybridobject);
 
+  /*
+   * Adds (or removes) observers.
+   * `EventBeatManager` does not own/retain observers; observers must overlive
+   * the manager or be properly removed before deallocation.
+   */
+  void addObserver(EventBeatManagerObserver const &observer) const;
+  void removeObserver(EventBeatManagerObserver const &observer) const;
+
  private:
   /*
    * Called by Java counterpart at the end of every run loop tick.
    */
   void tick();
 
-  RuntimeExecutor runtimeExecutor_;
-
   jni::alias_ref<EventBeatManager::jhybriddata> jhybridobject_;
 
-  mutable std::unordered_set<const EventBeat*>
-      registeredEventBeats_{}; // Protected by `mutex_`
+  mutable std::unordered_set<EventBeatManagerObserver const *>
+      observers_{}; // Protected by `mutex_`
 
   mutable std::mutex mutex_;