Fix activity leak bug

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: I7dfebac90aa43f89433f49e0ed60b3ff57bcc497

2 files changed, 50 insertions, 22 deletions

diff --git a/core/java/android/animation/AnimationHandler.java b/core/java/android/animation/AnimationHandler.java
index dcabf57cb8a9..df8a50a1fa5c 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<Object> mAnimatorRequestors = new ArraySet<>();
+    private final ArrayList<WeakReference<Object>> 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<Object> weakRef = null;
+                for (int i = mAnimatorRequestors.size() - 1; i >= 0; --i) {
+                    WeakReference<Object> 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<Object> 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 7e8ebd7fe076..e2447e93ff4b 100644
--- a/core/java/android/view/ViewRootImpl.java
+++ b/core/java/android/view/ViewRootImpl.java
@@ -1408,7 +1408,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");
+                }
             }
         }
     }
@@ -1759,7 +1763,12 @@ public final class ViewRootImpl implements ViewParent,
                 mAppVisibilityChanged = true;
                 scheduleTraversals();
             }
-            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");
+            }
         }
     }