From c16e993bb8417da9a4ff9e082a77a21688d75bac Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Tue, 20 Jun 2023 14:20:29 -0700 Subject: [PATCH] Fix SurfaceMountingManager leaking views from stopped surfaces (#37964) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/37964 When a Surface is stopped, we don't immediately destroy the SurfaceMountingManager but instead just tear down its internal state. This allows for better error handling (eg did this react tag ever exist, or is this non-existing tag). The way we construct the set of tags post-deletion is flawed though: `mTagToViewState.keySet()` does not create a new Set with all the tags used, but instead uses the underlying HashMap to iterate over the keys as needed. This effectively keeps all the Views inside that deleted surface alive. Changelog: [Android][Fixed] Surfaces in the new architecture no longer leak views once stopped Reviewed By: sammy-SC, rshest Differential Revision: D46840717 fbshipit-source-id: fad145e4dd21b216d1e64f5dc79900434cff1785 --- .../react/config/ReactFeatureFlags.java | 3 + .../mounting/SurfaceMountingManager.java | 55 ++++++++++++------- 2 files changed, 39 insertions(+), 19 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index 6eb5b53b12f..fce0e58f9c6 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -159,4 +159,7 @@ public class ReactFeatureFlags { /** Report mount operations from the host platform to notify mount hooks. */ public static boolean enableMountHooks = false; + + /** Fixes a leak in SurfaceMountingManager.mTagSetForStoppedSurface */ + public static boolean fixStoppedSurfaceTagSetLeak = true; } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java index f3fb5e0a002..be7bf13bbf6 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java @@ -16,6 +16,7 @@ import android.view.ViewParent; import androidx.annotation.AnyThread; import androidx.annotation.NonNull; import androidx.annotation.UiThread; +import androidx.collection.SparseArrayCompat; import com.facebook.common.logging.FLog; import com.facebook.infer.annotation.Assertions; import com.facebook.infer.annotation.ThreadConfined; @@ -52,6 +53,7 @@ import com.facebook.react.views.view.ReactMapBufferViewManager; import com.facebook.react.views.view.ReactViewManagerWrapper; import java.util.HashSet; import java.util.LinkedList; +import java.util.Map; import java.util.Queue; import java.util.Set; import java.util.Stack; @@ -91,7 +93,8 @@ public class SurfaceMountingManager { private RemoveDeleteTreeUIFrameCallback mRemoveDeleteTreeUIFrameCallback; // This is null *until* StopSurface is called. - private Set mTagSetForStoppedSurface; + private Set mTagSetForStoppedSurfaceLegacy; + private SparseArrayCompat mTagSetForStoppedSurface; private final int mSurfaceId; @@ -166,7 +169,10 @@ public class SurfaceMountingManager { // If Surface stopped, check if tag *was* associated with this Surface, even though it's been // deleted. This helps distinguish between scenarios where an invalid tag is referenced, vs // race conditions where an imperative method is called on a tag during/just after StopSurface. - if (mTagSetForStoppedSurface != null && mTagSetForStoppedSurface.contains(tag)) { + if (mTagSetForStoppedSurface != null && mTagSetForStoppedSurface.containsKey(tag)) { + return true; + } + if (mTagSetForStoppedSurfaceLegacy != null && mTagSetForStoppedSurfaceLegacy.contains(tag)) { return true; } if (mTagToViewState == null) { @@ -287,27 +293,38 @@ public class SurfaceMountingManager { } Runnable runnable = - new Runnable() { - @Override - public void run() { - // We must call `onDropViewInstance` on all remaining Views + () -> { + if (ReactFeatureFlags.fixStoppedSurfaceTagSetLeak) { + mTagSetForStoppedSurface = new SparseArrayCompat<>(); + for (Map.Entry entry : mTagToViewState.entrySet()) { + // Using this as a placeholder value in the map. We're using SparseArrayCompat + // since it can efficiently represent the list of pending tags + mTagSetForStoppedSurface.put(entry.getKey(), this); + + // We must call `onDropViewInstance` on all remaining Views + onViewStateDeleted(entry.getValue()); + } + } else { for (ViewState viewState : mTagToViewState.values()) { + // We must call `onDropViewInstance` on all remaining Views onViewStateDeleted(viewState); } - - // Evict all views from cache and memory - mTagSetForStoppedSurface = mTagToViewState.keySet(); - mTagToViewState = null; - mJSResponderHandler = null; - mRootViewManager = null; - mMountItemExecutor = null; - mOnViewAttachItems.clear(); - - if (ReactFeatureFlags.enableViewRecycling) { - mViewManagerRegistry.onSurfaceStopped(mSurfaceId); - } - FLog.e(TAG, "Surface [" + mSurfaceId + "] was stopped on SurfaceMountingManager."); + mTagSetForStoppedSurfaceLegacy = mTagToViewState.keySet(); } + + // Evict all views from cache and memory + // TODO: clear instead of nulling out to simplify null-safety in this class + mTagToViewState = null; + mJSResponderHandler = null; + mRootViewManager = null; + mMountItemExecutor = null; + mThemedReactContext = null; + mOnViewAttachItems.clear(); + + if (ReactFeatureFlags.enableViewRecycling) { + mViewManagerRegistry.onSurfaceStopped(mSurfaceId); + } + FLog.e(TAG, "Surface [" + mSurfaceId + "] was stopped on SurfaceMountingManager."); }; if (UiThreadUtil.isOnUiThread()) {