From 78c483328a20adc7557db98b88c8484a6a9b695a Mon Sep 17 00:00:00 2001
From: Evan Rosky <erosky@google.com>
Date: Wed, 26 Apr 2023 13:42:24 -0700
Subject: [PATCH] Prevent uncommitted activity visibilities

There are some degenerate cases (in tests currently) where
visibility is changed unexpectedly in the finishTransaction.
These are incorrect, but to make things a little more robust
and also to prevent unrelated changes from breaking tests,
add a failsafe mechanism.

Bug: 279672199
Test: atest NexusLauncherTests (need to run whole suite cuz
      it just happens randomly, not due to a specific test).
Change-Id: I19da051c0f6a476f61578cc5554c7b0eb2a66644
---
 .../com/android/server/wm/ActivityRecord.java   |  4 +++-
 .../java/com/android/server/wm/Transition.java  | 17 ++++++++++++-----
 .../android/server/wm/TransitionController.java | 16 ++++++++++++++++
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/services/core/java/com/android/server/wm/ActivityRecord.java b/services/core/java/com/android/server/wm/ActivityRecord.java
index 78c066bdc212f..c3cd3eca84f21 100644
--- a/services/core/java/com/android/server/wm/ActivityRecord.java
+++ b/services/core/java/com/android/server/wm/ActivityRecord.java
@@ -5400,7 +5400,9 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A
             return;
         }
         if (inFinishingTransition) {
-            // Let the finishing transition commit the visibility.
+            // Let the finishing transition commit the visibility, but let the controller know
+            // about it so that we can recover from degenerate cases.
+            mTransitionController.mValidateCommitVis.add(this);
             return;
         }
         // If we are preparing an app transition, then delay changing
diff --git a/services/core/java/com/android/server/wm/Transition.java b/services/core/java/com/android/server/wm/Transition.java
index d531ad175f108..ae6757c869bdb 100644
--- a/services/core/java/com/android/server/wm/Transition.java
+++ b/services/core/java/com/android/server/wm/Transition.java
@@ -984,13 +984,20 @@ class Transition implements BLASTSyncEngine.TransactionReadyListener {
             // Record all the now-hiding activities so that they are committed. Just use
             // mParticipants because we can avoid a new list this way.
             for (int i = 0; i < mTransientHideTasks.size(); ++i) {
-                // Only worry about tasks that were actually hidden. Otherwise, we could end-up
-                // committing visibility for activity-level changes that aren't part of this
-                // transition.
-                if (mTransientHideTasks.get(i).isVisibleRequested()) continue;
-                mTransientHideTasks.get(i).forAllActivities(r -> {
+                final Task rootTask = mTransientHideTasks.get(i);
+                rootTask.forAllActivities(r -> {
                     // Only check leaf-tasks that were collected
                     if (!mParticipants.contains(r.getTask())) return;
+                    if (rootTask.isVisibleRequested()) {
+                        // This transient-hide didn't hide, so don't commit anything (otherwise we
+                        // could prematurely commit invisible on unrelated activities). To be safe,
+                        // though, notify the controller to prevent degenerate cases.
+                        if (!r.isVisibleRequested()) {
+                            mController.mValidateCommitVis.add(r);
+                        }
+                        return;
+                    }
+                    // This did hide: commit immediately so that other transitions know about it.
                     mParticipants.add(r);
                 });
             }
diff --git a/services/core/java/com/android/server/wm/TransitionController.java b/services/core/java/com/android/server/wm/TransitionController.java
index 7950edaaa267a..d7f096f6d266c 100644
--- a/services/core/java/com/android/server/wm/TransitionController.java
+++ b/services/core/java/com/android/server/wm/TransitionController.java
@@ -132,6 +132,13 @@ class TransitionController {
      */
     final ArrayList<Runnable> mStateValidators = new ArrayList<>();
 
+    /**
+     * List of activity-records whose visibility changed outside the main/tracked part of a
+     * transition (eg. in the finish-transaction). These will be checked when idle to recover from
+     * degenerate states.
+     */
+    final ArrayList<ActivityRecord> mValidateCommitVis = new ArrayList<>();
+
     /**
      * Currently playing transitions (in the order they were started). When finished, records are
      * removed from this list.
@@ -848,6 +855,15 @@ class TransitionController {
             }
         }
         mStateValidators.clear();
+        for (int i = 0; i < mValidateCommitVis.size(); ++i) {
+            final ActivityRecord ar = mValidateCommitVis.get(i);
+            if (!ar.isVisibleRequested() && ar.isVisible()) {
+                Slog.e(TAG, "Uncommitted visibility change: " + ar);
+                ar.commitVisibility(ar.isVisibleRequested(), false /* layout */,
+                        false /* fromTransition */);
+            }
+        }
+        mValidateCommitVis.clear();
     }
 
     /**