Fix leak in Nodes due to removeClippedSubviews

Summary:
The removeClippedSubviews optimization often detaches views while
maintaining strong references to them (so they can be attached again later
on). However, when removing the parent view, any detached views end up not
being cleaned up or removed, thus leaking memory. This fixes this by
explicitly dropping detached views when the parent is removed.

Reviewed By: astreet

Differential Revision: D3337513
This commit is contained in:
Ahmed El-Helw 2016-05-24 14:48:23 -07:00
parent c62840c7a8
commit efb65e5665
2 changed files with 49 additions and 0 deletions

View File

@ -11,6 +11,8 @@ package com.facebook.react.flat;
import javax.annotation.Nullable; import javax.annotation.Nullable;
import java.util.Collection;
import android.view.View; import android.view.View;
import android.view.View.MeasureSpec; import android.view.View.MeasureSpec;
import android.view.ViewGroup; import android.view.ViewGroup;
@ -134,6 +136,30 @@ import com.facebook.react.uimanager.ViewManagerRegistry;
} }
} }
@Override
protected void dropView(View view) {
super.dropView(view);
// As a result of removeClippedSubviews, some views have strong references but are not attached
// to a parent. consequently, when the parent gets removed, these Views don't get cleaned up,
// because they aren't children (they also aren't removed from mTagsToViews, thus causing a
// leak). To solve this, we ask for said detached views and explicitly drop them.
if (view instanceof FlatViewGroup) {
FlatViewGroup flatViewGroup = (FlatViewGroup) view;
if (flatViewGroup.getRemoveClippedSubviews()) {
Collection<FlatViewGroup> detachedViews = flatViewGroup.getDetachedViews();
for (FlatViewGroup detachedChild : detachedViews) {
// we can do super here because removeClippedSubviews is currently not recursive. if/when
// we become recursive one day, this should call vanilla dropView to be recursive as well.
super.dropView(detachedChild);
// trigger onDetachedFromWindow - this is currently needed due to using attach/detach
// instead of add/remove. if we move to add/remove in the future, we can remove this.
flatViewGroup.removeDetachedView(detachedChild);
}
}
}
}
/* package */ void detachAllChildrenFromViews(int[] viewsToDetachAllChildrenFrom) { /* package */ void detachAllChildrenFromViews(int[] viewsToDetachAllChildrenFrom) {
for (int viewTag : viewsToDetachAllChildrenFrom) { for (int viewTag : viewsToDetachAllChildrenFrom) {
View view = resolveView(viewTag); View view = resolveView(viewTag);

View File

@ -13,6 +13,7 @@ import javax.annotation.Nullable;
import java.lang.ref.WeakReference; import java.lang.ref.WeakReference;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap; import java.util.HashMap;
import java.util.Map; import java.util.Map;
@ -407,6 +408,28 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper;
invalidate(); invalidate();
} }
/**
* Return a list of FlatViewGroups that are detached (due to being clipped) but that we have a
* strong reference to. This is used by the FlatNativeViewHierarchyManager to explicitly clean up
* those views when removing this parent.
*
* @return a Collection of FlatViewGroups to clean up
*/
Collection<FlatViewGroup> getDetachedViews() {
return mClippedSubviews.values();
}
/**
* Remove the detached view from the parent
* This is used during cleanup to trigger onDetachedFromWindow on any views that were in a
* temporary detached state due to them being clipped. This is called for cleanup of said views
* by FlatNativeViewHierarchyManager.
* @param flatViewGroup the detached FlatViewGroup to remove
*/
void removeDetachedView(FlatViewGroup flatViewGroup) {
removeDetachedView(flatViewGroup, false);
}
/* package */ void mountAttachDetachListeners(AttachDetachListener[] listeners) { /* package */ void mountAttachDetachListeners(AttachDetachListener[] listeners) {
if (mIsAttached) { if (mIsAttached) {
// Ordering of the following 2 statements is very important. While logically it makes sense to // Ordering of the following 2 statements is very important. While logically it makes sense to