Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit d95f73d9 authored by Riddle Hsu's avatar Riddle Hsu
Browse files

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
parent 0cbdd275

services/core/java/com/android/server/wm/WindowState.java

+10 −9
Original line number Diff line number Diff line
@@ -2433,14 +2433,6 @@ class WindowState extends WindowContainer<WindowState> 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<WindowState> 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<WindowState> implements WindowManagerP 
        // Otherwise we add the service to mDestroySurface and allow it to be processed in our next

        // transaction.

        if (mActivityRecord != null) {

            if (mAttrs.type == TYPE_BASE_APPLICATION) {

                mActivityRecord.destroySurfaces();

            } else {

                destroySurface(false /* cleanupOnResume */, mActivityRecord.mAppStopped);

            }

        } else {

            if (hasSurface) {

                mWmService.mDestroySurface.add(this);

services/tests/wmtests/src/com/android/server/wm/WindowStateTests.java

+39 −0
Original line number Diff line number Diff line
@@ -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");