From 1cf47ef3fc9ce2579d7f356dde2ea8c4f195b77e Mon Sep 17 00:00:00 2001 From: Evan Rosky Date: Wed, 13 Jul 2022 20:48:40 -0700 Subject: [PATCH] Prevent double-applying transactions With BLAST, transactions contain buffers. Since we pass the transaction between processes, its possible to commit the transaction twice. If the transaction contains buffers, it will confuse surfaceflinger. Account for this by clearing transactions when they are sent to remotes. The merge case is non-trivial, though, because the remote may not actually merge. So, for that, we clear the transaction only when it is merged. Fixing this revealed a bug in an edge-case triggered by tapltests, so this fixes that as well. Bug: 238328090 Test: atest TaplTestsLauncher3 Change-Id: If30b0bd4e8fad4658365826a25eac38dc06af4c7 --- .../transition/OneShotRemoteHandler.java | 7 ++++++ .../transition/RemoteTransitionHandler.java | 7 ++++++ .../shared/system/RemoteTransitionCompat.java | 23 +++++++++++++++++++ 3 files changed, 37 insertions(+) diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/transition/OneShotRemoteHandler.java b/libs/WindowManager/Shell/src/com/android/wm/shell/transition/OneShotRemoteHandler.java index 3e2a0e635a75f..ebaece2189aa3 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/transition/OneShotRemoteHandler.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/transition/OneShotRemoteHandler.java @@ -99,6 +99,8 @@ public class OneShotRemoteHandler implements Transitions.TransitionHandler { + " during unit tests"); } mRemote.getRemoteTransition().startAnimation(transition, info, startTransaction, cb); + // assume that remote will apply the start transaction. + startTransaction.clear(); } catch (RemoteException e) { Log.e(Transitions.TAG, "Error running remote transition.", e); if (mRemote.asBinder() != null) { @@ -120,6 +122,11 @@ public class OneShotRemoteHandler implements Transitions.TransitionHandler { @Override public void onTransitionFinished(WindowContainerTransaction wct, SurfaceControl.Transaction sct) { + // We have merged, since we sent the transaction over binder, the one in this + // process won't be cleared if the remote applied it. We don't actually know if the + // remote applied the transaction, but applying twice will break surfaceflinger + // so just assume the worst-case and clear the local transaction. + t.clear(); mMainExecutor.execute( () -> finishCallback.onTransitionFinished(wct, null /* wctCB */)); } diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/transition/RemoteTransitionHandler.java b/libs/WindowManager/Shell/src/com/android/wm/shell/transition/RemoteTransitionHandler.java index ece9f47e8788f..84a46ff69b683 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/transition/RemoteTransitionHandler.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/transition/RemoteTransitionHandler.java @@ -139,6 +139,8 @@ public class RemoteTransitionHandler implements Transitions.TransitionHandler { + " during unit tests"); } remote.getRemoteTransition().startAnimation(transition, info, startTransaction, cb); + // assume that remote will apply the start transaction. + startTransaction.clear(); } catch (RemoteException e) { Log.e(Transitions.TAG, "Error running remote transition.", e); unhandleDeath(remote.asBinder(), finishCallback); @@ -162,6 +164,11 @@ public class RemoteTransitionHandler implements Transitions.TransitionHandler { @Override public void onTransitionFinished(WindowContainerTransaction wct, SurfaceControl.Transaction sct) { + // We have merged, since we sent the transaction over binder, the one in this + // process won't be cleared if the remote applied it. We don't actually know if the + // remote applied the transaction, but applying twice will break surfaceflinger + // so just assume the worst-case and clear the local transaction. + t.clear(); mMainExecutor.execute(() -> { if (!mRequestedRemotes.containsKey(mergeTarget)) { Log.e(TAG, "Merged transition finished after it's mergeTarget (the " diff --git a/packages/SystemUI/shared/src/com/android/systemui/shared/system/RemoteTransitionCompat.java b/packages/SystemUI/shared/src/com/android/systemui/shared/system/RemoteTransitionCompat.java index a66dc77437925..ff2a7a1322887 100644 --- a/packages/SystemUI/shared/src/com/android/systemui/shared/system/RemoteTransitionCompat.java +++ b/packages/SystemUI/shared/src/com/android/systemui/shared/system/RemoteTransitionCompat.java @@ -224,6 +224,7 @@ public class RemoteTransitionCompat implements Parcelable { private WindowContainerToken mRecentsTask = null; private TransitionInfo mInfo = null; private ArrayList mOpeningLeashes = null; + private boolean mOpeningHome = false; private ArrayMap mLeashMap = null; private PictureInPictureSurfaceTransaction mPipTransaction = null; private IBinder mTransition = null; @@ -321,6 +322,7 @@ public class RemoteTransitionCompat implements Parcelable { } final int layer = mInfo.getChanges().size() * 3; mOpeningLeashes = new ArrayList<>(); + mOpeningHome = cancelRecents; final RemoteAnimationTargetCompat[] targets = new RemoteAnimationTargetCompat[openingTasks.size()]; for (int i = 0; i < openingTasks.size(); ++i) { @@ -406,6 +408,26 @@ public class RemoteTransitionCompat implements Parcelable { if (!mKeyguardLocked && mRecentsTask != null) { wct.restoreTransientOrder(mRecentsTask); } + } else if (toHome && mOpeningHome && mPausingTasks != null) { + // Special situaition where 3p launcher was changed during recents (this happens + // during tapltests...). Here we get both "return to home" AND "home opening". + // This is basically going home, but we have to restore recents order and also + // treat the home "pausing" task properly. + for (int i = mPausingTasks.size() - 1; i >= 0; --i) { + final TransitionInfo.Change change = mInfo.getChange(mPausingTasks.get(i)); + final ActivityManager.RunningTaskInfo taskInfo = change.getTaskInfo(); + if (taskInfo.topActivityType == ACTIVITY_TYPE_HOME) { + // Treat as opening (see above) + wct.reorder(mPausingTasks.get(i), true /* onTop */); + t.show(mInfo.getChange(mPausingTasks.get(i)).getLeash()); + } else { + // Treat as hiding (see below) + t.hide(mInfo.getChange(mPausingTasks.get(i)).getLeash()); + } + } + if (!mKeyguardLocked && mRecentsTask != null) { + wct.restoreTransientOrder(mRecentsTask); + } } else { for (int i = 0; i < mPausingTasks.size(); ++i) { if (!sendUserLeaveHint) { @@ -444,6 +466,7 @@ public class RemoteTransitionCompat implements Parcelable { mPausingTasks = null; mInfo = null; mOpeningLeashes = null; + mOpeningHome = false; mLeashMap = null; mTransition = null; }