From f4740576264f25a99b113340673c3c09f592474e Mon Sep 17 00:00:00 2001 From: Evan Rosky Date: Tue, 8 Jun 2021 13:53:59 -0700 Subject: [PATCH] Fix task-organizer + shell transit interractions during tests During tests, task-organizers sometimes get registered/unregistered overtop the shell task-organizer. This can race with transition animations (most common during teardown where the instrumentation isn't waiting for anything and doesn't know what the test was doing). To remedy this, add a waiting mechanism to transition controller so that these operations (like register/unregister) won't run during transition animations. This is designed to have basically 0 impact on normal usage. Also, because chanhing organizers goes through a "migrateSurface" flow, it can also mix-up unfavorably with transitions (esp. in cases where Shell uses transitions to handle visibility). For this one case, we manually "show" task surfaces after they've been migrated even for organized tasks. This simulates the desired effect that register/unregister shouldn't alter the system state. Bug: 183993924 Test: atest CrossAppDragAndDropTests Change-Id: I6a8f80fe92442e1ac12be8e5ff4322b58567157a --- .../server/wm/TaskOrganizerController.java | 37 +++++++++++++--- .../server/wm/TransitionController.java | 43 +++++++++++++++++++ 2 files changed, 74 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/wm/TaskOrganizerController.java b/services/core/java/com/android/server/wm/TaskOrganizerController.java index 3a2ca80f2e122..fd91394ce6668 100644 --- a/services/core/java/com/android/server/wm/TaskOrganizerController.java +++ b/services/core/java/com/android/server/wm/TaskOrganizerController.java @@ -388,6 +388,15 @@ class TaskOrganizerController extends ITaskOrganizerController.Stub { mOrganizer.mTaskOrganizer, t); } } + if (mService.getTransitionController().isShellTransitionsEnabled()) { + // dispose is only called outside of transitions (eg during unregister). Since + // we "migrate" surfaces when replacing organizers, visibility gets delegated + // to transitions; however, since there is no transition at this point, we have + // to manually show the surface here. + if (t.mTaskOrganizer != null && t.getSurfaceControl() != null) { + t.getSyncTransaction().show(t.getSurfaceControl()); + } + } } // Remove organizer state after removing tasks so we get a chance to send @@ -480,7 +489,8 @@ class TaskOrganizerController extends ITaskOrganizerController.Stub { final int uid = Binder.getCallingUid(); final long origId = Binder.clearCallingIdentity(); try { - synchronized (mGlobalLock) { + final ArrayList taskInfos = new ArrayList<>(); + final Runnable withGlobalLock = () -> { ProtoLog.v(WM_DEBUG_WINDOW_ORGANIZER, "Register task organizer=%s uid=%d", organizer.asBinder(), uid); if (!mTaskOrganizerStates.containsKey(organizer.asBinder())) { @@ -489,10 +499,10 @@ class TaskOrganizerController extends ITaskOrganizerController.Stub { new TaskOrganizerState(organizer, uid)); } - final ArrayList taskInfos = new ArrayList<>(); final TaskOrganizerState state = mTaskOrganizerStates.get(organizer.asBinder()); mService.mRootWindowContainer.forAllTasks((task) -> { - if (ArrayUtils.contains(UNSUPPORTED_WINDOWING_MODES, task.getWindowingMode())) { + if (ArrayUtils.contains(UNSUPPORTED_WINDOWING_MODES, + task.getWindowingMode())) { return; } @@ -502,11 +512,19 @@ class TaskOrganizerController extends ITaskOrganizerController.Stub { if (returnTask) { SurfaceControl outSurfaceControl = state.addTaskWithoutCallback(task, "TaskOrganizerController.registerTaskOrganizer"); - taskInfos.add(new TaskAppearedInfo(task.getTaskInfo(), outSurfaceControl)); + taskInfos.add( + new TaskAppearedInfo(task.getTaskInfo(), outSurfaceControl)); } }); - return new ParceledListSlice<>(taskInfos); + }; + if (mService.getTransitionController().isShellTransitionsEnabled()) { + mService.getTransitionController().mRunningLock.runWhenIdle(1000, withGlobalLock); + } else { + synchronized (mGlobalLock) { + withGlobalLock.run(); + } } + return new ParceledListSlice<>(taskInfos); } finally { Binder.restoreCallingIdentity(origId); } @@ -518,7 +536,7 @@ class TaskOrganizerController extends ITaskOrganizerController.Stub { final int uid = Binder.getCallingUid(); final long origId = Binder.clearCallingIdentity(); try { - synchronized (mGlobalLock) { + final Runnable withGlobalLock = () -> { final TaskOrganizerState state = mTaskOrganizerStates.get(organizer.asBinder()); if (state == null) { return; @@ -527,6 +545,13 @@ class TaskOrganizerController extends ITaskOrganizerController.Stub { organizer.asBinder(), uid); state.unlinkDeath(); state.dispose(); + }; + if (mService.getTransitionController().isShellTransitionsEnabled()) { + mService.getTransitionController().mRunningLock.runWhenIdle(1000, withGlobalLock); + } else { + synchronized (mGlobalLock) { + withGlobalLock.run(); + } } } finally { Binder.restoreCallingIdentity(origId); diff --git a/services/core/java/com/android/server/wm/TransitionController.java b/services/core/java/com/android/server/wm/TransitionController.java index 9195f4d4f03de..3186aeac0df84 100644 --- a/services/core/java/com/android/server/wm/TransitionController.java +++ b/services/core/java/com/android/server/wm/TransitionController.java @@ -63,6 +63,8 @@ class TransitionController { */ private final ArrayList mPlayingTransitions = new ArrayList<>(); + final Lock mRunningLock = new Lock(); + private final IBinder.DeathRecipient mTransitionPlayerDeath = () -> { // clean-up/finish any playing transitions. for (int i = 0; i < mPlayingTransitions.size(); ++i) { @@ -70,6 +72,7 @@ class TransitionController { } mPlayingTransitions.clear(); mTransitionPlayer = null; + mRunningLock.doNotifyLocked(); }; /** The transition currently being constructed (collecting participants). */ @@ -285,6 +288,7 @@ class TransitionController { ProtoLog.v(ProtoLogGroup.WM_DEBUG_WINDOW_TRANSITIONS, "Finish Transition: %s", record); mPlayingTransitions.remove(record); record.finishTransition(); + mRunningLock.doNotifyLocked(); } void moveToPlaying(Transition transition) { @@ -357,4 +361,43 @@ class TransitionController { false /* keyguardGoingAway */); } } + + class Lock { + private int mTransitionWaiters = 0; + void runWhenIdle(long timeout, Runnable r) { + synchronized (mAtm.mGlobalLock) { + if (!inTransition()) { + r.run(); + return; + } + mTransitionWaiters += 1; + } + final long startTime = SystemClock.uptimeMillis(); + final long endTime = startTime + timeout; + while (true) { + synchronized (mAtm.mGlobalLock) { + if (!inTransition() || SystemClock.uptimeMillis() > endTime) { + mTransitionWaiters -= 1; + r.run(); + return; + } + } + synchronized (this) { + try { + this.wait(timeout); + } catch (InterruptedException e) { + return; + } + } + } + } + + void doNotifyLocked() { + synchronized (this) { + if (mTransitionWaiters > 0) { + this.notifyAll(); + } + } + } + } }