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
This commit is contained in:
Riddle Hsu
2022-04-21 17:51:41 +08:00
parent 2e495889be
commit eebc3162a2
3 changed files with 25 additions and 19 deletions

View File

@@ -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);

View File

@@ -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");

View File

@@ -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