Do not trim tasks when updating activity visibilities

When updating visibility of all activities, the state of activity
may be changed to resumed, and it will also be added to recents
task. If the max amount of task is reached, the inactive tasks will
be trimmed. So it is possible that the index of visibility-update
loop becomes out of bounds.

Though it can also be solved by checking the index every time, that
seems to be a bit unintuitive and waste. And skip trimming should
also reduce the side effect of visibility-update to focus on its
major functionality.

Fixes: 154809437
Fixes: 142627724
Test: RecentTasksTest#testAddTasksInVisibilityUpdate_expectNoTrim

Change-Id: Iba66f2e6ce0beae3ef1187549318ef8779154a3b

6 files changed, 66 insertions, 37 deletions

diff --git a/services/core/java/com/android/server/wm/ActivityStack.java b/services/core/java/com/android/server/wm/ActivityStack.java
index 2ab03ce058b2..56aedb3ba954 100644
--- a/services/core/java/com/android/server/wm/ActivityStack.java
+++ b/services/core/java/com/android/server/wm/ActivityStack.java
@@ -1388,7 +1388,7 @@ class ActivityStack extends Task {
             boolean preserveWindows, boolean notifyClients) {
         mTopActivityOccludesKeyguard = false;
         mTopDismissingKeyguardActivity = null;
-        mStackSupervisor.getKeyguardController().beginActivityVisibilityUpdate();
+        mStackSupervisor.beginActivityVisibilityUpdate();
         try {
             mEnsureActivitiesVisibleHelper.process(
                     starting, configChanges, preserveWindows, notifyClients);
@@ -1399,7 +1399,7 @@ class ActivityStack extends Task {
                 notifyActivityDrawnLocked(null);
             }
         } finally {
-            mStackSupervisor.getKeyguardController().endActivityVisibilityUpdate();
+            mStackSupervisor.endActivityVisibilityUpdate();
         }
     }
 
diff --git a/services/core/java/com/android/server/wm/ActivityStackSupervisor.java b/services/core/java/com/android/server/wm/ActivityStackSupervisor.java
index fe9e5f3ca09e..4f8ad39d8ca2 100644
--- a/services/core/java/com/android/server/wm/ActivityStackSupervisor.java
+++ b/services/core/java/com/android/server/wm/ActivityStackSupervisor.java
@@ -332,11 +332,10 @@ public class ActivityStackSupervisor implements RecentTasks.Callbacks {
     PowerManager.WakeLock mGoingToSleepWakeLock;
 
     /**
-     * Temporary rect used during docked stack resize calculation so we don't need to create a new
-     * object each time.
+     * Used to keep {@link RootWindowContainer#ensureActivitiesVisible} from being entered
+     * recursively. And only update keyguard states once the nested updates are done.
      */
-    private final Rect tempRect = new Rect();
-    private final ActivityOptions mTmpOptions = ActivityOptions.makeBasic();
+    private int mVisibilityTransactionDepth;
 
     private ActivityMetricsLogger mActivityMetricsLogger;
 
@@ -1923,6 +1922,7 @@ public class ActivityStackSupervisor implements RecentTasks.Callbacks {
         pw.print(prefix);
         pw.println("mCurTaskIdForUser=" + mCurTaskIdForUser);
         pw.println(prefix + "mUserStackInFront=" + mRootWindowContainer.mUserStackInFront);
+        pw.println(prefix + "mVisibilityTransactionDepth=" + mVisibilityTransactionDepth);
         if (!mWaitingForActivityVisible.isEmpty()) {
             pw.println(prefix + "mWaitingForActivityVisible=");
             for (int i = 0; i < mWaitingForActivityVisible.size(); ++i) {
@@ -2314,6 +2314,24 @@ public class ActivityStackSupervisor implements RecentTasks.Callbacks {
                 "android.server.am:TURN_ON:" + reason);
     }
 
+    /** Starts a batch of visibility updates. */
+    void beginActivityVisibilityUpdate() {
+        mVisibilityTransactionDepth++;
+    }
+
+    /** Ends a batch of visibility updates. */
+    void endActivityVisibilityUpdate() {
+        mVisibilityTransactionDepth--;
+        if (mVisibilityTransactionDepth == 0) {
+            getKeyguardController().visibilitiesUpdated();
+        }
+    }
+
+    /** Returns {@code true} if the caller is on the path to update visibility. */
+    boolean inActivityVisibilityUpdate() {
+        return mVisibilityTransactionDepth > 0;
+    }
+
     /**
      * Begin deferring resume to avoid duplicate resumes in one pass.
      */
diff --git a/services/core/java/com/android/server/wm/KeyguardController.java b/services/core/java/com/android/server/wm/KeyguardController.java
index f672394251f2..4c10d5819c10 100644
--- a/services/core/java/com/android/server/wm/KeyguardController.java
+++ b/services/core/java/com/android/server/wm/KeyguardController.java
@@ -70,7 +70,6 @@ class KeyguardController {
     private boolean mKeyguardGoingAway;
     private boolean mDismissalRequested;
     private int mBeforeUnoccludeTransit;
-    private int mVisibilityTransactionDepth;
     private final SparseArray<KeyguardDisplayState> mDisplayStates = new SparseArray<>();
     private final ActivityTaskManagerService mService;
     private RootWindowContainer mRootWindowContainer;
@@ -252,24 +251,6 @@ class KeyguardController {
     }
 
     /**
-     * Starts a batch of visibility updates.
-     */
-    void beginActivityVisibilityUpdate() {
-        mVisibilityTransactionDepth++;
-    }
-
-    /**
-     * Ends a batch of visibility updates. After all batches are done, this method makes sure to
-     * update lockscreen occluded/dismiss state if needed.
-     */
-    void endActivityVisibilityUpdate() {
-        mVisibilityTransactionDepth--;
-        if (mVisibilityTransactionDepth == 0) {
-            visibilitiesUpdated();
-        }
-    }
-
-    /**
      * @return True if we may show an activity while Keyguard is showing because we are in the
      *         process of dismissing it anyways, false otherwise.
      */
@@ -292,7 +273,11 @@ class KeyguardController {
                 && !mWindowManager.isKeyguardSecure(mService.getCurrentUserId());
     }
 
-    private void visibilitiesUpdated() {
+    /**
+     * Makes sure to update lockscreen occluded/dismiss state if needed after completing all
+     * visibility updates ({@link ActivityStackSupervisor#endActivityVisibilityUpdate}).
+     */
+    void visibilitiesUpdated() {
         boolean requestDismissKeyguard = false;
         for (int displayNdx = mRootWindowContainer.getChildCount() - 1;
              displayNdx >= 0; displayNdx--) {
@@ -568,7 +553,6 @@ class KeyguardController {
         pw.println(prefix + "  mKeyguardGoingAway=" + mKeyguardGoingAway);
         dumpDisplayStates(pw, prefix);
         pw.println(prefix + "  mDismissalRequested=" + mDismissalRequested);
-        pw.println(prefix + "  mVisibilityTransactionDepth=" + mVisibilityTransactionDepth);
         pw.println();
     }
 
diff --git a/services/core/java/com/android/server/wm/RecentTasks.java b/services/core/java/com/android/server/wm/RecentTasks.java
index 09700c56deba..2d2e2e0fa662 100644
--- a/services/core/java/com/android/server/wm/RecentTasks.java
+++ b/services/core/java/com/android/server/wm/RecentTasks.java
@@ -1031,9 +1031,13 @@ class RecentTasks {
     void add(Task task) {
         if (DEBUG_RECENTS_TRIM_TASKS) Slog.d(TAG, "add: task=" + task);
 
+        // Only allow trimming task if it is not updating visibility for activities, so the caller
+        // doesn't need to handle unexpected size and index when looping task containers.
+        final boolean canTrimTask = !mSupervisor.inActivityVisibilityUpdate();
+
         // Clean up the hidden tasks when going to home because the user may not be unable to return
         // to the task from recents.
-        if (!mHiddenTasks.isEmpty() && task.isActivityTypeHome()) {
+        if (canTrimTask && !mHiddenTasks.isEmpty() && task.isActivityTypeHome()) {
             removeUnreachableHiddenTasks(task.getWindowingMode());
         }
 
@@ -1155,7 +1159,9 @@ class RecentTasks {
         }
 
         // Trim the set of tasks to the active set
-        trimInactiveRecentTasks();
+        if (canTrimTask) {
+            trimInactiveRecentTasks();
+        }
         notifyTaskPersisterLocked(task, false /* flush */);
     }
 
diff --git a/services/core/java/com/android/server/wm/RootWindowContainer.java b/services/core/java/com/android/server/wm/RootWindowContainer.java
index 77841dc2c0bf..b83881684995 100644
--- a/services/core/java/com/android/server/wm/RootWindowContainer.java
+++ b/services/core/java/com/android/server/wm/RootWindowContainer.java
@@ -262,9 +262,6 @@ class RootWindowContainer extends WindowContainer<DisplayContent>
     /** Set when a power hint has started, but not ended. */
     private boolean mPowerHintSent;
 
-    /** Used to keep ensureActivitiesVisible() from being entered recursively. */
-    private boolean mInEnsureActivitiesVisible = false;
-
     // The default minimal size that will be used if the activity doesn't specify its minimal size.
     // It will be calculated when the default display gets added.
     int mDefaultMinSizeOfResizeableTaskDp = -1;
@@ -1993,14 +1990,13 @@ class RootWindowContainer extends WindowContainer<DisplayContent>
      */
     void ensureActivitiesVisible(ActivityRecord starting, int configChanges,
             boolean preserveWindows, boolean notifyClients) {
-        if (mInEnsureActivitiesVisible) {
+        if (mStackSupervisor.inActivityVisibilityUpdate()) {
             // Don't do recursive work.
             return;
         }
-        mInEnsureActivitiesVisible = true;
 
         try {
-            mStackSupervisor.getKeyguardController().beginActivityVisibilityUpdate();
+            mStackSupervisor.beginActivityVisibilityUpdate();
             // First the front stacks. In case any are not fullscreen and are in front of home.
             for (int displayNdx = getChildCount() - 1; displayNdx >= 0; --displayNdx) {
                 final DisplayContent display = getChildAt(displayNdx);
@@ -2008,8 +2004,7 @@ class RootWindowContainer extends WindowContainer<DisplayContent>
                         notifyClients);
             }
         } finally {
-            mStackSupervisor.getKeyguardController().endActivityVisibilityUpdate();
-            mInEnsureActivitiesVisible = false;
+            mStackSupervisor.endActivityVisibilityUpdate();
         }
     }
 
diff --git a/services/tests/wmtests/src/com/android/server/wm/RecentTasksTest.java b/services/tests/wmtests/src/com/android/server/wm/RecentTasksTest.java
index ea933dfe42dc..5005c07832ab 100644
--- a/services/tests/wmtests/src/com/android/server/wm/RecentTasksTest.java
+++ b/services/tests/wmtests/src/com/android/server/wm/RecentTasksTest.java
@@ -46,6 +46,7 @@ import static org.mockito.ArgumentMatchers.anyBoolean;
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -276,6 +277,31 @@ public class RecentTasksTest extends ActivityTestsBase {
     }
 
     @Test
+    public void testAddTasksInVisibilityUpdate_expectNoTrim() {
+        mRecentTasks.setOnlyTestVisibleRange();
+        mRecentTasks.setParameters(-1 /* min */, 1 /* max */, -1 /* ms */);
+        mRecentTasks.add(mTasks.get(0));
+
+        doAnswer(invocation -> {
+            assertTrue(mSupervisor.inActivityVisibilityUpdate());
+            // Simulate an activity is resumed by EnsureActivitiesVisibleHelper. If its state is
+            // change to RESUMED, it will also be added to recents.
+            mRecentTasks.add(mTasks.get(1));
+            invocation.callRealMethod();
+            return null;
+        }).when(mSupervisor).endActivityVisibilityUpdate();
+
+        mTaskContainer.ensureActivitiesVisible(null /* starting */, 0 /* configChanges */,
+                false /* preserveWindows */, false /* notifyClients */);
+
+        assertFalse(mSupervisor.inActivityVisibilityUpdate());
+        assertThat(mCallbacksRecorder.mAdded).hasSize(2);
+        // Expect nothing is trimmed because we don't want the loop of ensure-visibility to be
+        // impacted by the arbitrary number of task removals.
+        assertNoTasksTrimmed();
+    }
+
+    @Test
     public void testAddTasksMultipleTasks_expectRemovedNoTrim() {
         // Add multiple same-affinity non-document tasks, ensure that it removes the other task,
         // but that it does not trim it