From 93f2744c95fb5096b326afc149f390030e3c8810 Mon Sep 17 00:00:00 2001 From: Chet Haase Date: Sat, 12 Nov 2022 00:46:34 +0000 Subject: [PATCH] Fix activity leak bug This is a cherry-pick from main branch: ID: I7dfebac90aa43f89433f49e0ed60b3ff57bcc497 It is possible for ViewRootImpl.doDie() to be called prior to one of the other calls which tells the system that the window is going to the foreground. This race condition can cause an activity leak since the ViewRootImpl in question is stored in a collection of "requestors" which determine when to pause/resume animators for background/foreground apps. The fix is two-fold: - Only cause an item to be added to the "requestor" list in AnimationHandler when doDie() has not yet been called (marked by the mReoved" flag) - Store the requestor objects as WeakReferences, rather than the objects themselves. Thus if the requestor list is the only referent to the object, it can be collected anyway, removing the leak potential. Bug: 258616235, 258616235 Test: AnimatorLeakTest passes Change-Id: Icdd6023f6530e3cc4b474f33dc34f6eb7400788e Merged-in: I7dfebac90aa43f89433f49e0ed60b3ff57bcc497 --- .../android/animation/AnimationHandler.java | 59 ++++++++++++------- core/java/android/view/ViewRootImpl.java | 13 +++- 2 files changed, 50 insertions(+), 22 deletions(-) diff --git a/core/java/android/animation/AnimationHandler.java b/core/java/android/animation/AnimationHandler.java index dcabf57cb8a93..df8a50a1fa5c1 100644 --- a/core/java/android/animation/AnimationHandler.java +++ b/core/java/android/animation/AnimationHandler.java @@ -19,10 +19,10 @@ package android.animation; import android.os.SystemClock; import android.os.SystemProperties; import android.util.ArrayMap; -import android.util.ArraySet; import android.util.Log; import android.view.Choreographer; +import java.lang.ref.WeakReference; import java.util.ArrayList; /** @@ -78,7 +78,7 @@ public class AnimationHandler { * store visible (foreground) requestors; if the set size reaches zero, there are no * objects in the foreground and it is time to pause animators. */ - private final ArraySet mAnimatorRequestors = new ArraySet<>(); + private final ArrayList> mAnimatorRequestors = new ArrayList<>(); private final Choreographer.FrameCallback mFrameCallback = new Choreographer.FrameCallback() { @Override @@ -141,19 +141,9 @@ public class AnimationHandler { * tracking obsolete+enabled requestors. */ public static void removeRequestor(Object requestor) { - getInstance().removeRequestorImpl(requestor); - } - - private void removeRequestorImpl(Object requestor) { - // Also request disablement, in case that requestor was the sole object keeping - // animators un-paused - requestAnimatorsEnabled(false, requestor); - mAnimatorRequestors.remove(requestor); + getInstance().requestAnimatorsEnabledImpl(false, requestor); if (LOCAL_LOGV) { - Log.v(TAG, "removeRequestorImpl for " + requestor); - for (int i = 0; i < mAnimatorRequestors.size(); ++i) { - Log.v(TAG, "animatorRequesters " + i + " = " + mAnimatorRequestors.valueAt(i)); - } + Log.v(TAG, "removeRequestor for " + requestor); } } @@ -173,10 +163,36 @@ public class AnimationHandler { private void requestAnimatorsEnabledImpl(boolean enable, Object requestor) { boolean wasEmpty = mAnimatorRequestors.isEmpty(); setAnimatorPausingEnabled(isPauseBgAnimationsEnabledInSystemProperties()); - if (enable) { - mAnimatorRequestors.add(requestor); - } else { - mAnimatorRequestors.remove(requestor); + synchronized (mAnimatorRequestors) { + // Only store WeakRef objects to avoid leaks + if (enable) { + // First, check whether such a reference is already on the list + WeakReference weakRef = null; + for (int i = mAnimatorRequestors.size() - 1; i >= 0; --i) { + WeakReference ref = mAnimatorRequestors.get(i); + Object referent = ref.get(); + if (referent == requestor) { + weakRef = ref; + } else if (referent == null) { + // Remove any reference that has been cleared + mAnimatorRequestors.remove(i); + } + } + if (weakRef == null) { + weakRef = new WeakReference<>(requestor); + mAnimatorRequestors.add(weakRef); + } + } else { + for (int i = mAnimatorRequestors.size() - 1; i >= 0; --i) { + WeakReference ref = mAnimatorRequestors.get(i); + Object referent = ref.get(); + if (referent == requestor || referent == null) { + // remove requested item or item that has been cleared + mAnimatorRequestors.remove(i); + } + } + // If a reference to the requestor wasn't in the list, nothing to remove + } } if (!sAnimatorPausingEnabled) { // Resume any animators that have been paused in the meantime, otherwise noop @@ -198,9 +214,12 @@ public class AnimationHandler { } } if (LOCAL_LOGV) { - Log.v(TAG, enable ? "enable" : "disable" + " animators for " + requestor); + Log.v(TAG, (enable ? "enable" : "disable") + " animators for " + requestor + + " with pauseDelay of " + Animator.getBackgroundPauseDelay()); for (int i = 0; i < mAnimatorRequestors.size(); ++i) { - Log.v(TAG, "animatorRequesters " + i + " = " + mAnimatorRequestors.valueAt(i)); + Log.v(TAG, "animatorRequestors " + i + " = " + + mAnimatorRequestors.get(i) + " with referent " + + mAnimatorRequestors.get(i).get()); } } } diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java index 8f4a836b68614..b4698dee6019a 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -1409,7 +1409,11 @@ public final class ViewRootImpl implements ViewParent, mFirstPostImeInputStage = earlyPostImeStage; mPendingInputEventQueueLengthCounterName = "aq:pending:" + counterSuffix; - AnimationHandler.requestAnimatorsEnabled(mAppVisible, this); + if (!mRemoved || !mAppVisible) { + AnimationHandler.requestAnimatorsEnabled(mAppVisible, this); + } else if (LOCAL_LOGV) { + Log.v(mTag, "setView() enabling visibility when removed"); + } } } } @@ -1747,7 +1751,12 @@ public final class ViewRootImpl implements ViewParent, if (!mAppVisible) { WindowManagerGlobal.trimForeground(); } - AnimationHandler.requestAnimatorsEnabled(mAppVisible, this); + // Only enable if the window is not already removed (via earlier call to doDie()) + if (!mRemoved || !mAppVisible) { + AnimationHandler.requestAnimatorsEnabled(mAppVisible, this); + } else if (LOCAL_LOGV) { + Log.v(mTag, "handleAppVisibility() enabling visibility when removed"); + } } }