Add option to use raw pointer instead of shared_ptr in EventEmitterWr…

…apper Summary: changelog: [internal] Retaining `EventEmitter` beyond runtime triggers a crash. Let's try to use raw pointer in `EventEmitterWrapper` to see if it fixes some crashes. Reviewed By: philIip Differential Revision: D31307332 fbshipit-source-id: cd059b6c56f8dffe985b3ecb62cdafe823ba1462
facebook · Oct 2, 2021 · 36f3bf2 · 36f3bf2
1 parent 2372e7a
commit 36f3bf2
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 6 deletions.
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
@@ -243,8 +243,9 @@ Binding::getInspectorDataForInstance(
   }
 
   EventEmitterWrapper *cEventEmitter = cthis(eventEmitterWrapper);
-  InspectorData data =
-      scheduler->getInspectorDataForInstance(*cEventEmitter->eventEmitter);
+  InspectorData data = scheduler->getInspectorDataForInstance(
+      enableEventEmitterRawPointer_ ? *cEventEmitter->eventEmitterPointer
+                                    : *cEventEmitter->eventEmitter);
 
   folly::dynamic result = folly::dynamic::object;
   result["fileName"] = data.fileName;
@@ -511,6 +512,9 @@ void Binding::installFabricUIManager(
   disableRevisionCheckForPreallocation_ =
       config->getBool("react_fabric:disable_revision_check_for_preallocation");
 
+  enableEventEmitterRawPointer_ =
+      config->getBool("react_fabric:enable_event_emitter_wrapper_raw_pointer");
+
   if (enableFabricLogs_) {
     LOG(WARNING) << "Binding::installFabricUIManager() was called (address: "
                  << this << ").";
@@ -920,8 +924,11 @@ void Binding::schedulerDidFinishTransaction(
           mountItem.newChildShadowView.eventEmitter;
       auto javaEventEmitter = EventEmitterWrapper::newObjectJavaArgs();
       EventEmitterWrapper *cEventEmitter = cthis(javaEventEmitter);
-      cEventEmitter->eventEmitter = eventEmitter;
-
+      if (enableEventEmitterRawPointer_) {
+        cEventEmitter->eventEmitterPointer = eventEmitter.get();
+      } else {
+        cEventEmitter->eventEmitter = eventEmitter;
+      }
       temp[0] = mountItem.newChildShadowView.tag;
       temp[1] = isLayoutable;
       env->SetIntArrayRegion(intBufferArray, intBufferPosition, 2, temp);
@@ -1071,7 +1078,11 @@ void Binding::schedulerDidFinishTransaction(
       // Do not hold a reference to javaEventEmitter from the C++ side.
       auto javaEventEmitter = EventEmitterWrapper::newObjectJavaArgs();
       EventEmitterWrapper *cEventEmitter = cthis(javaEventEmitter);
-      cEventEmitter->eventEmitter = eventEmitter;
+      if (enableEventEmitterRawPointer_) {
+        cEventEmitter->eventEmitterPointer = eventEmitter.get();
+      } else {
+        cEventEmitter->eventEmitter = eventEmitter;
+      }
 
       (*objBufferArray)[objBufferPosition++] = javaEventEmitter.get();
     }
@@ -1198,7 +1209,11 @@ void Binding::preallocateShadowView(
     if (eventEmitter != nullptr) {
       javaEventEmitter = EventEmitterWrapper::newObjectJavaArgs();
       EventEmitterWrapper *cEventEmitter = cthis(javaEventEmitter);
-      cEventEmitter->eventEmitter = eventEmitter;
+      if (enableEventEmitterRawPointer_) {
+        cEventEmitter->eventEmitterPointer = eventEmitter.get();
+      } else {
+        cEventEmitter->eventEmitter = eventEmitter;
+      }
     }
   }
 

diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h
@@ -202,6 +202,7 @@ class Binding : public jni::HybridClass<Binding>,
   bool enableFabricLogs_{false};
   bool enableEarlyEventEmitterUpdate_{false};
   bool disableRevisionCheckForPreallocation_{false};
+  bool enableEventEmitterRawPointer_{false};
 };
 
 } // namespace react

diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.cpp
@@ -22,6 +22,15 @@ void EventEmitterWrapper::invokeEvent(
     std::string eventName,
     NativeMap *payload,
     int category) {
+  if (eventEmitterPointer) {
+    eventEmitterPointer->dispatchEvent(
+        std::move(eventName),
+        payload->consume(),
+        EventPriority::AsynchronousBatched,
+        static_cast<RawEvent::Category>(category));
+    return;
+  }
+
   // It is marginal, but possible for this to be constructed without a valid
   // EventEmitter. In those cases, make sure we noop/blackhole events instead of
   // crashing.
@@ -38,6 +47,11 @@ void EventEmitterWrapper::invokeUniqueEvent(
     std::string eventName,
     NativeMap *payload,
     int customCoalesceKey) {
+  if (eventEmitterPointer) {
+    eventEmitterPointer->dispatchUniqueEvent(
+        std::move(eventName), payload->consume());
+    return;
+  }
   // TODO: customCoalesceKey currently unused
   // It is marginal, but possible for this to be constructed without a valid
   // EventEmitter. In those cases, make sure we noop/blackhole events instead of

diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.h
@@ -24,6 +24,7 @@ class EventEmitterWrapper : public jni::HybridClass<EventEmitterWrapper> {
   static void registerNatives();
 
   SharedEventEmitter eventEmitter;
+  EventEmitter const *eventEmitterPointer;
 
   void invokeEvent(std::string eventName, NativeMap *params, int category);
   void invokeUniqueEvent(