From 313d7d79d4257efcce5f3a555e335a74ce56df53 Mon Sep 17 00:00:00 2001 From: Andrew Knapp Date: Thu, 13 Mar 2025 04:17:17 -0700 Subject: [PATCH] Fix 49958: Close a view leak due to lossy onAnimationEnd callback (#49959) Summary: Android's onAnimationEnd callback is lossy and ocasionally just does not fire. However the LayoutAnimationController maintains a sparse array of animations (with Strong View refs) that is only cleaned when the onAnimationEnd callback is invoked. This results in a leak of Android View objects over time. To avoid this, the Strong View refs are migrated to WeakReference's and the associated sparse array is cleaned of any invalid layout animations in response to the reset() call. This closes two leaks: 1. Unbound growth in LayoutAnimationController::mLayoutHandlers 2. Pinning View objects into memory as the sole remaining GC root ## Changelog: 1. Made OpacityAnimation and PositionAndSizeAnimation classes hold weak refs to views only 2. Added a method to LayoutHandlingAnimation to surface if their view ref is gone 3. Added cleanup for Animation with bad view refs Pick one each for the category and type tags: [ANDROID] [Fixed]- Fixes memory leak Pull Request resolved: https://github.com/facebook/react-native/pull/49959 Test Plan: * Primarily code inspection and regression given the intermittent nature of Android's failure to execute the callback. Reviewed By: alanleedev Differential Revision: D71037262 Pulled By: javache fbshipit-source-id: 1fa4eaa2ca839f347a55cb37e2648db972748586 --- .../LayoutAnimationController.java | 14 ++++- .../LayoutHandlingAnimation.kt | 9 ++++ .../layoutanimation/OpacityAnimation.kt | 28 +++++----- .../PositionAndSizeAnimation.kt | 53 ++++++++++--------- 4 files changed, 66 insertions(+), 38 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/LayoutAnimationController.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/LayoutAnimationController.java index 612f0b015cc..be0b35efd2a 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/LayoutAnimationController.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/LayoutAnimationController.java @@ -87,6 +87,12 @@ public class LayoutAnimationController { mCompletionRunnable = null; mShouldAnimateLayout = false; mMaxAnimationDuration = -1; + for (int i = mLayoutHandlers.size() - 1; i >= 0; i--) { + LayoutHandlingAnimation animation = mLayoutHandlers.valueAt(i); + if (!animation.isValid()) { + mLayoutHandlers.removeAt(i); + } + } } public boolean shouldAnimateLayout(View viewToAnimate) { @@ -121,8 +127,12 @@ public class LayoutAnimationController { // the existing animation would still animate to the old layout. LayoutHandlingAnimation existingAnimation = mLayoutHandlers.get(reactTag); if (existingAnimation != null) { - existingAnimation.onLayoutUpdate(x, y, width, height); - return; + if (!existingAnimation.isValid()) { + mLayoutHandlers.remove(reactTag); + } else { + existingAnimation.onLayoutUpdate(x, y, width, height); + return; + } } // Determine which animation to use : if view is initially invisible, use create animation, diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/LayoutHandlingAnimation.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/LayoutHandlingAnimation.kt index b75c8d06603..e847f44edca 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/LayoutHandlingAnimation.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/LayoutHandlingAnimation.kt @@ -23,4 +23,13 @@ internal interface LayoutHandlingAnimation { * @param height the new height value for the view */ fun onLayoutUpdate(x: Int, y: Int, width: Int, height: Int) + + /** + * Returns whether the animation is valid and should be used. Because layout animations generally + * hold {@link java.lang.ref.WeakReference} objects, it's possible that the + * view has been garbage collected. In this case, the animation should not be used. + * + * @return true if the animation is valid and can be used, false otherwise + */ + fun isValid(): Boolean } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/OpacityAnimation.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/OpacityAnimation.kt index b87021cf44a..65ac4882de7 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/OpacityAnimation.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/OpacityAnimation.kt @@ -14,17 +14,16 @@ import com.facebook.react.common.annotations.VisibleForTesting import com.facebook.react.common.annotations.internal.LegacyArchitecture import com.facebook.react.common.annotations.internal.LegacyArchitectureLogLevel import com.facebook.react.common.annotations.internal.LegacyArchitectureLogger +import java.lang.ref.WeakReference /** * Animation responsible for updating opacity of a view. It should ideally use hardware texture to * optimize rendering performances. */ @LegacyArchitecture -internal class OpacityAnimation( - private val view: View, - private val startOpacity: Float, - endOpacity: Float -) : Animation() { +internal class OpacityAnimation(view: View, private val startOpacity: Float, endOpacity: Float) : + Animation() { + private val viewRef = WeakReference(view) private val deltaOpacity = endOpacity - startOpacity init { @@ -33,19 +32,24 @@ internal class OpacityAnimation( "OpacityAnimation", LegacyArchitectureLogLevel.WARNING) } - class OpacityAnimationListener(private val view: View) : Animation.AnimationListener { + class OpacityAnimationListener(view: View) : Animation.AnimationListener { + private val viewRef = WeakReference(view) private var layerTypeChanged = false override fun onAnimationStart(animation: Animation) { - if (view.hasOverlappingRendering() && view.layerType == View.LAYER_TYPE_NONE) { - layerTypeChanged = true - view.setLayerType(View.LAYER_TYPE_HARDWARE, null) + viewRef.get()?.let { view -> + if (view.hasOverlappingRendering() && view.layerType == View.LAYER_TYPE_NONE) { + layerTypeChanged = true + view.setLayerType(View.LAYER_TYPE_HARDWARE, null) + } } } override fun onAnimationEnd(animation: Animation) { - if (layerTypeChanged) { - view.setLayerType(View.LAYER_TYPE_NONE, null) + viewRef.get()?.let { view -> + if (layerTypeChanged) { + view.setLayerType(View.LAYER_TYPE_NONE, null) + } } } @@ -56,7 +60,7 @@ internal class OpacityAnimation( @VisibleForTesting public override fun applyTransformation(interpolatedTime: Float, t: Transformation) { - view.alpha = startOpacity + deltaOpacity * interpolatedTime + viewRef.get()?.let { view -> view.alpha = startOpacity + deltaOpacity * interpolatedTime } } override fun willChangeBounds(): Boolean { diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/PositionAndSizeAnimation.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/PositionAndSizeAnimation.kt index 98e656386cd..d63d5491b00 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/PositionAndSizeAnimation.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/layoutanimation/PositionAndSizeAnimation.kt @@ -12,6 +12,7 @@ import android.view.animation.Animation import android.view.animation.Transformation import com.facebook.react.common.annotations.internal.LegacyArchitecture import com.facebook.react.common.annotations.internal.LegacyArchitectureLogger +import java.lang.ref.WeakReference /** * Animation responsible for updating size and position of a view. We can't use scaling as view @@ -20,13 +21,9 @@ import com.facebook.react.common.annotations.internal.LegacyArchitectureLogger * ScaleAnimation and TranslateAnimation. */ @LegacyArchitecture -internal class PositionAndSizeAnimation( - private val view: View, - x: Int, - y: Int, - width: Int, - height: Int -) : Animation(), LayoutHandlingAnimation { +internal class PositionAndSizeAnimation(view: View, x: Int, y: Int, width: Int, height: Int) : + Animation(), LayoutHandlingAnimation { + private val viewRef = WeakReference(view) private var startX = 0f private var startY = 0f private var deltaX = 0f @@ -41,15 +38,17 @@ internal class PositionAndSizeAnimation( } override fun applyTransformation(interpolatedTime: Float, t: Transformation) { - val newX = startX + deltaX * interpolatedTime - val newY = startY + deltaY * interpolatedTime - val newWidth = startWidth + deltaWidth * interpolatedTime - val newHeight = startHeight + deltaHeight * interpolatedTime - view.layout( - Math.round(newX), - Math.round(newY), - Math.round(newX + newWidth), - Math.round(newY + newHeight)) + viewRef.get()?.let { view -> + val newX = startX + deltaX * interpolatedTime + val newY = startY + deltaY * interpolatedTime + val newWidth = startWidth + deltaWidth * interpolatedTime + val newHeight = startHeight + deltaHeight * interpolatedTime + view.layout( + Math.round(newX), + Math.round(newY), + Math.round(newX + newWidth), + Math.round(newY + newHeight)) + } } override fun onLayoutUpdate(x: Int, y: Int, width: Int, height: Int) { @@ -58,20 +57,26 @@ internal class PositionAndSizeAnimation( calculateAnimation(x, y, width, height) } + override fun isValid(): Boolean { + return viewRef.get() != null + } + override fun willChangeBounds(): Boolean { return true } private fun calculateAnimation(x: Int, y: Int, width: Int, height: Int) { - startX = view.x - view.translationX - startY = view.y - view.translationY - startWidth = view.width - startHeight = view.height + viewRef.get()?.let { view -> + startX = view.x - view.translationX + startY = view.y - view.translationY + startWidth = view.width + startHeight = view.height - deltaX = x - startX - deltaY = y - startY - deltaWidth = width - startWidth - deltaHeight = height - startHeight + deltaX = x - startX + deltaY = y - startY + deltaWidth = width - startWidth + deltaHeight = height - startHeight + } } private companion object {