From f0f64c1bee2c5295d6713de3787cfad9e164916d Mon Sep 17 00:00:00 2001 From: Charles Chen Date: Tue, 9 Jun 2020 16:31:13 +0800 Subject: Try to fix IndexOutBoundsException in Task#removeChild When calling Task#removeChild in a Task with finishing task overlay activities, it calls ActivityStackSupervisor#removeTask, and goes to performClearTaskAtIndexLocked and calls removeChild() recursively, which leads to iterating over mChildren twice. This CL introduces a flag mInRemoveTask to prevent the double invocation of removeTask, and also refactors performClearTaskAtIndesLocked a bit to align with the current behavior. Test: atest WmTests:TaskTests Bug: 157663342 Change-Id: I9e3bb6947bca9adc87497d16947de644f3227ee4 --- .../android/server/wm/ActivityStackSupervisor.java | 21 +++++++++++----- services/core/java/com/android/server/wm/Task.java | 21 ++++++++-------- .../src/com/android/server/wm/TaskTests.java | 28 ++++++++++++++++++++++ 3 files changed, 53 insertions(+), 17 deletions(-) (limited to 'services') diff --git a/services/core/java/com/android/server/wm/ActivityStackSupervisor.java b/services/core/java/com/android/server/wm/ActivityStackSupervisor.java index 407b9fcbca74..759af6a2b424 100644 --- a/services/core/java/com/android/server/wm/ActivityStackSupervisor.java +++ b/services/core/java/com/android/server/wm/ActivityStackSupervisor.java @@ -1504,12 +1504,21 @@ public class ActivityStackSupervisor implements RecentTasks.Callbacks { } void removeTask(Task task, boolean killProcess, boolean removeFromRecents, String reason) { - task.removeTaskActivitiesLocked(reason); - cleanUpRemovedTaskLocked(task, killProcess, removeFromRecents); - mService.getLockTaskController().clearLockedTask(task); - mService.getTaskChangeNotificationController().notifyTaskStackChanged(); - if (task.isPersistable) { - mService.notifyTaskPersisterLocked(null, true); + if (task.mInRemoveTask) { + // Prevent recursion. + return; + } + task.mInRemoveTask = true; + try { + task.performClearTask(reason); + cleanUpRemovedTaskLocked(task, killProcess, removeFromRecents); + mService.getLockTaskController().clearLockedTask(task); + mService.getTaskChangeNotificationController().notifyTaskStackChanged(); + if (task.isPersistable) { + mService.notifyTaskPersisterLocked(null, true); + } + } finally { + task.mInRemoveTask = false; } } diff --git a/services/core/java/com/android/server/wm/Task.java b/services/core/java/com/android/server/wm/Task.java index 748244e1a5c2..c664a841fc1f 100644 --- a/services/core/java/com/android/server/wm/Task.java +++ b/services/core/java/com/android/server/wm/Task.java @@ -435,6 +435,13 @@ class Task extends WindowContainer { static final int FLAG_FORCE_HIDDEN_FOR_TASK_ORG = 1 << 1; private int mForceHiddenFlags = 0; + // TODO(b/160201781): Revisit double invocation issue in Task#removeChild. + /** + * Skip {@link ActivityStackSupervisor#removeTask(Task, boolean, boolean, String)} execution if + * {@code true} to prevent double traversal of {@link #mChildren} in a loop. + */ + boolean mInRemoveTask; + // When non-null, this is a transaction that will get applied on the next frame returned after // a relayout is requested from the client. While this is only valid on a leaf task; since the // transaction can effect an ancestor task, this also needs to keep track of the ancestor task @@ -1496,11 +1503,8 @@ class Task extends WindowContainer { return autoRemoveRecents || (!hasChild() && !getHasBeenVisible()); } - /** - * Completely remove all activities associated with an existing - * task starting at a specified index. - */ - private void performClearTaskAtIndexLocked(String reason) { + /** Completely remove all activities associated with an existing task. */ + void performClearTask(String reason) { // Broken down into to cases to avoid object create due to capturing mStack. if (getStack() == null) { forAllActivities((r) -> { @@ -1524,7 +1528,7 @@ class Task extends WindowContainer { */ void performClearTaskLocked() { mReuseTask = true; - performClearTaskAtIndexLocked("clear-task-all"); + performClearTask("clear-task-all"); mReuseTask = false; } @@ -1585,11 +1589,6 @@ class Task extends WindowContainer { return false; } - void removeTaskActivitiesLocked(String reason) { - // Just remove the entire task. - performClearTaskAtIndexLocked(reason); - } - String lockTaskAuthToString() { switch (mLockTaskAuth) { case LOCK_TASK_AUTH_DONT_LOCK: return "LOCK_TASK_AUTH_DONT_LOCK"; diff --git a/services/tests/wmtests/src/com/android/server/wm/TaskTests.java b/services/tests/wmtests/src/com/android/server/wm/TaskTests.java index 1415c506a1c9..9d88ada5a90c 100644 --- a/services/tests/wmtests/src/com/android/server/wm/TaskTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/TaskTests.java @@ -28,6 +28,8 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.clearInvocations; import android.graphics.Point; @@ -158,4 +160,30 @@ public class TaskTests extends WindowTestsBase { assertEquals(activity1, task1.isInTask(activity1)); assertNull(task1.isInTask(activity2)); } + + @Test + public void testRemoveChildForOverlayTask() { + final Task task = createTaskStackOnDisplay(mDisplayContent); + final int taskId = task.mTaskId; + final ActivityRecord activity1 = + WindowTestUtils.createActivityRecordInTask(mDisplayContent, task); + final ActivityRecord activity2 = + WindowTestUtils.createActivityRecordInTask(mDisplayContent, task); + final ActivityRecord activity3 = + WindowTestUtils.createActivityRecordInTask(mDisplayContent, task); + activity1.setTaskOverlay(true); + activity2.setTaskOverlay(true); + activity3.setTaskOverlay(true); + + assertEquals(3, task.getChildCount()); + assertTrue(task.onlyHasTaskOverlayActivities(true)); + + task.removeChild(activity1); + + verify(task.mStackSupervisor).removeTask(any(), anyBoolean(), anyBoolean(), anyString()); + assertEquals(2, task.getChildCount()); + task.forAllActivities((r) -> { + assertTrue(r.finishing); + }); + } } -- cgit v1.2.3-59-g8ed1b