From d95f73d9a3662bad36c862546a97699059127598 Mon Sep 17 00:00:00 2001 From: Riddle Hsu Date: Tue, 7 Jun 2022 18:55:21 +0800 Subject: [PATCH] Avoid recursive destruction from exit animation done All WindowState belonging to an activity have non-null mActivityRecord. The original code may attempt to destroy parent surface from a child window's onExitAnimationDone(). Then it could have a case: removing parent -> remove child -> remove parent. The case is prevented by not destroying all surfaces under the activity if the animated window is not the main window of activity. Also move the check of mRemoved to avoid duplicated invocation of super.removeImmediately(). Assume the hierarchy: Activity - W (activity window) - X (dialog window) - Y (popup window) Y is IME layer target. The steps in WindowState: X onAnimationFinished(onExitAnimationDone) X destroySurface X removeImmediately Y removeImmediately (*) Y cancelAnimation Y onAnimationFinished(onExitAnimationDone) X destroySurface X removeImmediately CRASH in imeTarget.compareTo(this) (from needsRelativeLayeringToIme()) Because (*) hasn't reached setImeLayeringTarget(null) yet, Y is still imeTarget but its parent is cleared. With this change: X onAnimationFinished(onExitAnimationDone) X destroySurface X removeImmediately Y removeImmediately Y cancelAnimation Y onAnimationFinished(onExitAnimationDone) [NO destroySurface X] <- The exact fix Y destroySurface Y removeImmediately (early return) Fix: 232092201 Test: atest WindowStateTests#testOnExitAnimationDone Change-Id: I91781fa79dfdf414108803fac714930a20385780 --- .../com/android/server/wm/WindowState.java | 19 ++++----- .../android/server/wm/WindowStateTests.java | 39 +++++++++++++++++++ 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/services/core/java/com/android/server/wm/WindowState.java b/services/core/java/com/android/server/wm/WindowState.java index 1abe24e926fe1..3bb40b03a0a56 100644 --- a/services/core/java/com/android/server/wm/WindowState.java +++ b/services/core/java/com/android/server/wm/WindowState.java @@ -2433,14 +2433,6 @@ class WindowState extends WindowContainer implements WindowManagerP @Override void removeImmediately() { - if (!mRemoved) { - // Destroy surface before super call. The general pattern is that the children need - // to be removed before the parent (so that the sync-engine tracking works). Since - // WindowStateAnimator is a "virtual" child, we have to do it manually here. - mWinAnimator.destroySurfaceLocked(getSyncTransaction()); - } - super.removeImmediately(); - if (mRemoved) { // Nothing to do. ProtoLog.v(WM_DEBUG_ADD_REMOVE, @@ -2449,6 +2441,11 @@ class WindowState extends WindowContainer implements WindowManagerP } mRemoved = true; + // Destroy surface before super call. The general pattern is that the children need + // to be removed before the parent (so that the sync-engine tracking works). Since + // WindowStateAnimator is a "virtual" child, we have to do it manually here. + mWinAnimator.destroySurfaceLocked(getSyncTransaction()); + super.removeImmediately(); mWillReplaceWindow = false; if (mReplacementWindow != null) { @@ -5090,7 +5087,11 @@ class WindowState extends WindowContainer implements WindowManagerP // Otherwise we add the service to mDestroySurface and allow it to be processed in our next // transaction. if (mActivityRecord != null) { - mActivityRecord.destroySurfaces(); + if (mAttrs.type == TYPE_BASE_APPLICATION) { + mActivityRecord.destroySurfaces(); + } else { + destroySurface(false /* cleanupOnResume */, mActivityRecord.mAppStopped); + } } else { if (hasSurface) { mWmService.mDestroySurface.add(this); diff --git a/services/tests/wmtests/src/com/android/server/wm/WindowStateTests.java b/services/tests/wmtests/src/com/android/server/wm/WindowStateTests.java index 724204f79eabe..e8f343e73256f 100644 --- a/services/tests/wmtests/src/com/android/server/wm/WindowStateTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/WindowStateTests.java @@ -74,6 +74,7 @@ import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.atMost; import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.when; @@ -471,6 +472,44 @@ public class WindowStateTests extends WindowTestsBase { assertTrue(appWindow.mRemoved); } + @Test + public void testOnExitAnimationDone() { + final WindowState parent = createWindow(null, TYPE_APPLICATION, "parent"); + final WindowState child = createWindow(parent, TYPE_APPLICATION_PANEL, "child"); + final SurfaceControl.Transaction t = parent.getPendingTransaction(); + child.startAnimation(t, mock(AnimationAdapter.class), false /* hidden */, + SurfaceAnimator.ANIMATION_TYPE_WINDOW_ANIMATION); + parent.mAnimatingExit = parent.mRemoveOnExit = parent.mWindowRemovalAllowed = true; + child.mAnimatingExit = child.mRemoveOnExit = child.mWindowRemovalAllowed = true; + final int[] numRemovals = new int[2]; + parent.registerWindowContainerListener(new WindowContainerListener() { + @Override + public void onRemoved() { + numRemovals[0]++; + } + }); + child.registerWindowContainerListener(new WindowContainerListener() { + @Override + public void onRemoved() { + numRemovals[1]++; + } + }); + spyOn(parent); + // parent onExitAnimationDone + // -> child onExitAnimationDone() -> no-op because isAnimating() + // -> parent destroySurface() + // -> parent removeImmediately() because mDestroying+mRemoveOnExit + // -> child removeImmediately() -> cancelAnimation() + // -> child onExitAnimationDone() + // -> child destroySurface() because animation is canceled + // -> child removeImmediately() -> no-op because mRemoved + parent.onExitAnimationDone(); + // There must be no additional destroySurface() of parent from its child. + verify(parent, atMost(1)).destroySurface(anyBoolean(), anyBoolean()); + assertEquals(1, numRemovals[0]); + assertEquals(1, numRemovals[1]); + } + @Test public void testLayoutSeqResetOnReparent() { final WindowState app = createWindow(null, TYPE_APPLICATION, "app");