From 50e12c9d27cff182f7513410411e832acd8704b5 Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Mon, 24 Apr 2023 03:58:37 -0700 Subject: [PATCH] Unmount React applications when re-creating context [RFC] (#37004) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/37004 Currently in RN Android when re-creating the context we don't unmount the underlying React application. This violates ViewManager and React hooks contracts, who are no longer able to properly unmount views, and instead the view hierarchy is forcefully torn down by Android UI. This differs from iOS, where we do unmount the application on reloads. This is a trade-off with performance, as we'll keep the JS thread alive slightly longer to complete shutdown, but it's the right call for correctness. It also only mainly affects development, as recreating the context is rare in production. Repro steps: ``` useEffect(() => { console.log('Playground useEffect invoked'); return () => { console.log('Playground useEffect destructor invoked'); }; }, []); ``` Validate that when reloading the application, the second console.log is printed. Changelog: [Android][Changed] React trees will be unmounted when the application is reloaded Reviewed By: luluwu2032 Differential Revision: D45145520 fbshipit-source-id: a4dcd2ff4a8fc14cb0f276a5ef9afe21d1104735 --- .../facebook/react/ReactInstanceManager.java | 89 ++++++++++++++----- .../com/facebook/react/ReactRootView.java | 35 -------- .../react/config/ReactFeatureFlags.java | 6 ++ .../react/modules/fabric/ReactFabric.java | 25 ------ 4 files changed, 71 insertions(+), 84 deletions(-) delete mode 100644 packages/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/fabric/ReactFabric.java diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java index c7804a09eb6a57..c2a23b2c4c80f2 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java @@ -92,7 +92,6 @@ import com.facebook.react.modules.core.DeviceEventManagerModule; import com.facebook.react.modules.core.ReactChoreographer; import com.facebook.react.modules.debug.interfaces.DeveloperSettings; -import com.facebook.react.modules.fabric.ReactFabric; import com.facebook.react.packagerconnection.RequestHandler; import com.facebook.react.surface.ReactStage; import com.facebook.react.turbomodule.core.TurboModuleManager; @@ -102,6 +101,7 @@ import com.facebook.react.uimanager.ReactRoot; import com.facebook.react.uimanager.UIManagerHelper; import com.facebook.react.uimanager.ViewManager; +import com.facebook.react.uimanager.common.UIManagerType; import com.facebook.react.views.imagehelper.ResourceDrawableIdHelper; import com.facebook.soloader.SoLoader; import com.facebook.systrace.Systrace; @@ -855,7 +855,10 @@ private void clearReactRoot(ReactRoot reactRoot) { * with the provided reactRoot reactRoot will be started asynchronously, i.e this method won't * block. This reactRoot will then be tracked by this manager and in case of catalyst instance * restart it will be re-attached. + * + * @deprecated This method should be internal to ReactRootView and ReactInstanceManager */ + @Deprecated @ThreadConfined(UI) public void attachRootView(ReactRoot reactRoot) { UiThreadUtil.assertOnUiThread(); @@ -865,6 +868,8 @@ public void attachRootView(ReactRoot reactRoot) { // Ideally reactRoot should be initialized with id == NO_ID if (mAttachedReactRoots.add(reactRoot)) { clearReactRoot(reactRoot); + } else { + FLog.e(ReactConstants.TAG, "ReactRoot was attached multiple times"); } // If react context is being created in the background, JS application will be started @@ -872,9 +877,7 @@ public void attachRootView(ReactRoot reactRoot) { // reactRoot list. ReactContext currentContext = getCurrentReactContext(); if (mCreateReactContextThread == null && currentContext != null) { - if (reactRoot.getState().compareAndSet(ReactRoot.STATE_STOPPED, ReactRoot.STATE_STARTED)) { - attachRootViewToInstance(reactRoot); - } + attachRootViewToInstance(reactRoot); } } @@ -882,18 +885,20 @@ public void attachRootView(ReactRoot reactRoot) { * Detach given {@param reactRoot} from current catalyst instance. It's safe to call this method * multiple times on the same {@param reactRoot} - in that case view will be detached with the * first call. + * + * @deprecated This method should be internal to ReactRootView and ReactInstanceManager */ + @Deprecated @ThreadConfined(UI) public void detachRootView(ReactRoot reactRoot) { UiThreadUtil.assertOnUiThread(); - synchronized (mAttachedReactRoots) { - if (mAttachedReactRoots.contains(reactRoot)) { - ReactContext currentContext = getCurrentReactContext(); - mAttachedReactRoots.remove(reactRoot); - if (currentContext != null && currentContext.hasActiveReactInstance()) { - detachViewFromInstance(reactRoot, currentContext.getCatalystInstance()); - } - } + if (!mAttachedReactRoots.remove(reactRoot)) { + return; + } + + ReactContext reactContext = mCurrentReactContext; + if (reactContext != null && reactContext.hasActiveReactInstance()) { + detachRootViewFromInstance(reactRoot, reactContext); } } @@ -1150,9 +1155,7 @@ private void setupReactContext(final ReactApplicationContext reactContext) { ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_START); for (ReactRoot reactRoot : mAttachedReactRoots) { - if (reactRoot.getState().compareAndSet(ReactRoot.STATE_STOPPED, ReactRoot.STATE_STARTED)) { - attachRootViewToInstance(reactRoot); - } + attachRootViewToInstance(reactRoot); } ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_END); } @@ -1194,6 +1197,11 @@ private void setupReactContext(final ReactApplicationContext reactContext) { private void attachRootViewToInstance(final ReactRoot reactRoot) { FLog.d(ReactConstants.TAG, "ReactInstanceManager.attachRootViewToInstance()"); + if (!reactRoot.getState().compareAndSet(ReactRoot.STATE_STOPPED, ReactRoot.STATE_STARTED)) { + // Already started + return; + } + Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "attachRootViewToInstance"); @Nullable @@ -1210,7 +1218,6 @@ private void attachRootViewToInstance(final ReactRoot reactRoot) { @Nullable Bundle initialProperties = reactRoot.getAppProperties(); final int rootTag; - if (reactRoot.getUIManagerType() == FABRIC) { rootTag = uiManager.startSurface( @@ -1245,18 +1252,48 @@ private void attachRootViewToInstance(final ReactRoot reactRoot) { Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); } - private void detachViewFromInstance(ReactRoot reactRoot, CatalystInstance catalystInstance) { - FLog.d(ReactConstants.TAG, "ReactInstanceManager.detachViewFromInstance()"); + private void detachRootViewFromInstance(ReactRoot reactRoot, ReactContext reactContext) { + FLog.d(ReactConstants.TAG, "ReactInstanceManager.detachRootViewFromInstance()"); UiThreadUtil.assertOnUiThread(); - if (reactRoot.getUIManagerType() == FABRIC) { - catalystInstance - .getJSModule(ReactFabric.class) - .unmountComponentAtNode(reactRoot.getRootViewTag()); + + if (!reactRoot.getState().compareAndSet(ReactRoot.STATE_STARTED, ReactRoot.STATE_STOPPED)) { + // ReactRoot was already stopped + return; + } + + @UIManagerType int uiManagerType = reactRoot.getUIManagerType(); + if (uiManagerType == UIManagerType.FABRIC) { + // Stop surface in Fabric. + // Calling FabricUIManager.stopSurface causes the C++ Binding.stopSurface + // to be called synchronously over the JNI, which causes an empty tree + // to be committed via the Scheduler, which will cause mounting instructions + // to be queued up and synchronously executed to delete and remove + // all the views in the hierarchy. + final int surfaceId = reactRoot.getRootViewTag(); + if (surfaceId != View.NO_ID) { + UIManager uiManager = UIManagerHelper.getUIManager(reactContext, uiManagerType); + if (uiManager != null) { + uiManager.stopSurface(surfaceId); + } else { + FLog.w(ReactConstants.TAG, "Failed to stop surface, UIManager has already gone away"); + reactRoot.getRootViewGroup().removeAllViews(); + } + } else { + ReactSoftExceptionLogger.logSoftException( + TAG, + new RuntimeException( + "detachRootViewFromInstance called with ReactRootView with invalid id")); + reactRoot.getRootViewGroup().removeAllViews(); + } } else { - catalystInstance + reactContext + .getCatalystInstance() .getJSModule(AppRegistry.class) .unmountApplicationComponentAtRootTag(reactRoot.getRootViewTag()); } + + // The view is no longer attached, so mark it as such by resetting its ID. + reactRoot.getRootViewGroup().setId(View.NO_ID); } @ThreadConfined(UI) @@ -1269,7 +1306,11 @@ private void tearDownReactContext(ReactContext reactContext) { synchronized (mAttachedReactRoots) { for (ReactRoot reactRoot : mAttachedReactRoots) { - clearReactRoot(reactRoot); + if (ReactFeatureFlags.unmountApplicationOnInstanceDetach) { + detachRootViewFromInstance(reactRoot, reactContext); + } else { + clearReactRoot(reactRoot); + } } } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java index 8be37b052acbd3..f0a0b7dbef5a23 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java @@ -611,41 +611,6 @@ private void updateRootLayoutSpecs( @ThreadConfined(UI) public void unmountReactApplication() { UiThreadUtil.assertOnUiThread(); - // Stop surface in Fabric. - // Calling FabricUIManager.stopSurface causes the C++ Binding.stopSurface - // to be called synchronously over the JNI, which causes an empty tree - // to be committed via the Scheduler, which will cause mounting instructions - // to be queued up and synchronously executed to delete and remove - // all the views in the hierarchy. - if (hasActiveReactInstance()) { - final ReactContext reactApplicationContext = getCurrentReactContext(); - if (reactApplicationContext != null && isFabric()) { - @Nullable - UIManager uiManager = - UIManagerHelper.getUIManager(reactApplicationContext, getUIManagerType()); - if (uiManager != null) { - final int surfaceId = this.getId(); - - // In case of "retry" or error dialogues being shown, this ReactRootView could be - // reused (with the same surfaceId, or a different one). Ensure the ReactRootView - // is marked as unused in case of that. - setId(NO_ID); - - // Remove all children from ReactRootView - removeAllViews(); - - if (surfaceId == NO_ID) { - ReactSoftExceptionLogger.logSoftException( - TAG, - new RuntimeException( - "unmountReactApplication called on ReactRootView with invalid id")); - } else { - uiManager.stopSurface(surfaceId); - } - } - } - } - if (mReactInstanceManager != null && mIsAttachedToInstance) { mReactInstanceManager.detachRootView(this); mIsAttachedToInstance = false; diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index 2c5adaac55de57..9884ba9e687e28 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -141,4 +141,10 @@ public class ReactFeatureFlags { * HostObject pattern */ public static boolean useNativeState = false; + + /** + * Unmount React application on ReactInstance detach. Controls rollout of change to align React + * application lifecycle with React Native instance. + */ + public static boolean unmountApplicationOnInstanceDetach = false; } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/fabric/ReactFabric.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/fabric/ReactFabric.java deleted file mode 100644 index 46bd31ba032dd7..00000000000000 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/fabric/ReactFabric.java +++ /dev/null @@ -1,25 +0,0 @@ -/* - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -package com.facebook.react.modules.fabric; - -import com.facebook.react.bridge.JavaScriptModule; - -/** - * JS module used to execute Fabric specific methods. Note: This is a temporary class that will be - * replaced when Fabric is fully integrated with the rest of the modules. - */ -public interface ReactFabric extends JavaScriptModule { - - /** - * JS method used to unmount Fabric surfaces. - * - * @param rootTag {@link int} react tag of Root {@link - * com.facebook.react.uimanager.ReactShadowNode} - */ - void unmountComponentAtNode(int rootTag); -}