Fix thread-safety of enableFabricPendingEventQueue experiment (#39424)

Summary: Pull Request resolved: #39424 Make this struct-like class private, and fix incorrect threading access to `mPendingEventQueue`. Changelog: [Android][Fixed] Improve threading in enableFabricPendingEventQueue experiment Reviewed By: rshest Differential Revision: D44667266 fbshipit-source-id: abf6b9b4ce01f23ac406cd29a2e40544d9b6464c
facebook · Sep 14, 2023 · 318b4c0 · 318b4c0
1 parent 9d52767
commit 318b4c0
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 58 deletions.
diff --git a/...es/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/...es/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java
@@ -61,7 +61,6 @@
 import com.facebook.react.fabric.mounting.MountItemDispatcher;
 import com.facebook.react.fabric.mounting.MountingManager;
 import com.facebook.react.fabric.mounting.SurfaceMountingManager;
-import com.facebook.react.fabric.mounting.SurfaceMountingManager.ViewEvent;
 import com.facebook.react.fabric.mounting.mountitems.BatchMountItem;
 import com.facebook.react.fabric.mounting.mountitems.DispatchCommandMountItem;
 import com.facebook.react.fabric.mounting.mountitems.MountItem;
@@ -964,7 +963,7 @@ public void receiveEvent(
         // access to the event emitter later when the view is mounted. For now just save the event
         // in the view state and trigger it later.
         mMountingManager.enqueuePendingEvent(
-            reactTag, new ViewEvent(eventName, params, eventCategory, canCoalesceEvent));
+            surfaceId, reactTag, eventName, canCoalesceEvent, params, eventCategory);
       } else {
         // This can happen if the view has disappeared from the screen (because of async events)
         FLog.d(TAG, "Unable to invoke event: " + eventName + " for reactTag: " + reactTag);

diff --git a/...native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java b/...native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java
@@ -23,16 +23,17 @@
 import com.facebook.react.bridge.ReadableMap;
 import com.facebook.react.bridge.RetryableMountingLayerException;
 import com.facebook.react.bridge.UiThreadUtil;
+import com.facebook.react.bridge.WritableMap;
 import com.facebook.react.common.mapbuffer.MapBuffer;
 import com.facebook.react.fabric.FabricUIManager;
 import com.facebook.react.fabric.events.EventEmitterWrapper;
-import com.facebook.react.fabric.mounting.SurfaceMountingManager.ViewEvent;
 import com.facebook.react.fabric.mounting.mountitems.MountItem;
 import com.facebook.react.touch.JSResponderHandler;
 import com.facebook.react.uimanager.RootViewManager;
 import com.facebook.react.uimanager.ThemedReactContext;
 import com.facebook.react.uimanager.ViewManagerRegistry;
 import com.facebook.react.uimanager.common.ViewUtil;
+import com.facebook.react.uimanager.events.EventCategoryDef;
 import com.facebook.yoga.YogaMeasureMode;
 import java.util.Map;
 import java.util.Queue;
@@ -423,13 +424,18 @@ public long measureMapBuffer(
             attachmentsPositions);
   }
 
-  public void enqueuePendingEvent(int reactTag, ViewEvent viewEvent) {
-    @Nullable SurfaceMountingManager smm = getSurfaceManagerForView(reactTag);
+  public void enqueuePendingEvent(
+      int surfaceId,
+      int reactTag,
+      String eventName,
+      boolean canCoalesceEvent,
+      @Nullable WritableMap params,
+      @EventCategoryDef int eventCategory) {
+    @Nullable SurfaceMountingManager smm = getSurfaceManager(surfaceId);
     if (smm == null) {
       // Cannot queue event without valid surface mountng manager. Do nothing here.
       return;
     }
-
-    smm.enqueuePendingEvent(reactTag, viewEvent);
+    smm.enqueuePendingEvent(reactTag, eventName, canCoalesceEvent, params, eventCategory);
   }
 }
diff --git a/...ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java b/...ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java
@@ -15,6 +15,7 @@
 import android.view.ViewParent;
 import androidx.annotation.AnyThread;
 import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
 import androidx.annotation.UiThread;
 import androidx.collection.SparseArrayCompat;
 import com.facebook.common.logging.FLog;
@@ -59,7 +60,6 @@
 import java.util.Stack;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentLinkedQueue;
-import javax.annotation.Nullable;
 
 public class SurfaceMountingManager {
   public static final String TAG = SurfaceMountingManager.class.getSimpleName();
@@ -1118,25 +1118,16 @@ public void updateEventEmitter(int reactTag, @NonNull EventEmitterWrapper eventE
       previousEventEmitterWrapper.destroy();
     }
 
-    Queue<ViewEvent> pendingEventQueue = viewState.mPendingEventQueue;
+    Queue<PendingViewEvent> pendingEventQueue = viewState.mPendingEventQueue;
     if (pendingEventQueue != null) {
       // Invoke pending event queued to the view state
-      for (ViewEvent viewEvent : pendingEventQueue) {
-        dispatchEvent(eventEmitter, viewEvent);
+      for (PendingViewEvent viewEvent : pendingEventQueue) {
+        viewEvent.dispatch(eventEmitter);
       }
       viewState.mPendingEventQueue = null;
     }
   }
 
-  private void dispatchEvent(EventEmitterWrapper eventEmitter, ViewEvent viewEvent) {
-    if (viewEvent.canCoalesceEvent()) {
-      eventEmitter.dispatchUnique(viewEvent.getEventName(), viewEvent.getParams());
-    } else {
-      eventEmitter.dispatch(
-          viewEvent.getEventName(), viewEvent.getParams(), viewEvent.getEventCategory());
-    }
-  }
-
   @UiThread
   public synchronized void setJSResponder(
       int reactTag, int initialReactTag, boolean blockNativeResponder) {
@@ -1303,10 +1294,13 @@ public void printSurfaceState() {
     }
   }
 
-  @UiThread
-  public void enqueuePendingEvent(int reactTag, ViewEvent viewEvent) {
-    UiThreadUtil.assertOnUiThread();
-
+  @AnyThread
+  public void enqueuePendingEvent(
+      int reactTag,
+      String eventName,
+      boolean canCoalesceEvent,
+      @Nullable WritableMap params,
+      @EventCategoryDef int eventCategory) {
     // When the surface stopped we will reset the view state map. We are not going to enqueue
     // pending events as they are not expected to be dispatched anyways.
     if (mTagToViewState == null) {
@@ -1318,23 +1312,23 @@ public void enqueuePendingEvent(int reactTag, ViewEvent viewEvent) {
       // Cannot queue event without view state. Do nothing here.
       return;
     }
-    EventEmitterWrapper eventEmitter = viewState.mEventEmitter;
-    if (eventEmitter != null) {
-      // TODO T152630743: Verify threading for mEventEmitter
-      FLog.i(
-          TAG,
-          "Queue pending events when event emitter is null for the given view state, this should be dispatched instead - surfaceId: "
-              + mSurfaceId
-              + " reactTag: "
-              + reactTag);
-      dispatchEvent(eventEmitter, viewEvent);
-      return;
-    }
 
-    if (viewState.mPendingEventQueue == null) {
-      viewState.mPendingEventQueue = new LinkedList<>();
-    }
-    viewState.mPendingEventQueue.add(viewEvent);
+    PendingViewEvent viewEvent =
+        new PendingViewEvent(eventName, params, eventCategory, canCoalesceEvent);
+    UiThreadUtil.runOnUiThread(
+        new Runnable() {
+          @Override
+          public void run() {
+            if (viewState.mEventEmitter != null) {
+              viewEvent.dispatch(viewState.mEventEmitter);
+            } else {
+              if (viewState.mPendingEventQueue == null) {
+                viewState.mPendingEventQueue = new LinkedList<>();
+              }
+              viewState.mPendingEventQueue.add(viewEvent);
+            }
+          }
+        });
   }
 
   /**
@@ -1350,7 +1344,10 @@ private static class ViewState {
     @Nullable public ReadableMap mCurrentLocalData = null;
     @Nullable public StateWrapper mStateWrapper = null;
     @Nullable public EventEmitterWrapper mEventEmitter = null;
-    @Nullable public Queue<ViewEvent> mPendingEventQueue = null;
+
+    @ThreadConfined(UI)
+    @Nullable
+    public Queue<PendingViewEvent> mPendingEventQueue = null;
 
     private ViewState(
         int reactTag, @Nullable View view, @Nullable ReactViewManagerWrapper viewManager) {
@@ -1387,13 +1384,13 @@ public String toString() {
     }
   }
 
-  public static class ViewEvent {
+  private static class PendingViewEvent {
     private final String mEventName;
     private final boolean mCanCoalesceEvent;
     private final @EventCategoryDef int mEventCategory;
-    private @Nullable WritableMap mParams;
+    private final @Nullable WritableMap mParams;
 
-    public ViewEvent(
+    public PendingViewEvent(
         String eventName,
         @Nullable WritableMap params,
         @EventCategoryDef int eventCategory,
@@ -1404,20 +1401,12 @@ public ViewEvent(
       mCanCoalesceEvent = canCoalesceEvent;
     }
 
-    public String getEventName() {
-      return mEventName;
-    }
-
-    public boolean canCoalesceEvent() {
-      return mCanCoalesceEvent;
-    }
-
-    public @EventCategoryDef int getEventCategory() {
-      return mEventCategory;
-    }
-
-    public @Nullable WritableMap getParams() {
-      return mParams;
+    public void dispatch(EventEmitterWrapper eventEmitter) {
+      if (mCanCoalesceEvent) {
+        eventEmitter.dispatchUnique(mEventName, mParams);
+      } else {
+        eventEmitter.dispatch(mEventName, mParams, mEventCategory);
+      }
     }
   }