From ad65b2a9e1c1e3d1cab9090b314b75ec0eedd4af Mon Sep 17 00:00:00 2001 From: Denis Koroskin Date: Wed, 16 Dec 2015 10:05:43 -0800 Subject: [PATCH] Remove referenced to dropped views Summary: There is currently a bug where we never release any Views that no longer display, still storing hard references in NativeViewHierarchyManager. This diff is fixing this bug, and here is how: a) there is already logic in place to drop FlatShadowNodes (UIImplementation.removeShadowNode). b) there is already logic in place to drop Views (NativeViewHierarchyManager.dropView(int reactTag) - used to private but needs to be protected so I can call it) c) (the missing part) when we are about to drop a FlatShadowNode, check if it mount to a View (i.e. there is a View associated with that node), put it into a ArrayList. When we finished updates to a nodes hierarchy (which happens in non-UI thread), collect ids from those nodes and enqueue a UIOperation that will drop all the Views. We can either forward nodes as FlatShadowNode[], or only ids as int[]. Both should be fine, but as a rule of thumb we don't touch shadow node hierarchy from UI thread (as we don't touch Views from non-UI thread) so passing int[] is what this code is doing. Reviewed By: ahmedre Differential Revision: D2757310 --- .../flat/FlatNativeViewHierarchyManager.java | 6 ++++++ .../react/flat/FlatUIImplementation.java | 6 +++++- .../react/flat/FlatUIViewOperationQueue.java | 18 ++++++++++++++++++ .../com/facebook/react/flat/StateBuilder.java | 10 ++++++++++ 4 files changed, 39 insertions(+), 1 deletion(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatNativeViewHierarchyManager.java b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatNativeViewHierarchyManager.java index a8f44a960..6a321f9af 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatNativeViewHierarchyManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatNativeViewHierarchyManager.java @@ -122,6 +122,12 @@ import com.facebook.react.uimanager.ViewManagerRegistry; resolveView(reactTag).setPadding(paddingLeft, paddingTop, paddingRight, paddingBottom); } + /* package */ void dropViews(int[] viewsToDrop) { + for (int viewToDrop : viewsToDrop) { + dropView(resolveView(viewToDrop)); + } + } + /* package */ void detachAllChildrenFromViews(int[] viewsToDetachAllChildrenFrom) { for (int viewTag : viewsToDetachAllChildrenFrom) { View view = resolveView(viewTag); diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIImplementation.java b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIImplementation.java index adf9b5e30..716799ab0 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIImplementation.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIImplementation.java @@ -149,9 +149,13 @@ public class FlatUIImplementation extends UIImplementation { "Invariant failure, needs sorting! prevIndex: " + prevIndex + " index: " + index); } - ReactShadowNode child = parentNode.removeChildAt(index); + FlatShadowNode child = (FlatShadowNode) parentNode.removeChildAt(index); prevIndex = index; + if (child.mountsToView()) { + mStateBuilder.dropView(child); + } + removeShadowNode(child); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIViewOperationQueue.java b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIViewOperationQueue.java index 0c829d327..de18b70bd 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIViewOperationQueue.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIViewOperationQueue.java @@ -128,6 +128,20 @@ import com.facebook.react.uimanager.UIViewOperationQueue; } } + private final class DropViews implements UIOperation { + + private final int[] mViewsToDrop; + + private DropViews(int[] viewsToDrop) { + mViewsToDrop = viewsToDrop; + } + + @Override + public void execute() { + mNativeViewHierarchyManager.dropViews(mViewsToDrop); + } + } + public final class DetachAllChildrenFromViews implements UIViewOperationQueue.UIOperation { private @Nullable int[] mViewsToDetachAllChildrenFrom; @@ -181,6 +195,10 @@ import com.facebook.react.uimanager.UIViewOperationQueue; new SetPadding(reactTag, paddingLeft, paddingTop, paddingRight, paddingBottom)); } + public void enqueueDropViews(int[] viewsToDrop) { + enqueueUIOperation(new DropViews(viewsToDrop)); + } + public DetachAllChildrenFromViews enqueueDetachAllChildrenFromViews() { DetachAllChildrenFromViews op = new DetachAllChildrenFromViews(); enqueueUIOperation(op); diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java b/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java index 54b536f9f..ccce7b24a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java @@ -40,6 +40,7 @@ import com.facebook.react.uimanager.CatalystStylesDiffMap; private final ArrayList mViewsToDetachAllChildrenFrom = new ArrayList<>(); private final ArrayList mViewsToDetach = new ArrayList<>(); private final ArrayList mViewsToUpdateBounds = new ArrayList<>(); + private final ArrayList mViewsToDrop = new ArrayList<>(); private @Nullable FlatUIViewOperationQueue.DetachAllChildrenFromViews mDetachAllChildrenFromViews; @@ -78,6 +79,11 @@ import com.facebook.react.uimanager.CatalystStylesDiffMap; updateViewBounds(mViewsToUpdateBounds.get(i)); } mViewsToUpdateBounds.clear(); + + if (!mViewsToDrop.isEmpty()) { + mOperationsQueue.enqueueDropViews(collectViewTags(mViewsToDrop)); + mViewsToDrop.clear(); + } } /** @@ -107,6 +113,10 @@ import com.facebook.react.uimanager.CatalystStylesDiffMap; node.signalBackingViewIsCreated(); } + /* package */ void dropView(FlatShadowNode node) { + mViewsToDrop.add(node); + } + private void addNodeRegion(NodeRegion nodeRegion) { mNodeRegions.add(nodeRegion); }