diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java index 31f95bf7cad..740809e39ea 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -253,9 +253,16 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { @AnyThread @ThreadConfined(ANY) @Override - public void stopSurface(int surfaceID) { + public void stopSurface(final int surfaceID) { mReactContextForRootTag.remove(surfaceID); mBinding.stopSurface(surfaceID); + UiThreadUtil.runOnUiThread( + new Runnable() { + @Override + public void run() { + mMountingManager.deleteRootView(surfaceID); + } + }); } @Override @@ -876,7 +883,6 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { BatchMountItem batchMountItem = (BatchMountItem) mountItem; if (!surfaceActiveForExecution( batchMountItem.getRootTag(), "dispatchMountItems BatchMountItem")) { - batchMountItem.executeDeletes(mMountingManager); continue; } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java index 3e1a234246c..db364e1696f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java @@ -101,12 +101,20 @@ public class MountingManager { rootView.setId(reactRootTag); } + /** Delete rootView and all children/ */ + @UiThread + public void deleteRootView(int reactRootTag) { + if (mTagToViewState.containsKey(reactRootTag)) { + dropView(mTagToViewState.get(reactRootTag).mView, true); + } + } + /** Releases all references to given native View. */ @UiThread - private void dropView(@NonNull View view) { + private void dropView(@NonNull View view, boolean deleteImmediately) { UiThreadUtil.assertOnUiThread(); - int reactTag = view.getId(); + final int reactTag = view.getId(); ViewState state = getViewState(reactTag); ViewManager viewManager = state.mViewManager; @@ -115,29 +123,75 @@ public class MountingManager { viewManager.onDropViewInstance(view); } if (view instanceof ViewGroup && viewManager instanceof ViewGroupManager) { - ViewGroup viewGroup = (ViewGroup) view; - ViewGroupManager viewGroupManager = getViewGroupManager(state); - for (int i = viewGroupManager.getChildCount(viewGroup) - 1; i >= 0; i--) { - View child = viewGroupManager.getChildAt(viewGroup, i); - if (getNullableViewState(child.getId()) != null) { - if (SHOW_CHANGED_VIEW_HIERARCHIES) { - FLog.e( - TAG, - "Automatically dropping view that is still attached to a parent being dropped. Parent: [" - + reactTag - + "] child: [" - + child.getId() - + "]"); - } - dropView(child); - } - viewGroupManager.removeViewAt(viewGroup, i); + final ViewGroup viewGroup = (ViewGroup) view; + final ViewGroupManager viewGroupManager = getViewGroupManager(state); + + // As documented elsewhere, sometimes when a child is removed from a parent, that change + // is not immediately available in the hierarchy until a future UI tick. This can cause + // inconsistent child counts, etc, but it can _also_ cause us to drop views that shouldn't, + // because they're removed from the parent but that change isn't immediately visible. So, + // we do two things: 1) delay this logic until the next UI thread tick, 2) ignore children + // who don't report the expected parent. + // For most cases, we _do not_ want this logic to run, anyway, since it either means that we + // don't have a correct set of MountingInstructions; or it means that we're tearing down an + // entire screen, in which case we can safely delete everything immediately, not having + // executed any remove instructions immediately before this. + if (deleteImmediately) { + dropChildren(reactTag, viewGroup, viewGroupManager); + } else { + UiThreadUtil.runOnUiThread( + new Runnable() { + @Override + public void run() { + dropChildren(reactTag, viewGroup, viewGroupManager); + } + }); } } mTagToViewState.remove(reactTag); } + @UiThread + private void dropChildren( + int reactTag, + @NonNull ViewGroup viewGroup, + @NonNull ViewGroupManager viewGroupManager) { + for (int i = viewGroupManager.getChildCount(viewGroup) - 1; i >= 0; i--) { + View child = viewGroupManager.getChildAt(viewGroup, i); + if (getNullableViewState(child.getId()) != null) { + if (SHOW_CHANGED_VIEW_HIERARCHIES) { + FLog.e( + TAG, + "Automatically dropping view that is still attached to a parent being dropped. Parent: [" + + reactTag + + "] child: [" + + child.getId() + + "]"); + } + ViewParent childParent = child.getParent(); + if (childParent == null || !childParent.equals(viewGroup)) { + int childParentId = + (childParent == null + ? -1 + : (childParent instanceof ViewGroup ? ((ViewGroup) childParent).getId() : -1)); + FLog.e( + TAG, + "Recursively deleting children of [" + + reactTag + + "] but parent of child [" + + child.getId() + + "] is [" + + childParentId + + "]"); + } else { + dropView(child, true); + } + } + viewGroupManager.removeViewAt(viewGroup, i); + } + } + @UiThread public void addViewAt(final int parentTag, final int tag, final int index) { UiThreadUtil.assertOnUiThread(); @@ -491,7 +545,7 @@ public class MountingManager { View view = viewState.mView; if (view != null) { - dropView(view); + dropView(view, false); } else { mTagToViewState.remove(reactTag); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/BatchMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/BatchMountItem.java index f4422ca54e0..d685be1ae02 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/BatchMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/BatchMountItem.java @@ -8,7 +8,6 @@ package com.facebook.react.fabric.mounting.mountitems; import androidx.annotation.NonNull; -import com.facebook.common.logging.FLog; import com.facebook.proguard.annotations.DoNotStrip; import com.facebook.react.bridge.ReactMarker; import com.facebook.react.bridge.ReactMarkerConstants; @@ -72,32 +71,7 @@ public class BatchMountItem implements MountItem { beginMarkers("mountViews"); for (int mountItemIndex = 0; mountItemIndex < mSize; mountItemIndex++) { - MountItem mountItem = mMountItems[mountItemIndex]; - mountItem.execute(mountingManager); - } - - endMarkers(); - } - - /** - * In the case of teardown/stopSurface, we want to delete all views associated with a SurfaceID. - * It can be the case that a single BatchMountItem contains both the create *and* delete - * instruction for a view, so this needs to be failsafe. - * - * @param mountingManager - */ - public void executeDeletes(@NonNull MountingManager mountingManager) { - beginMarkers("deleteViews"); - - for (int mountItemIndex = 0; mountItemIndex < mSize; mountItemIndex++) { - MountItem mountItem = mMountItems[mountItemIndex]; - if (mountItem instanceof RemoveDeleteMultiMountItem) { - try { - ((RemoveDeleteMultiMountItem) mountItem).executeDeletes(mountingManager, true); - } catch (RuntimeException e) { - FLog.e(TAG, "Ignoring deletion exception", e); - } - } + mMountItems[mountItemIndex].execute(mountingManager); } endMarkers(); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/RemoveDeleteMultiMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/RemoveDeleteMultiMountItem.java index 8a25f84b743..daf3ed5ca75 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/RemoveDeleteMultiMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/RemoveDeleteMultiMountItem.java @@ -56,30 +56,11 @@ public class RemoveDeleteMultiMountItem implements MountItem { } // After removing all views, delete all views marked for deletion. - executeDeletes(mountingManager, false); - } - - /** - * Execute only deletion operations. When being executed as part of shutdown/stopping surface, - * deletion failures can be ignored. For example: if there is a batch of MountItems being executed - * as part of stopSurface, a "Create" that is not executed may have a matching "Delete". The - * Delete will fail but we can safely ignore it in those cases. - * - * @param mountingManager - * @param ignoreFailures - */ - public void executeDeletes(@NonNull MountingManager mountingManager, boolean ignoreFailures) { for (int i = 0; i < mMetadata.length; i += 4) { int flags = mMetadata[i + FLAGS_INDEX]; if ((flags & DELETE_FLAG) != 0) { int tag = mMetadata[i + TAG_INDEX]; - try { - mountingManager.deleteView(tag); - } catch (IllegalStateException e) { - if (!ignoreFailures) { - throw e; - } - } + mountingManager.deleteView(tag); } } }