From eebc3162a2d7d8dad225cc7c52e5bc3ab200037a Mon Sep 17 00:00:00 2001 From: Riddle Hsu Date: Thu, 21 Apr 2022 17:51:41 +0800 Subject: [PATCH] Do not change created-by-organizer task to different organizer Otherwise when registering another organizer, all tasks will belong to the latest organizer. And since commit 4d63b2b, it will remove all created-by-organizer tasks when disposing, which also removes the tasks created by other organizer. That causes the original organizer to have unexpected state or crash when it becomes the active organizer. Remove parameter forceUpdate because it is no-op (setTaskOrganizer also skips for the same instance). It was used in commit 77338ab. Bug: 228977409 Test: atest WindowOrganizerTests# \ testUnregisterOrganizer_removesTasksCreatedByIt Test: atest AivityLifecycleLegacySplitScreenTests And enter split screen from recents. SystemUI should not crash. Change-Id: I1c495717c536f021a98b338189b7924630dcc7f3 --- .../core/java/com/android/server/wm/Task.java | 18 ++++++++-------- .../server/wm/TaskOrganizerController.java | 5 ++--- .../server/wm/WindowOrganizerTests.java | 21 ++++++++++++------- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/services/core/java/com/android/server/wm/Task.java b/services/core/java/com/android/server/wm/Task.java index a46544d6c9021..de5e0f332c46d 100644 --- a/services/core/java/com/android/server/wm/Task.java +++ b/services/core/java/com/android/server/wm/Task.java @@ -1169,7 +1169,7 @@ class Task extends TaskFragment { // Call this again after super onParentChanged in-case the surface wasn't created yet // (happens when the task is first inserted into the hierarchy). It's a no-op if it // already ran fully within super.onParentChanged - updateTaskOrganizerState(false /* forceUpdate */); + updateTaskOrganizerState(); // TODO(b/168037178): The check for null display content and setting it to null doesn't // really make sense here... @@ -1945,7 +1945,7 @@ class Task extends TaskFragment { } saveLaunchingStateIfNeeded(); - final boolean taskOrgChanged = updateTaskOrganizerState(false /* forceUpdate */); + final boolean taskOrgChanged = updateTaskOrganizerState(); if (taskOrgChanged) { updateSurfacePosition(getSyncTransaction()); if (!isOrganized()) { @@ -4261,21 +4261,18 @@ class Task extends TaskFragment { return true; } - boolean updateTaskOrganizerState(boolean forceUpdate) { - return updateTaskOrganizerState(forceUpdate, false /* skipTaskAppeared */); + boolean updateTaskOrganizerState() { + return updateTaskOrganizerState(false /* skipTaskAppeared */); } /** * Called when the task state changes (ie. from windowing mode change) an the task organizer * state should also be updated. * - * @param forceUpdate Updates the task organizer to the one currently specified in the task - * org controller for the task's windowing mode, ignoring the cached - * windowing mode checks. * @param skipTaskAppeared Skips calling taskAppeared for the new organizer if it has changed * @return {@code true} if task organizer changed. */ - boolean updateTaskOrganizerState(boolean forceUpdate, boolean skipTaskAppeared) { + boolean updateTaskOrganizerState(boolean skipTaskAppeared) { if (getSurfaceControl() == null) { // Can't call onTaskAppeared without a surfacecontrol, so defer this until next one // is created. @@ -4287,7 +4284,10 @@ class Task extends TaskFragment { final TaskOrganizerController controller = mWmService.mAtmService.mTaskOrganizerController; final ITaskOrganizer organizer = controller.getTaskOrganizer(); - if (!forceUpdate && mTaskOrganizer == organizer) { + // Do not change to different organizer if the task is created by organizer because only + // the creator knows how to manage it. + if (mCreatedByOrganizer && mTaskOrganizer != null && organizer != null + && mTaskOrganizer != organizer) { return false; } return setTaskOrganizer(organizer, skipTaskAppeared); diff --git a/services/core/java/com/android/server/wm/TaskOrganizerController.java b/services/core/java/com/android/server/wm/TaskOrganizerController.java index 8a0ae65267a32..d6ebdf70194a7 100644 --- a/services/core/java/com/android/server/wm/TaskOrganizerController.java +++ b/services/core/java/com/android/server/wm/TaskOrganizerController.java @@ -256,7 +256,7 @@ class TaskOrganizerController extends ITaskOrganizerController.Stub { // organizer is disposed off to avoid inconsistent behavior. t.removeImmediately(); } else { - t.updateTaskOrganizerState(true /* forceUpdate */); + t.updateTaskOrganizerState(); } if (mOrganizedTasks.contains(t)) { // updateTaskOrganizerState should remove the task from the list, but still @@ -380,8 +380,7 @@ class TaskOrganizerController extends ITaskOrganizerController.Stub { final TaskOrganizerState state = mTaskOrganizerStates.get(organizer.asBinder()); mService.mRootWindowContainer.forAllTasks((task) -> { boolean returnTask = !task.mCreatedByOrganizer; - task.updateTaskOrganizerState(true /* forceUpdate */, - returnTask /* skipTaskAppeared */); + task.updateTaskOrganizerState(returnTask /* skipTaskAppeared */); if (returnTask) { SurfaceControl outSurfaceControl = state.addTaskWithoutCallback(task, "TaskOrganizerController.registerTaskOrganizer"); diff --git a/services/tests/wmtests/src/com/android/server/wm/WindowOrganizerTests.java b/services/tests/wmtests/src/com/android/server/wm/WindowOrganizerTests.java index 832bd2d8b9517..40ca2506fab1a 100644 --- a/services/tests/wmtests/src/com/android/server/wm/WindowOrganizerTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/WindowOrganizerTests.java @@ -370,13 +370,16 @@ public class WindowOrganizerTests extends WindowTestsBase { // Ensure events dispatch to organizer. mWm.mAtmService.mTaskOrganizerController.dispatchPendingEvents(); assertContainsTasks(existingTasks2, rootTask); - verify(organizer2, times(1)).onTaskAppeared(any(RunningTaskInfo.class), + verify(organizer2, never()).onTaskAppeared(any(RunningTaskInfo.class), any(SurfaceControl.class)); verify(organizer2, times(0)).onTaskVanished(any()); - // Removed tasks from the original organizer - assertTaskVanished(organizer, true /* expectVanished */, rootTask, rootTask2); - assertTrue(rootTask2.isOrganized()); + // The non-CreatedByOrganizer task is removed from the original organizer. + assertTaskVanished(organizer, true /* expectVanished */, rootTask); + assertEquals(organizer2, rootTask.mTaskOrganizer); + // The CreatedByOrganizer task should be still organized by the original organizer. + assertEquals(organizer, rootTask2.mTaskOrganizer); + clearInvocations(organizer); // Now we unregister the second one, the first one should automatically be reregistered // so we verify that it's now seeing changes. mWm.mAtmService.mTaskOrganizerController.unregisterTaskOrganizer(organizer2); @@ -385,9 +388,13 @@ public class WindowOrganizerTests extends WindowTestsBase { verify(organizer, times(2)) .onTaskAppeared(any(RunningTaskInfo.class), any(SurfaceControl.class)); - assertFalse(rootTask2.isOrganized()); - assertTaskVanished(organizer2, true /* expectVanished */, rootTask, - rootTask2); + + // Unregister the first one. The CreatedByOrganizer task created by it must be removed. + mWm.mAtmService.mTaskOrganizerController.unregisterTaskOrganizer(organizer); + assertFalse(rootTask2.isAttached()); + assertFalse(task2.isAttached()); + // Normal task should keep. + assertTrue(task.isAttached()); } @Test