From 20b4879dfd34716997e92bb5d89b0e4f873fef98 Mon Sep 17 00:00:00 2001 From: Luna Wei Date: Tue, 26 Mar 2019 10:19:26 -0700 Subject: [PATCH] - LayoutAnimations cause invalid view operations Summary: [Android] [Fixed] - LayoutAnimations cause invalid view operations The dating team has found a consistent repro where following an order of steps will get you the following exception: https://lookaside.facebook.com/intern/pixelcloudnew/asset/?id=2113362972287761 The exception is due to the fact that the following operation `delete child tag 17 from parent tag 481 which is located at index 2` cannot be performed because parent tag 481 only has 2 children.. and they also happen to not have the tag 17 as a child. Somehow the operations and the tree they act upon are out of sync. RN receives operations from React via the native module `UIManagerModule`. The operations use tags (an identifier for a view) and indices to determine what view is updated. These operations (grouped together as a batch) are then passed to the UI thread where we perform them on the platform views. LayoutAnimations are implemented per batch in RN. When LayoutAnimations are on, qualified view updates are animated. Because the delete operation is animated, RN doesn't remove the view from the platform view hierarchy immediately but asynchronously -- after the animation is complete. This is problematic for other view operations that rely on an index on where to insert or delete a view because during the creation of those operations, it was assumed all prior operations were performed synchronously. This is what was occurring in the repro and there were silent view errors occurring before the exception was thrown. This diff proposes a solution to track all pending delete operations across operations. We introduce a map that is keyed on view tags and has a value of a sparse array that represents child index -> count of views that are pending deletion. `{11: [0=1], 481: [2=1]}` where this would be interpreted as for index 0 under view tag 11, there is one async view deletion that has not completed and under tag 481 there is one async view deletion at index 2. We use the map to adjust indices on add / delete operations. We also update the count when the deletion animation is complete and remove the tag from the map when it is deleted. Regarding worst case sizing of the map => O(# of platform views rendered) Reviewed By: mdvacca Differential Revision: D14529038 fbshipit-source-id: 86d8982845e25a2b23d6d89ce27852fd77dbb060 --- .../uimanager/NativeViewHierarchyManager.java | 67 ++++++++++++++++--- .../NativeViewHierarchyOptimizer.java | 14 ++-- .../react/uimanager/UIImplementation.java | 5 +- .../react/uimanager/UIViewOperationQueue.java | 13 ++-- 4 files changed, 78 insertions(+), 21 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java index 0d49abf79a0..e846052e09f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java @@ -11,6 +11,7 @@ import android.content.res.Resources; import android.os.Build; import android.util.SparseArray; import android.util.SparseBooleanArray; +import android.util.SparseIntArray; import android.view.Menu; import android.view.MenuItem; import android.view.View; @@ -35,6 +36,8 @@ import com.facebook.react.uimanager.layoutanimation.LayoutAnimationController; import com.facebook.react.uimanager.layoutanimation.LayoutAnimationListener; import com.facebook.systrace.Systrace; import com.facebook.systrace.SystraceMessage; +import java.util.HashMap; +import java.util.Map; import javax.annotation.Nullable; import javax.annotation.concurrent.NotThreadSafe; @@ -73,6 +76,7 @@ public class NativeViewHierarchyManager { private final JSResponderHandler mJSResponderHandler = new JSResponderHandler(); private final RootViewManager mRootViewManager; private final LayoutAnimationController mLayoutAnimator = new LayoutAnimationController(); + private final Map mTagsToPendingIndicesToDelete = new HashMap<>(); private boolean mLayoutAnimationEnabled; private PopupMenu mPopupMenu; @@ -336,19 +340,49 @@ public class NativeViewHierarchyManager { return stringBuilder.toString(); } + /** + * Given an index to action on under synchronous deletes, return an updated index factoring in + * asynchronous deletes (where the async delete operations have not yet been performed) + */ + private int normalizeIndex(int index, SparseIntArray pendingIndices) { + int normalizedIndex = index; + for (int i = 0; i <= index; i++) { + normalizedIndex += pendingIndices.get(i); + } + return normalizedIndex; + } + + /** + * Given React tag, return sparse array of direct child indices that are pending deletion (due to + * async view deletion) + */ + private SparseIntArray getOrCreatePendingIndicesToDelete(int tag) { + SparseIntArray pendingIndicesToDelete = mTagsToPendingIndicesToDelete.get(tag); + if (pendingIndicesToDelete == null) { + pendingIndicesToDelete = new SparseIntArray(); + mTagsToPendingIndicesToDelete.put(tag, pendingIndicesToDelete); + } + return pendingIndicesToDelete; + } + /** * @param tag react tag of the node we want to manage * @param indicesToRemove ordered (asc) list of indicies at which view should be removed * @param viewsToAdd ordered (asc based on mIndex property) list of tag-index pairs that represent * a view which should be added at the specified index * @param tagsToDelete list of tags corresponding to views that should be removed + * @param indicesToDelete parallel list to tagsToDelete, list of indices of those tags */ public synchronized void manageChildren( int tag, @Nullable int[] indicesToRemove, @Nullable ViewAtIndex[] viewsToAdd, - @Nullable int[] tagsToDelete) { + @Nullable int[] tagsToDelete, + @Nullable int[] indicesToDelete) { UiThreadUtil.assertOnUiThread(); + + final SparseIntArray pendingIndicesToDelete = getOrCreatePendingIndicesToDelete(tag); + final ViewGroup viewToManage = (ViewGroup) mTagsToViews.get(tag); final ViewGroupManager viewManager = (ViewGroupManager) resolveViewManager(tag); if (viewToManage == null) { @@ -405,7 +439,8 @@ public class NativeViewHierarchyManager { tagsToDelete)); } - View viewToRemove = viewManager.getChildAt(viewToManage, indexToRemove); + int normalizedIndexToRemove = normalizeIndex(indexToRemove, pendingIndicesToDelete); + View viewToRemove = viewManager.getChildAt(viewToManage, normalizedIndexToRemove); if (mLayoutAnimationEnabled && mLayoutAnimator.shouldAnimateLayout(viewToRemove) && @@ -413,7 +448,7 @@ public class NativeViewHierarchyManager { // The view will be removed and dropped by the 'delete' layout animation // instead, so do nothing } else { - viewManager.removeViewAt(viewToManage, indexToRemove); + viewManager.removeViewAt(viewToManage, normalizedIndexToRemove); } lastIndexToRemove = indexToRemove; @@ -435,13 +470,15 @@ public class NativeViewHierarchyManager { viewsToAdd, tagsToDelete)); } - viewManager.addView(viewToManage, viewToAdd, viewAtIndex.mIndex); + int normalizedIndexToAdd = normalizeIndex(viewAtIndex.mIndex, pendingIndicesToDelete); + viewManager.addView(viewToManage, viewToAdd, normalizedIndexToAdd); } } if (tagsToDelete != null) { for (int i = 0; i < tagsToDelete.length; i++) { int tagToDelete = tagsToDelete[i]; + final int indexToDelete = indicesToDelete[i]; final View viewToDestroy = mTagsToViews.get(tagToDelete); if (viewToDestroy == null) { throw new IllegalViewOperationException( @@ -457,13 +494,20 @@ public class NativeViewHierarchyManager { if (mLayoutAnimationEnabled && mLayoutAnimator.shouldAnimateLayout(viewToDestroy)) { - mLayoutAnimator.deleteView(viewToDestroy, new LayoutAnimationListener() { - @Override - public void onAnimationEnd() { - viewManager.removeView(viewToManage, viewToDestroy); - dropView(viewToDestroy); - } - }); + int updatedCount = pendingIndicesToDelete.get(indexToDelete, 0) + 1; + pendingIndicesToDelete.put(indexToDelete, updatedCount); + mLayoutAnimator.deleteView( + viewToDestroy, + new LayoutAnimationListener() { + @Override + public void onAnimationEnd() { + viewManager.removeView(viewToManage, viewToDestroy); + dropView(viewToDestroy); + + int count = pendingIndicesToDelete.get(indexToDelete, 0); + pendingIndicesToDelete.put(indexToDelete, Math.max(0, count - 1)); + } + }); } else { dropView(viewToDestroy); } @@ -580,6 +624,7 @@ public class NativeViewHierarchyManager { } viewGroupManager.removeAllViews(viewGroup); } + mTagsToPendingIndicesToDelete.remove(view.getId()); mTagsToViews.remove(view.getId()); mTagsToViewManagers.remove(view.getId()); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyOptimizer.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyOptimizer.java index 084006fe465..f371f7ed0a9 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyOptimizer.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyOptimizer.java @@ -145,13 +145,15 @@ public class NativeViewHierarchyOptimizer { int[] indicesToRemove, int[] tagsToRemove, ViewAtIndex[] viewsToAdd, - int[] tagsToDelete) { + int[] tagsToDelete, + int[] indicesToDelete) { if (!ENABLED) { mUIViewOperationQueue.enqueueManageChildren( nodeToManage.getReactTag(), indicesToRemove, viewsToAdd, - tagsToDelete); + tagsToDelete, + indicesToDelete); return; } @@ -276,9 +278,10 @@ public class NativeViewHierarchyOptimizer { mUIViewOperationQueue.enqueueManageChildren( nativeNodeToRemoveFrom.getReactTag(), - new int[]{index}, + new int[] {index}, null, - shouldDelete ? new int[]{nodeToRemove.getReactTag()} : null); + shouldDelete ? new int[] {nodeToRemove.getReactTag()} : null, + shouldDelete ? new int[] {index} : null); } else { for (int i = nodeToRemove.getChildCount() - 1; i >= 0; i--) { removeNodeFromParent(nodeToRemove.getChildAt(i), shouldDelete); @@ -301,7 +304,8 @@ public class NativeViewHierarchyOptimizer { mUIViewOperationQueue.enqueueManageChildren( parent.getReactTag(), null, - new ViewAtIndex[]{new ViewAtIndex(child.getReactTag(), index)}, + new ViewAtIndex[] {new ViewAtIndex(child.getReactTag(), index)}, + null, null); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java index d9791957b70..18ff42605f3 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java @@ -347,6 +347,7 @@ public class UIImplementation { int[] indicesToRemove = new int[numToMove + numToRemove]; int[] tagsToRemove = new int[indicesToRemove.length]; int[] tagsToDelete = new int[numToRemove]; + int[] indicesToDelete = new int[numToRemove]; if (numToMove > 0) { Assertions.assertNotNull(moveFrom); @@ -380,6 +381,7 @@ public class UIImplementation { indicesToRemove[numToMove + i] = indexToRemove; tagsToRemove[numToMove + i] = tagToRemove; tagsToDelete[i] = tagToRemove; + indicesToDelete[i] = indexToRemove; } } @@ -423,7 +425,8 @@ public class UIImplementation { indicesToRemove, tagsToRemove, viewsToAdd, - tagsToDelete); + tagsToDelete, + indicesToDelete); } for (int i = 0; i < tagsToDelete.length; i++) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java index 9b86f083e2c..4d84d7545c0 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java @@ -210,16 +210,19 @@ public class UIViewOperationQueue { private final @Nullable int[] mIndicesToRemove; private final @Nullable ViewAtIndex[] mViewsToAdd; private final @Nullable int[] mTagsToDelete; + private final @Nullable int[] mIndicesToDelete; public ManageChildrenOperation( int tag, @Nullable int[] indicesToRemove, @Nullable ViewAtIndex[] viewsToAdd, - @Nullable int[] tagsToDelete) { + @Nullable int[] tagsToDelete, + @Nullable int[] indicesToDelete) { super(tag); mIndicesToRemove = indicesToRemove; mViewsToAdd = viewsToAdd; mTagsToDelete = tagsToDelete; + mIndicesToDelete = indicesToDelete; } @Override @@ -228,7 +231,8 @@ public class UIViewOperationQueue { mTag, mIndicesToRemove, mViewsToAdd, - mTagsToDelete); + mTagsToDelete, + mIndicesToDelete); } } @@ -778,9 +782,10 @@ public class UIViewOperationQueue { int reactTag, @Nullable int[] indicesToRemove, @Nullable ViewAtIndex[] viewsToAdd, - @Nullable int[] tagsToDelete) { + @Nullable int[] tagsToDelete, + @Nullable int[] indicesToDelete) { mOperations.add( - new ManageChildrenOperation(reactTag, indicesToRemove, viewsToAdd, tagsToDelete)); + new ManageChildrenOperation(reactTag, indicesToRemove, viewsToAdd, tagsToDelete, indicesToDelete)); } public void enqueueSetChildren(