From a715c5eba7ed7a7d8da22ba833ff135392255ba5 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Wed, 20 Jan 2021 17:43:51 -0800 Subject: [PATCH] Addressing leaked Views in Fabric MountingManager Summary: MountingManager keeps a map of tags to Views, and attempts to clean it up by (1) deleting tags when they're explicitly deleted, (2) recursively deleting all Views when the View hierarchy is torn down. However, there appear to be.... substantial gaps here. In tests, when navigating between screen X -> screen Y -> back to X (triggering a StopSurface), each "StopSurface" resulted in 200-600 Views being leaked (!). What is causing these leaks? Well, for one, the "dropView" mechanism isn't perfect, so it might be missed Views. Second, Views don't always guarantee that `reactTag == getId()`, so that could result in leaks. Third, View preallocation on Android complicates things: Views can be preallocated and then never even inserted into the View hierarchy, so DELETE mutations could never be issued. Fourth, StopSurface is also complicated on Android (largely because of View preallocatioAndroid (largely because of View preallocation). So, I introduce a new mechanism: keep a list of all tags for a surface, and remove all tags for a surface when the surface is torn down. This should be fool-proof: it handles preallocation and normal creation; it can handle deletes; and we're guaranteed that tags cannot be added after a surface is stopped. Is this overly complicating things? Well, hopefully we can simplify all of this in the longterm. But until we get rid of View Preallocation, it seems like we need this mechanism - and View Preallocation might be around for a while, or forever. Other thoughts: it's possible that using other data-structures could be more efficient, but I'm guessing the perf implications here are marginal (compared to the insane amount of memory leaks we're fixing). It could also simplify things to have a SurfaceMountingManager interface that implies all actions happen on a specific surface, including teardown. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D25985409 fbshipit-source-id: f55b533770b1630c6c2a9b7a694d953aa3324428 --- .../fabric/mounting/MountingManager.java | 31 +++++++++++++++++-- .../mountitems/IntBufferBatchMountItem.java | 3 +- .../mountitems/PreAllocateViewMountItem.java | 2 +- 3 files changed, 31 insertions(+), 5 deletions(-) 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 494e3c1f46b971..a22d94046d512a 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 @@ -42,6 +42,8 @@ import com.facebook.react.uimanager.ViewManager; import com.facebook.react.uimanager.ViewManagerRegistry; import com.facebook.yoga.YogaMeasureMode; +import java.util.LinkedHashMap; +import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; /** @@ -52,13 +54,15 @@ public class MountingManager { public static final String TAG = MountingManager.class.getSimpleName(); private static final boolean SHOW_CHANGED_VIEW_HIERARCHIES = ReactBuildConfig.DEBUG && false; - @NonNull private final ConcurrentHashMap mTagToViewState; + @NonNull private final ConcurrentHashMap mTagToViewState; // any thread + @NonNull private final LinkedHashMap> mRootTagToTags; // UI thread only @NonNull private final JSResponderHandler mJSResponderHandler = new JSResponderHandler(); @NonNull private final ViewManagerRegistry mViewManagerRegistry; @NonNull private final RootViewManager mRootViewManager = new RootViewManager(); public MountingManager(@NonNull ViewManagerRegistry viewManagerRegistry) { mTagToViewState = new ConcurrentHashMap<>(); + mRootTagToTags = new LinkedHashMap<>(); mViewManagerRegistry = viewManagerRegistry; } @@ -120,6 +124,7 @@ public void run() { + "explicitly overwrite the id field to View.NO_ID before calling addRootView."); } rootView.setId(reactRootTag); + mRootTagToTags.put(reactRootTag, new TreeSet()); } }); } @@ -131,6 +136,17 @@ public void deleteRootView(int reactRootTag) { if (rootViewState != null && rootViewState.mView != null) { dropView(rootViewState.mView, true); } + + // Iterate through all tags of reactRootTag, delete all retained Views associated with tags + // Tags that could be left-over, even after `dropView` is called: PreAllocated Views that were + // never properly deleted, for example. There might be other causes of leaks as well. + // This doesn't remove Views from the View Hierarchy, it just ensures that we don't leak + // memory by holding onto native Views. + TreeSet tags = mRootTagToTags.get(reactRootTag); + mRootTagToTags.remove(reactRootTag); + for (int tag : tags) { + mTagToViewState.remove(tag); + } } /** Releases all references to given native View. */ @@ -512,6 +528,7 @@ public void run() { public void createView( @NonNull ThemedReactContext themedReactContext, @NonNull String componentName, + int rootTag, int reactTag, @Nullable ReadableMap props, @Nullable StateWrapper stateWrapper, @@ -542,6 +559,7 @@ public void createView( viewState.mCurrentState = (stateWrapper != null ? stateWrapper.getState() : null); mTagToViewState.put(reactTag, viewState); + mRootTagToTags.get(rootTag).add(reactTag); } @UiThread @@ -622,7 +640,7 @@ public void updatePadding(int reactTag, int left, int top, int right, int bottom } @UiThread - public void deleteView(int reactTag) { + public void deleteView(int rootTag, int reactTag) { UiThreadUtil.assertOnUiThread(); ViewState viewState = getNullableViewState(reactTag); @@ -643,6 +661,7 @@ public void deleteView(int reactTag) { // Additionally, as documented in `dropView`, we cannot always trust a // view's children to be up-to-date. mTagToViewState.remove(reactTag); + mRootTagToTags.get(rootTag).remove(reactTag); // For non-root views we notify viewmanager with {@link ViewManager#onDropInstance} ViewManager viewManager = viewState.mViewManager; @@ -675,6 +694,7 @@ public void updateState(final int reactTag, @Nullable StateWrapper stateWrapper) public void preallocateView( @NonNull ThemedReactContext reactContext, String componentName, + int rootTag, int reactTag, @Nullable ReadableMap props, @Nullable StateWrapper stateWrapper, @@ -685,7 +705,12 @@ public void preallocateView( "View for component " + componentName + " with tag " + reactTag + " already exists."); } - createView(reactContext, componentName, reactTag, props, stateWrapper, isLayoutable); + // Views can be preallocated before the surface is started + if (mRootTagToTags.get(rootTag) == null) { + mRootTagToTags.put(rootTag, new TreeSet()); + } + + createView(reactContext, componentName, rootTag, reactTag, props, stateWrapper, isLayoutable); } @UiThread diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java index 27fa3783dd0c22..7aa0d46a2d0b1d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java @@ -140,12 +140,13 @@ public void execute(@NonNull MountingManager mountingManager) { mountingManager.createView( mContext, componentName, + mRootTag, mIntBuffer[i++], castToProps(mObjBuffer[j++]), castToState(mObjBuffer[j++]), mIntBuffer[i++] == 1); } else if (type == INSTRUCTION_DELETE) { - mountingManager.deleteView(mIntBuffer[i++]); + mountingManager.deleteView(mRootTag, mIntBuffer[i++]); } else if (type == INSTRUCTION_INSERT) { int tag = mIntBuffer[i++]; int parentTag = mIntBuffer[i++]; diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java index 0e3c7140eafbf2..873d67714cb8fb 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java @@ -64,7 +64,7 @@ public void execute(@NonNull MountingManager mountingManager) { + mRootTag); } mountingManager.preallocateView( - mContext, mComponent, mReactTag, mProps, mStateWrapper, mIsLayoutable); + mContext, mComponent, mRootTag, mReactTag, mProps, mStateWrapper, mIsLayoutable); } @Override