From 74a756846fdab1ef7d183c4df3069a23fcd0d49e Mon Sep 17 00:00:00 2001 From: David Vacca Date: Fri, 4 Dec 2020 19:55:54 -0800 Subject: [PATCH] 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 --- .../facebook/react/ReactInstanceManager.java | 29 +++++++++++++++---- .../com/facebook/react/ReactRootView.java | 6 ++++ .../react/config/ReactFeatureFlags.java | 6 ++++ .../facebook/react/uimanager/ReactRoot.java | 14 +++++++++ 4 files changed, 50 insertions(+), 5 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java index d685867afbbf77..95026e8385f86b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java +++ b/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); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java b/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java index d15ad24067a87d..bac18eed25ef04 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java +++ b/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); diff --git a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index 7d1729782db3f8..260d366e29019a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/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; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactRoot.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactRoot.java index 2fba8c52164c75..9ce17ea680954a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactRoot.java +++ b/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(); }