Skip to content

Commit

Permalink
Unmount React applications when re-creating context [RFC] (facebook#3…
Browse files Browse the repository at this point in the history 
…7004)

Summary:
Pull Request resolved: facebook#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

Differential Revision: D45145520

fbshipit-source-id: 1034c9201908d18a402adc6ee05fae0c2f3cd6d0
  • Loading branch information
@javache @facebook-github-bot
javache authored and facebook-github-bot committed Apr 21, 2023
1 parent 57c3692 commit 5c5a822
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 84 deletions.
89 changes: 65 additions & 24 deletions ...ages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -865,35 +868,37 @@ 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
// automatically when creation completes, as reactRoot is part of the attached
// reactRoot list.
ReactContext currentContext = getCurrentReactContext();
if (mCreateReactContextThread == null && currentContext != null) {
if (reactRoot.getState().compareAndSet(ReactRoot.STATE_STOPPED, ReactRoot.STATE_STARTED)) {
attachRootViewToInstance(reactRoot);
}
attachRootViewToInstance(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);
}
}

Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand Down Expand Up @@ -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)
Expand All @@ -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);
}
}
}

Expand Down
35 changes: 0 additions & 35 deletions packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 6 additions & 0 deletions .../react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
25 changes: 0 additions & 25 deletions ...eact-native/ReactAndroid/src/main/java/com/facebook/react/modules/fabric/ReactFabric.java
View file

This file was deleted.

0 comments on commit 5c5a822

Please sign in to comment.