Fix race condition on startSurface

Summary:
The root cause of this bug is a race condition between the onMeasure method and setupReactContext.

ReactInstanceManager.attachRootView() method is responsible of the initialization of ReactRootViews and it is invoked by ReactRootView.onMeasure() method in the UIThread

Important initialization steps:
1. Clear the Id of the ReactRootView
2. Add the ReactRootView to the mAttachedReactRoots
3. Call StartSurface (if the bridge has been initialized)

Sometimes, when this method is invoked for the first time, the bridge is not initialized, in those cases we delay the start of the surface.

Once the bridge is initialized, StartSurface is called by the setupReactContext() running in the NativeModuleThread.

Since onMeasure can be called multiple times, it is possible that we call "StartSurface" twice for the same ReactRootView, causing the bug reported on T78832286.

This diff adds an extra check to prevent calling "StartSurface" twice. The fix is done using an AtomicInteger comparison and it is gated by the flag "enableStartSurfaceRaceConditionFix". Once we verify this works fine in production we will clean up the code, remove the flags and maybe revisit the API of ReactRoot.

changelog: [Android] Fix race-condition on the initialization of ReactRootViews

Reviewed By: JoshuaGross

Differential Revision: D25255877

fbshipit-source-id: ca8fb00f50e86891fb4c5a06240177cc1a0186d9

ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java

@@ -795,6 +795,9 @@ public void showDevOptionsDialog() {
   }
 
   private void clearReactRoot(ReactRoot reactRoot) {
+    if (ReactFeatureFlags.enableStartSurfaceRaceConditionFix) {
+      reactRoot.getState().compareAndSet(ReactRoot.STATE_STARTED, ReactRoot.STATE_STOPPED);
+    }
     reactRoot.getRootViewGroup().removeAllViews();
     reactRoot.getRootViewGroup().setId(View.NO_ID);
   }
@@ -810,17 +813,28 @@ private void clearReactRoot(ReactRoot reactRoot) {
   @ThreadConfined(UI)
   public void attachRootView(ReactRoot reactRoot) {
     UiThreadUtil.assertOnUiThread();
-    mAttachedReactRoots.add(reactRoot);
 
-    // Reset reactRoot content as it's going to be populated by the application content from JS.
-    clearReactRoot(reactRoot);
+    // Calling clearReactRoot is necessary to initialize the Id on reactRoot
+    // This is necessary independently if the RN Bridge has been initialized or not.
+    // Ideally reactRoot should be initialized with id == NO_ID
+    if (ReactFeatureFlags.enableStartSurfaceRaceConditionFix) {
+      if (mAttachedReactRoots.add(reactRoot)) {
+        clearReactRoot(reactRoot);
+      }
+    } else {
+      mAttachedReactRoots.add(reactRoot);
+      clearReactRoot(reactRoot);
+    }
 
     // If react context is being created in the background, JS application will be started
     // automatically when creation completes, as reactRoot reactRoot is part of the attached
     // reactRoot reactRoot list.
     ReactContext currentContext = getCurrentReactContext();
     if (mCreateReactContextThread == null && currentContext != null) {
-      attachRootViewToInstance(reactRoot);
+      if (!ReactFeatureFlags.enableStartSurfaceRaceConditionFix
+          || reactRoot.getState().compareAndSet(ReactRoot.STATE_STOPPED, ReactRoot.STATE_STARTED)) {
+        attachRootViewToInstance(reactRoot);
+      }
     }
   }
 
@@ -1087,7 +1101,12 @@ private void setupReactContext(final ReactApplicationContext reactContext) {
 
       ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_START);
       for (ReactRoot reactRoot : mAttachedReactRoots) {
-        attachRootViewToInstance(reactRoot);
+        if (!ReactFeatureFlags.enableStartSurfaceRaceConditionFix
+            || reactRoot
+                .getState()
+                .compareAndSet(ReactRoot.STATE_STOPPED, ReactRoot.STATE_STARTED)) {
+          attachRootViewToInstance(reactRoot);
+        }
       }
       ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_END);
     }

ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java

@@ -56,6 +56,7 @@
 import com.facebook.react.uimanager.common.UIManagerType;
 import com.facebook.react.uimanager.events.EventDispatcher;
 import com.facebook.systrace.Systrace;
+import java.util.concurrent.atomic.AtomicInteger;
 
 /**
  * Default root view for catalyst apps. Provides the ability to listen for size changes so that a UI
@@ -98,6 +99,7 @@ public interface ReactRootViewEventListener {
   private int mLastOffsetX = Integer.MIN_VALUE;
   private int mLastOffsetY = Integer.MIN_VALUE;
   private @UIManagerType int mUIManagerType = DEFAULT;
+  private final AtomicInteger mState = new AtomicInteger(STATE_STOPPED);
 
   public ReactRootView(Context context) {
     super(context);
@@ -413,6 +415,10 @@ public String getSurfaceID() {
     return appProperties != null ? appProperties.getString("surfaceID") : null;
   }
 
+  public AtomicInteger getState() {
+    return mState;
+  }
+
   public static Point getViewportOffset(View v) {
     int[] locationInWindow = new int[2];
     v.getLocationInWindow(locationInWindow);

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

@@ -91,4 +91,10 @@ public class ReactFeatureFlags {
 
   /** Disable UI update operations in non-Fabric renderer after catalyst instance was destroyed */
   public static boolean disableNonFabricViewOperationsOnCatalystDestroy = false;
+
+  /**
+   * Fixes race-condition in the initialization of RN surface. TODO T78832286: remove this flag once
+   * we verify the fix is correct in production
+   */
+  public static boolean enableStartSurfaceRaceConditionFix = false;
 }

ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactRoot.java

@@ -11,10 +11,16 @@
 import android.view.ViewGroup;
 import androidx.annotation.Nullable;
 import com.facebook.react.uimanager.common.UIManagerType;
+import java.util.concurrent.atomic.AtomicInteger;
 
 /** Interface for the root native view of a React native application */
 public interface ReactRoot {
 
+  /** This constant represents that ReactRoot hasn't started yet or it has been destroyed. */
+  int STATE_STOPPED = 0;
+  /** This constant represents that ReactRoot has started. */
+  int STATE_STARTED = 1;
+
   /** Return cached launch properties for app */
   @Nullable
   Bundle getAppProperties();
@@ -58,4 +64,12 @@ public interface ReactRoot {
   @Deprecated
   @Nullable
   String getSurfaceID();
+
+  /**
+   * This API is likely to change once the fix of T78832286 is confirmed TODO: T78832286 revisit
+   * this API
+   *
+   * @return an {@link AtomicInteger} that represents the state of the ReactRoot object. WARNING:
+   */
+  AtomicInteger getState();
 }