From 271f4fca0100908be04a56f1c8eba63103ceb8dd Mon Sep 17 00:00:00 2001 From: Evan Rosky Date: Fri, 13 May 2022 10:22:47 -0700 Subject: [PATCH] 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 --- .../android/server/wm/DisplayRotation.java | 8 +- ...ysicalDisplaySwitchTransitionLauncher.java | 2 +- .../wm/RemoteDisplayChangeController.java | 110 ++++++++---------- 3 files changed, 50 insertions(+), 70 deletions(-) diff --git a/services/core/java/com/android/server/wm/DisplayRotation.java b/services/core/java/com/android/server/wm/DisplayRotation.java index 86ed44e394250..e5ec4c3dfcd25 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 3e01f08994f89..d209f08e6312c 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 553384b06268c..43baebc7255aa 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 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); - } - - mIsWaitingForRemoteDisplayChange = false; - - for (int i = 0; i < mCallbacks.size(); i++) { - ContinueRemoteDisplayChangeCallback callback = mCallbacks.get(i); - callback.onContinueRemoteDisplayChange(appliedChange, transaction); + for (int i = 0; i < mCallbacks.size(); ++i) { + mCallbacks.get(i).onContinueRemoteDisplayChange(null /* transaction */); } + mCallbacks.clear(); } } - private IDisplayChangeWindowCallback createCallback(RemoteDisplayChange originalChange) { + 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( + @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); } }