Guard against unsafe EventEmitter setup and teardown

Summary:
Because of T92179998, T93607943, and T93394807, we are still seeking resolution to tricky crashes wrt the use of EventEmitters.

I believe the recent spike is because of two recent changes: we pass in EventEmitters earlier, during PreAllocation; and we clean them up earlier, during stopSurface, to avoid jsi::~Pointer crashes.

Additionally, the gating previously added around the PreAllocation path was incorrect and led to more nullptrs being passed around as EventEmitters.

To mitigate these issues:

1) I am adding/fixing gating to preallocation and early cleanup paths
2) I am making EventEmitterWrapper more resilient by ensuring EventEmitter is non-null before invoking it.
3) I am making sure that in more cases, we pass a non-null EventEmitter pointer to Java.
4) I am backing out the synchronization in EventEmitterWrapper (java side) as that did not resolve the issue and is a pessimisation

There are older, unchanged paths that could still be passing in nullptr as the EventEmitter (Update and Create). As those have not changed recently, I'm not going to fix those cases and instead, we can now rely on the caller to ensure that the EventEmitter is non-null before calling.

Changelog: [internal]

Differential Revision: D29252806

fbshipit-source-id: 5c68d95fa2465afe45e0083a0685c8c1abf31619

Author: JoshuaGross

ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java

@@ -66,6 +66,8 @@ public static boolean isMapBufferSerializationEnabled() {
 
   public static boolean enableLockFreeEventDispatcher = false;
 
+  public static boolean enableAggressiveEventEmitterCleanup = false;
+
   //
   // ScrollView C++ UpdateState vs onScroll race fixes
   //

ReactAndroid/src/main/java/com/facebook/react/fabric/events/EventEmitterWrapper.java

@@ -48,13 +48,11 @@ private native void invokeUniqueEvent(
    * @param params {@link WritableMap} payload of the event
    */
   public void invoke(@NonNull String eventName, @Nullable WritableMap params) {
-    synchronized (mHybridData) {
-      if (!isValid()) {
-        return;
-      }
-      NativeMap payload = params == null ? new WritableNativeMap() : (NativeMap) params;
-      invokeEvent(eventName, payload);
+    if (!isValid()) {
+      return;
     }
+    NativeMap payload = params == null ? new WritableNativeMap() : (NativeMap) params;
+    invokeEvent(eventName, payload);
   }
 
   /**
@@ -66,20 +64,16 @@ public void invoke(@NonNull String eventName, @Nullable WritableMap params) {
    */
   public void invokeUnique(
       @NonNull String eventName, @Nullable WritableMap params, int customCoalesceKey) {
-    synchronized (mHybridData) {
-      if (!isValid()) {
-        return;
-      }
-      NativeMap payload = params == null ? new WritableNativeMap() : (NativeMap) params;
-      invokeUniqueEvent(eventName, payload, customCoalesceKey);
+    if (!isValid()) {
+      return;
     }
+    NativeMap payload = params == null ? new WritableNativeMap() : (NativeMap) params;
+    invokeUniqueEvent(eventName, payload, customCoalesceKey);
   }
 
   public void destroy() {
-    synchronized (mHybridData) {
-      if (mHybridData != null) {
-        mHybridData.resetNative();
-      }
+    if (mHybridData != null) {
+      mHybridData.resetNative();
     }
   }
 

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

@@ -1151,11 +1151,14 @@ void Binding::preallocateShadowView(
   }
 
   // Do not hold a reference to javaEventEmitter from the C++ side.
-  auto javaEventEmitter = EventEmitterWrapper::newObjectJavaArgs();
+  local_ref<EventEmitterWrapper::JavaPart> javaEventEmitter = nullptr;
   if (enableEarlyEventEmitterUpdate_) {
     SharedEventEmitter eventEmitter = shadowView.eventEmitter;
-    EventEmitterWrapper *cEventEmitter = cthis(javaEventEmitter);
-    cEventEmitter->eventEmitter = eventEmitter;
+    if (eventEmitter != nullptr) {
+      javaEventEmitter = EventEmitterWrapper::newObjectJavaArgs();
+      EventEmitterWrapper *cEventEmitter = cthis(javaEventEmitter);
+      cEventEmitter->eventEmitter = eventEmitter;
+    }
   }
 
   local_ref<ReadableMap::javaobject> props = castReadableMap(
@@ -1169,7 +1172,7 @@ void Binding::preallocateShadowView(
       component.get(),
       props.get(),
       (javaStateWrapper != nullptr ? javaStateWrapper.get() : nullptr),
-      javaEventEmitter.get(),
+      (javaEventEmitter != nullptr ? javaEventEmitter.get() : nullptr),
       isLayoutableShadowNode);
 }
 

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

@@ -21,16 +21,26 @@ EventEmitterWrapper::initHybrid(jni::alias_ref<jclass>) {
 void EventEmitterWrapper::invokeEvent(
     std::string eventName,
     NativeMap *payload) {
-  eventEmitter->dispatchEvent(
-      eventName, payload->consume(), EventPriority::AsynchronousBatched);
+  // 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.
+  if (eventEmitter != nullptr) {
+    eventEmitter->dispatchEvent(
+        eventName, payload->consume(), EventPriority::AsynchronousBatched);
+  }
 }
 
 void EventEmitterWrapper::invokeUniqueEvent(
     std::string eventName,
     NativeMap *payload,
     int customCoalesceKey) {
   // TODO: customCoalesceKey currently unused
-  eventEmitter->dispatchUniqueEvent(eventName, payload->consume());
+  // 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.
+  if (eventEmitter != nullptr) {
+    eventEmitter->dispatchUniqueEvent(eventName, payload->consume());
+  }
 }
 
 void EventEmitterWrapper::registerNatives() {

ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java

@@ -27,6 +27,7 @@
 import com.facebook.react.bridge.SoftAssertions;
 import com.facebook.react.bridge.UiThreadUtil;
 import com.facebook.react.common.build.ReactBuildConfig;
+import com.facebook.react.config.ReactFeatureFlags;
 import com.facebook.react.fabric.events.EventEmitterWrapper;
 import com.facebook.react.fabric.mounting.MountingManager.MountItemExecutor;
 import com.facebook.react.fabric.mounting.mountitems.MountItem;
@@ -250,9 +251,11 @@ public void stopSurface() {
         viewState.mStateWrapper.destroyState();
         viewState.mStateWrapper = null;
       }
-      if (viewState.mEventEmitter != null) {
-        viewState.mEventEmitter.destroy();
-        viewState.mEventEmitter = null;
+      if (ReactFeatureFlags.enableAggressiveEventEmitterCleanup) {
+        if (viewState.mEventEmitter != null) {
+          viewState.mEventEmitter.destroy();
+          viewState.mEventEmitter = null;
+        }
       }
     }