From c199efbedad862609bc82a06d58f5b665eb6e324 Mon Sep 17 00:00:00 2001 From: Chet Haase Date: Thu, 8 Dec 2022 04:46:35 +0000 Subject: DO NOT MERGE Revert "Fix activity leak bug" Revert "Fix test cleanup issue in AnimatorLeakTest" Revert submission 20610032-cherrypick-AnimatorLeak fixes-4lh72bu61o Reason for revert: Startup regression found in post-submit Reverted Changes: Icdd6023f6:Fix activity leak bug Ieac851550:Fix test cleanup issue in AnimatorLeakTest This is a tentative revert to see whether it addresses the regression while I continue chasing the underlying problem Bug: 261518932 Test: presubmit Change-Id: Ica9a75b94c8c260cceb6719f8f6b95ba67d4e356 --- core/java/android/animation/AnimationHandler.java | 75 +++++++++++------------ core/java/android/view/ViewRootImpl.java | 13 +--- 2 files changed, 37 insertions(+), 51 deletions(-) diff --git a/core/java/android/animation/AnimationHandler.java b/core/java/android/animation/AnimationHandler.java index df8a50a1fa5c..19606c16cb70 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; /** @@ -40,7 +40,7 @@ import java.util.ArrayList; public class AnimationHandler { private static final String TAG = "AnimationHandler"; - private static final boolean LOCAL_LOGV = false; + private static final boolean LOCAL_LOGV = true; /** * Internal per-thread collections used to avoid set collisions as animations start and end @@ -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 ArrayList> mAnimatorRequestors = new ArrayList<>(); + private final ArraySet mAnimatorRequestors = new ArraySet<>(); private final Choreographer.FrameCallback mFrameCallback = new Choreographer.FrameCallback() { @Override @@ -141,9 +141,24 @@ public class AnimationHandler { * tracking obsolete+enabled requestors. */ public static void removeRequestor(Object requestor) { - getInstance().requestAnimatorsEnabledImpl(false, requestor); + getInstance().removeRequestorImpl(requestor); + } + + private void removeRequestorImpl(Object requestor) { + // Also request disablement, in case that requestor was the sole object keeping + // animators un-paused + long startTime = System.nanoTime(); + requestAnimatorsEnabled(false, requestor); + Log.d(TAG, "removeRequestorImpl called requestAnimatorsEnabled after " + + (System.nanoTime() - startTime)); + mAnimatorRequestors.remove(requestor); + Log.d(TAG, "removeRequestorImpl removed requestor after " + + (System.nanoTime() - startTime)); if (LOCAL_LOGV) { - Log.v(TAG, "removeRequestor for " + requestor); + Log.v(TAG, "removeRequestorImpl for " + requestor); + for (int i = 0; i < mAnimatorRequestors.size(); ++i) { + Log.v(TAG, "animatorRequesters " + i + " = " + mAnimatorRequestors.valueAt(i)); + } } } @@ -161,44 +176,25 @@ public class AnimationHandler { } private void requestAnimatorsEnabledImpl(boolean enable, Object requestor) { + long startTime = System.nanoTime(); boolean wasEmpty = mAnimatorRequestors.isEmpty(); setAnimatorPausingEnabled(isPauseBgAnimationsEnabledInSystemProperties()); - 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 - } + Log.d(TAG, "requestAnimatorsEnabledImpl called setAnimatorPausingEnabled after " + + (System.nanoTime() - startTime)); + if (enable) { + mAnimatorRequestors.add(requestor); + } else { + mAnimatorRequestors.remove(requestor); } + Log.d(TAG, "requestAnimatorsEnabledImpl added/removed after " + + (System.nanoTime() - startTime)); if (!sAnimatorPausingEnabled) { // Resume any animators that have been paused in the meantime, otherwise noop // Leave logic above so that if pausing gets re-enabled, the state of the requestors // list is valid resumeAnimators(); + Log.d(TAG, "requestAnimatorsEnabledImpl resumed, returning after " + + (System.nanoTime() - startTime)); return; } boolean isEmpty = mAnimatorRequestors.isEmpty(); @@ -213,13 +209,12 @@ public class AnimationHandler { Animator.getBackgroundPauseDelay()); } } + Log.d(TAG, "requestAnimatorsEnabledImpl post was/is check after " + + (System.nanoTime() - startTime)); if (LOCAL_LOGV) { - Log.v(TAG, (enable ? "enable" : "disable") + " animators for " + requestor - + " with pauseDelay of " + Animator.getBackgroundPauseDelay()); + Log.v(TAG, enable ? "enable" : "disable" + " animators for " + requestor); for (int i = 0; i < mAnimatorRequestors.size(); ++i) { - Log.v(TAG, "animatorRequestors " + i + " = " - + mAnimatorRequestors.get(i) + " with referent " - + mAnimatorRequestors.get(i).get()); + Log.v(TAG, "animatorRequesters " + i + " = " + mAnimatorRequestors.valueAt(i)); } } } diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java index b4698dee6019..8f4a836b6861 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -1409,11 +1409,7 @@ public final class ViewRootImpl implements ViewParent, mFirstPostImeInputStage = earlyPostImeStage; mPendingInputEventQueueLengthCounterName = "aq:pending:" + counterSuffix; - if (!mRemoved || !mAppVisible) { - AnimationHandler.requestAnimatorsEnabled(mAppVisible, this); - } else if (LOCAL_LOGV) { - Log.v(mTag, "setView() enabling visibility when removed"); - } + AnimationHandler.requestAnimatorsEnabled(mAppVisible, this); } } } @@ -1751,12 +1747,7 @@ public final class ViewRootImpl implements ViewParent, if (!mAppVisible) { WindowManagerGlobal.trimForeground(); } - // 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"); - } + AnimationHandler.requestAnimatorsEnabled(mAppVisible, this); } } -- cgit v1.2.3-59-g8ed1b