From 8a88ca8131a87b5f5e3614c0cec07da992132546 Mon Sep 17 00:00:00 2001 From: Riddle Hsu Date: Mon, 29 Jun 2020 21:08:56 +0800 Subject: [PATCH] Fix index out of bounds when removing deferred containers This makes the commit 793442c general to handle the removal from all level since the hierarchy won't always keep the same shape. Also rename to handleCompleteDeferredRemoval since it is not a pure function for checking. Fixes: 159865315 Test: atest WindowContainerTests#testHandleCompleteDeferredRemoval Change-Id: I34e02652fb765c1cf78cbd3e86a07e592f02e7df Merged-In: I34e02652fb765c1cf78cbd3e86a07e592f02e7df --- .../com/android/server/wm/ActivityRecord.java | 4 +-- .../com/android/server/wm/ActivityStack.java | 17 ++---------- .../com/android/server/wm/DisplayContent.java | 24 +++-------------- .../server/wm/RootWindowContainer.java | 4 +-- .../android/server/wm/WindowContainer.java | 18 ++++++++++--- .../server/wm/WindowContainerTests.java | 26 +++++++++++++++++++ 6 files changed, 50 insertions(+), 43 deletions(-) diff --git a/services/core/java/com/android/server/wm/ActivityRecord.java b/services/core/java/com/android/server/wm/ActivityRecord.java index c4663ca19f87f..2e9f704484886 100644 --- a/services/core/java/com/android/server/wm/ActivityRecord.java +++ b/services/core/java/com/android/server/wm/ActivityRecord.java @@ -3146,11 +3146,11 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A } @Override - boolean checkCompleteDeferredRemoval() { + boolean handleCompleteDeferredRemoval() { if (mIsExiting) { removeIfPossible(); } - return super.checkCompleteDeferredRemoval(); + return super.handleCompleteDeferredRemoval(); } void onRemovedFromDisplay() { diff --git a/services/core/java/com/android/server/wm/ActivityStack.java b/services/core/java/com/android/server/wm/ActivityStack.java index b4bc0f5b3a32d..8f0de7312ee5a 100644 --- a/services/core/java/com/android/server/wm/ActivityStack.java +++ b/services/core/java/com/android/server/wm/ActivityStack.java @@ -86,7 +86,6 @@ import static com.android.server.wm.TaskProto.ACTIVITY_TYPE; import static com.android.server.wm.TaskProto.ANIMATING_BOUNDS; import static com.android.server.wm.TaskProto.BOUNDS; import static com.android.server.wm.TaskProto.CREATED_BY_ORGANIZER; -import static com.android.server.wm.TaskProto.DEFER_REMOVAL; import static com.android.server.wm.TaskProto.DISPLAY_ID; import static com.android.server.wm.TaskProto.FILLS_PARENT; import static com.android.server.wm.TaskProto.LAST_NON_FULLSCREEN_BOUNDS; @@ -248,11 +247,6 @@ class ActivityStack extends Task { private Rect mTmpRect = new Rect(); private Rect mTmpRect2 = new Rect(); - /** Detach this stack from its display when animation completes. */ - // TODO: maybe tie this to WindowContainer#removeChild some how... - // TODO: This is no longer set. Okay to remove or was the set removed by accident? - private boolean mDeferRemoval; - // If this is true, we are in the bounds animating mode. The task will be down or upscaled to // perfectly fit the region it would have been cropped to. We may also avoid certain logic we // would otherwise apply while resizing, while resizing in the bounds animating mode. @@ -3232,9 +3226,6 @@ class ActivityStack extends Task { @Override void dump(PrintWriter pw, String prefix, boolean dumpAll) { - if (mDeferRemoval) { - pw.println(prefix + "mDeferRemoval=true"); - } super.dump(pw, prefix, dumpAll); if (!mExitingActivities.isEmpty()) { pw.println(); @@ -3304,15 +3295,12 @@ class ActivityStack extends Task { } /** Returns true if a removal action is still being deferred. */ - boolean checkCompleteDeferredRemoval() { + boolean handleCompleteDeferredRemoval() { if (isAnimating(TRANSITION | CHILDREN)) { return true; } - if (mDeferRemoval) { - removeImmediately(); - } - return super.checkCompleteDeferredRemoval(); + return super.handleCompleteDeferredRemoval(); } public DisplayInfo getDisplayInfo() { @@ -3380,7 +3368,6 @@ class ActivityStack extends Task { mLastNonFullscreenBounds.dumpDebug(proto, LAST_NON_FULLSCREEN_BOUNDS); } - proto.write(DEFER_REMOVAL, mDeferRemoval); proto.write(ANIMATING_BOUNDS, mBoundsAnimating); if (mSurfaceControl != null) { diff --git a/services/core/java/com/android/server/wm/DisplayContent.java b/services/core/java/com/android/server/wm/DisplayContent.java index 241de2e0e068f..c56440785bba5 100644 --- a/services/core/java/com/android/server/wm/DisplayContent.java +++ b/services/core/java/com/android/server/wm/DisplayContent.java @@ -2739,6 +2739,7 @@ class DisplayContent extends WindowContainer= 0; --i) { - final DisplayChildWindowContainer child = getChildAt(i); - stillDeferringRemoval |= child.checkCompleteDeferredRemoval(); - if (getChildCount() == 0) { - // If this display is pending to be removed because it contains an activity with - // {@link ActivityRecord#mIsExiting} is true, this display may be removed when - // completing the removal of the last activity from - // {@link ActivityRecord#checkCompleteDeferredRemoval}. - return false; - } - } + boolean handleCompleteDeferredRemoval() { + final boolean stillDeferringRemoval = super.handleCompleteDeferredRemoval(); if (!stillDeferringRemoval && mDeferredRemoval) { removeImmediately(); return false; } - return true; - } - - /** @return 'true' if removal of this display content is deferred due to active animation. */ - boolean isRemovalDeferred() { - return mDeferredRemoval; + return stillDeferringRemoval; } void adjustForImeIfNeeded() { diff --git a/services/core/java/com/android/server/wm/RootWindowContainer.java b/services/core/java/com/android/server/wm/RootWindowContainer.java index 2d30b737b2746..119b188dfa00c 100644 --- a/services/core/java/com/android/server/wm/RootWindowContainer.java +++ b/services/core/java/com/android/server/wm/RootWindowContainer.java @@ -990,9 +990,7 @@ class RootWindowContainer extends WindowContainer } // Remove all deferred displays stacks, tasks, and activities. - for (int displayNdx = mChildren.size() - 1; displayNdx >= 0; --displayNdx) { - mChildren.get(displayNdx).checkCompleteDeferredRemoval(); - } + handleCompleteDeferredRemoval(); forAllDisplays(dc -> { dc.getInputMonitor().updateInputWindowsLw(true /*force*/); diff --git a/services/core/java/com/android/server/wm/WindowContainer.java b/services/core/java/com/android/server/wm/WindowContainer.java index 2977968f57543..5a73fabaf9d06 100644 --- a/services/core/java/com/android/server/wm/WindowContainer.java +++ b/services/core/java/com/android/server/wm/WindowContainer.java @@ -1044,13 +1044,25 @@ class WindowContainer extends ConfigurationContainer< return mChildren.peekLast(); } - /** Returns true if there is still a removal being deferred */ - boolean checkCompleteDeferredRemoval() { + /** + * Removes the containers which were deferred. + * + * @return {@code true} if there is still a removal being deferred. + */ + boolean handleCompleteDeferredRemoval() { boolean stillDeferringRemoval = false; for (int i = mChildren.size() - 1; i >= 0; --i) { final WindowContainer wc = mChildren.get(i); - stillDeferringRemoval |= wc.checkCompleteDeferredRemoval(); + stillDeferringRemoval |= wc.handleCompleteDeferredRemoval(); + if (!hasChild()) { + // All child containers of current level could be removed from a removal of + // descendant. E.g. if a display is pending to be removed because it contains an + // activity with {@link ActivityRecord#mIsExiting} is true, the display may be + // removed when completing the removal of the last activity from + // {@link ActivityRecord#checkCompleteDeferredRemoval}. + return false; + } } return stillDeferringRemoval; diff --git a/services/tests/wmtests/src/com/android/server/wm/WindowContainerTests.java b/services/tests/wmtests/src/com/android/server/wm/WindowContainerTests.java index 87485eac34121..efc03df877b74 100644 --- a/services/tests/wmtests/src/com/android/server/wm/WindowContainerTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/WindowContainerTests.java @@ -27,6 +27,7 @@ import static android.window.DisplayAreaOrganizer.FEATURE_DEFAULT_TASK_CONTAINER import static com.android.dx.mockito.inline.extended.ExtendedMockito.any; import static com.android.dx.mockito.inline.extended.ExtendedMockito.anyFloat; +import static com.android.dx.mockito.inline.extended.ExtendedMockito.anyInt; import static com.android.dx.mockito.inline.extended.ExtendedMockito.doReturn; import static com.android.dx.mockito.inline.extended.ExtendedMockito.eq; import static com.android.dx.mockito.inline.extended.ExtendedMockito.mock; @@ -824,6 +825,31 @@ public class WindowContainerTests extends WindowTestsBase { assertEquals(newDc, activity.mDisplayContent); } + @Test + public void testHandleCompleteDeferredRemoval() { + final DisplayContent displayContent = createNewDisplay(); + // Do not reparent activity to default display when removing the display. + doReturn(true).when(displayContent).shouldDestroyContentOnRemove(); + final ActivityRecord r = new ActivityTestsBase.StackBuilder(mWm.mRoot) + .setDisplay(displayContent).build().getTopMostActivity(); + // Add a window and make the activity animating so the removal of activity is deferred. + createWindow(null, TYPE_BASE_APPLICATION, r, "win"); + doReturn(true).when(r).isAnimating(anyInt(), anyInt()); + + displayContent.remove(); + // Ensure that ActivityRecord#onRemovedFromDisplay is called. + r.destroyed("test"); + // The removal is deferred, so the activity is still in the display. + assertEquals(r, displayContent.getTopMostActivity()); + + // Assume the animation is done so the deferred removal can continue. + doReturn(false).when(r).isAnimating(anyInt(), anyInt()); + + assertFalse(displayContent.handleCompleteDeferredRemoval()); + assertFalse(displayContent.hasChild()); + assertFalse(r.hasChild()); + } + @Test public void testTaskCanApplyAnimation() { final ActivityStack stack = createTaskStackOnDisplay(mDisplayContent);