Fix duplicate-call issues in remote-display-change

The display-change code was just accumulating callbacks without
ever removing them. This is both a memory leak and was calling
stale callbacks which yielded out-of-order changes.

Additionally, it was calling all callbacks for each individual
callback received (meaning the first of 2 in-flight remotes
would call both callbacks before the second was received). This
meant that the transactions were being used in the wrong
callbacks and that some callbacks would be called out-of-order.

Lastly, the display-rotation failure-case was wrong. Its supposed
to fall-back on the new rotation not the original rotation.

Bug: 232477270
Test: atest PinnedStackTests -- shouldn't crash due to
      rotation-outside-of-transition anymore.
Change-Id: Icc7a90448edb648a8110cd83018a1477111beedd

3 files changed, 48 insertions, 68 deletions

diff --git a/services/core/java/com/android/server/wm/DisplayRotation.java b/services/core/java/com/android/server/wm/DisplayRotation.java
index 86ed44e39425..e5ec4c3dfcd2 100644
--- a/services/core/java/com/android/server/wm/DisplayRotation.java
+++ b/services/core/java/com/android/server/wm/DisplayRotation.java
@@ -530,13 +530,7 @@ public class DisplayRotation {
     private void startRemoteRotation(int fromRotation, int toRotation) {
         mDisplayContent.mRemoteDisplayChangeController.performRemoteDisplayChange(
                 fromRotation, toRotation, null /* newDisplayAreaInfo */,
-                (appliedChange, transaction) -> {
-                    final int newRotation = appliedChange != null
-                            ? appliedChange.toRotation
-                            // Timeout occurred, use old rotation
-                            : fromRotation;
-                    continueRotation(newRotation, transaction);
-                }
+                (transaction) -> continueRotation(toRotation, transaction)
         );
     }
 
diff --git a/services/core/java/com/android/server/wm/PhysicalDisplaySwitchTransitionLauncher.java b/services/core/java/com/android/server/wm/PhysicalDisplaySwitchTransitionLauncher.java
index 3e01f08994f8..d209f08e6312 100644
--- a/services/core/java/com/android/server/wm/PhysicalDisplaySwitchTransitionLauncher.java
+++ b/services/core/java/com/android/server/wm/PhysicalDisplaySwitchTransitionLauncher.java
@@ -120,7 +120,7 @@ public class PhysicalDisplaySwitchTransitionLauncher {
 
         final boolean started = mDisplayContent.mRemoteDisplayChangeController
                 .performRemoteDisplayChange(fromRotation, toRotation, newDisplayAreaInfo,
-                        (appliedChange, transaction) -> continueDisplayUpdate(transaction));
+                        this::continueDisplayUpdate);
 
         if (!started) {
             markTransitionAsReady();
diff --git a/services/core/java/com/android/server/wm/RemoteDisplayChangeController.java b/services/core/java/com/android/server/wm/RemoteDisplayChangeController.java
index 553384b06268..43baebc7255a 100644
--- a/services/core/java/com/android/server/wm/RemoteDisplayChangeController.java
+++ b/services/core/java/com/android/server/wm/RemoteDisplayChangeController.java
@@ -18,14 +18,15 @@ package com.android.server.wm;
 
 import static com.android.internal.protolog.ProtoLogGroup.WM_DEBUG_CONFIGURATION;
 
+import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.os.RemoteException;
+import android.util.Slog;
 import android.view.IDisplayChangeWindowCallback;
 import android.window.DisplayAreaInfo;
 import android.window.WindowContainerTransaction;
 
 import com.android.internal.protolog.common.ProtoLog;
-import com.android.internal.util.function.pooled.PooledLambda;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -38,16 +39,16 @@ import java.util.List;
  */
 public class RemoteDisplayChangeController {
 
+    private static final String TAG = "RemoteDisplayChangeController";
+
     private static final int REMOTE_DISPLAY_CHANGE_TIMEOUT_MS = 800;
 
     private final WindowManagerService mService;
     private final int mDisplayId;
 
-    private boolean mIsWaitingForRemoteDisplayChange;
-    private final Runnable mTimeoutRunnable = () -> {
-        continueDisplayChange(null /* appliedChange */, null /* transaction */);
-    };
+    private final Runnable mTimeoutRunnable = this::onContinueTimedOut;
 
+    // all remote changes that haven't finished yet.
     private final List<ContinueRemoteDisplayChangeCallback> mCallbacks = new ArrayList<>();
 
     public RemoteDisplayChangeController(WindowManagerService service, int displayId) {
@@ -61,7 +62,7 @@ public class RemoteDisplayChangeController {
      *  to perform in sync with the display change.
      */
     public boolean isWaitingForRemoteDisplayChange() {
-        return mIsWaitingForRemoteDisplayChange;
+        return !mCallbacks.isEmpty();
     }
 
     /**
@@ -79,7 +80,6 @@ public class RemoteDisplayChangeController {
         if (mService.mDisplayChangeController == null) {
             return false;
         }
-        mIsWaitingForRemoteDisplayChange = true;
         mCallbacks.add(callback);
 
         if (newDisplayAreaInfo != null) {
@@ -95,91 +95,77 @@ public class RemoteDisplayChangeController {
                     toRotation);
         }
 
-        final RemoteDisplayChange change = new RemoteDisplayChange(fromRotation, toRotation,
-                newDisplayAreaInfo);
-        final IDisplayChangeWindowCallback remoteCallback = createCallback(change);
+        final IDisplayChangeWindowCallback remoteCallback = createCallback(callback);
         try {
-            mService.mDisplayChangeController.onDisplayChange(mDisplayId, fromRotation, toRotation,
-                    newDisplayAreaInfo, remoteCallback);
-
             mService.mH.removeCallbacks(mTimeoutRunnable);
             mService.mH.postDelayed(mTimeoutRunnable, REMOTE_DISPLAY_CHANGE_TIMEOUT_MS);
+            mService.mDisplayChangeController.onDisplayChange(mDisplayId, fromRotation, toRotation,
+                    newDisplayAreaInfo, remoteCallback);
             return true;
         } catch (RemoteException e) {
-            mIsWaitingForRemoteDisplayChange = false;
+            Slog.e(TAG, "Exception while dispatching remote display-change", e);
+            mCallbacks.remove(callback);
             return false;
         }
     }
 
-    private void continueDisplayChange(@Nullable RemoteDisplayChange appliedChange,
-            @Nullable WindowContainerTransaction transaction) {
+    private void onContinueTimedOut() {
+        Slog.e(TAG, "RemoteDisplayChange timed-out, UI might get messed-up after this.");
+        // timed-out, so run all continue callbacks and clear the list
         synchronized (mService.mGlobalLock) {
-            if (appliedChange != null) {
-                ProtoLog.v(WM_DEBUG_CONFIGURATION,
-                        "Received remote change for Display[%d], applied: [%dx%d, rot = %d]",
-                        mDisplayId,
-                        appliedChange.displayAreaInfo != null ? appliedChange.displayAreaInfo
-                                .configuration.windowConfiguration.getMaxBounds().width() : -1,
-                        appliedChange.displayAreaInfo != null ? appliedChange.displayAreaInfo
-                                .configuration.windowConfiguration.getMaxBounds().height() : -1,
-                        appliedChange.toRotation);
-            } else {
-                ProtoLog.v(WM_DEBUG_CONFIGURATION, "Remote change for Display[%d]: timeout reached",
-                        mDisplayId);
+            for (int i = 0; i < mCallbacks.size(); ++i) {
+                mCallbacks.get(i).onContinueRemoteDisplayChange(null /* transaction */);
             }
+            mCallbacks.clear();
+        }
+    }
 
-            mIsWaitingForRemoteDisplayChange = false;
-
-            for (int i = 0; i < mCallbacks.size(); i++) {
-                ContinueRemoteDisplayChangeCallback callback = mCallbacks.get(i);
-                callback.onContinueRemoteDisplayChange(appliedChange, transaction);
+    private void continueDisplayChange(@NonNull ContinueRemoteDisplayChangeCallback callback,
+            @Nullable WindowContainerTransaction transaction) {
+        synchronized (mService.mGlobalLock) {
+            int idx = mCallbacks.indexOf(callback);
+            if (idx < 0) {
+                // already called this callback or a more-recent one (eg. via timeout)
+                return;
+            }
+            for (int i = 0; i < idx; ++i) {
+                // Expect remote callbacks in order. If they don't come in order, then force
+                // ordering by continuing everything up until this one with empty transactions.
+                mCallbacks.get(i).onContinueRemoteDisplayChange(null /* transaction */);
             }
+            mCallbacks.subList(0, idx + 1).clear();
+            if (mCallbacks.isEmpty()) {
+                mService.mH.removeCallbacks(mTimeoutRunnable);
+            }
+            callback.onContinueRemoteDisplayChange(transaction);
         }
     }
 
-    private IDisplayChangeWindowCallback createCallback(RemoteDisplayChange originalChange) {
+    private IDisplayChangeWindowCallback createCallback(
+            @NonNull ContinueRemoteDisplayChangeCallback callback) {
         return new IDisplayChangeWindowCallback.Stub() {
                     @Override
                     public void continueDisplayChange(WindowContainerTransaction t) {
                         synchronized (mService.mGlobalLock) {
-                            mService.mH.removeCallbacks(mTimeoutRunnable);
-                            mService.mH.sendMessage(PooledLambda.obtainMessage(
-                                    RemoteDisplayChangeController::continueDisplayChange,
-                                    RemoteDisplayChangeController.this,
-                                    originalChange, t));
+                            if (!mCallbacks.contains(callback)) {
+                                // already ran this callback or a more-recent one.
+                                return;
+                            }
+                            mService.mH.post(() -> RemoteDisplayChangeController.this
+                                    .continueDisplayChange(callback, t));
                         }
                     }
                 };
     }
 
     /**
-     * Data class that contains information about a remote display change
-     */
-    public static class RemoteDisplayChange {
-        final int fromRotation;
-        final int toRotation;
-        @Nullable
-        final DisplayAreaInfo displayAreaInfo;
-
-        public RemoteDisplayChange(int fromRotation, int toRotation,
-                @Nullable DisplayAreaInfo displayAreaInfo) {
-            this.fromRotation = fromRotation;
-            this.toRotation = toRotation;
-            this.displayAreaInfo = displayAreaInfo;
-        }
-    }
-
-    /**
      * Callback interface to handle continuation of the remote display change
      */
     public interface ContinueRemoteDisplayChangeCallback {
         /**
          * This method is called when the remote display change has been applied
-         * @param appliedChange the change that was applied or null if there was
-         *                      an error during remote display change (e.g. timeout)
          * @param transaction window changes collected by the remote display change
          */
-        void onContinueRemoteDisplayChange(@Nullable RemoteDisplayChange appliedChange,
-                @Nullable WindowContainerTransaction transaction);
+        void onContinueRemoteDisplayChange(@Nullable WindowContainerTransaction transaction);
     }
 }