From 792f6f69c913f82af0b9cd8145738deeae5dfa17 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Thu, 27 Aug 2020 01:30:08 -0700 Subject: [PATCH] New StopSurface deleteView mechanism Summary: Simplifying the StopSurface flow. Before we would still attempt to execute MountItems, but only the "Delete" operations. This was... well, frankly, overcomplicated. Instead we can just ignore all future MountInstructions for that Surface and delete all views recursively from the root. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D23338752 fbshipit-source-id: 6e7ab29ad85572782bfc6a39845a8a619f001559 --- .../react/fabric/FabricUIManager.java | 10 +- .../fabric/mounting/MountingManager.java | 94 +++++++++++++++---- .../mounting/mountitems/BatchMountItem.java | 28 +----- .../RemoveDeleteMultiMountItem.java | 21 +---- 4 files changed, 84 insertions(+), 69 deletions(-) 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); } } }