From e32aa4fd2b22b5caa75826dc4ec086d1eb7b832e Mon Sep 17 00:00:00 2001 From: Chris Li Date: Tue, 2 Nov 2021 16:36:56 +0800 Subject: [PATCH] Fix NPE for animate frozen snapshot Before, the snapshot can be null if the window is unfreezed but not removed from the display changing list, or when it fails to take screenshot (like because the display is not ready). Now, we make sure to remove the window from the changing list in such cases. Fix: 204400505 Test: atest WmTests:WindowContainerTests Change-Id: I517232fc2ec689680a00b499495a4bad5a0d5f15 --- .../android/server/wm/SurfaceAnimator.java | 4 ++ .../com/android/server/wm/SurfaceFreezer.java | 39 ++++++++++++---- .../android/server/wm/WindowContainer.java | 11 +++-- .../android/server/wm/AppTransitionTests.java | 1 + .../android/server/wm/TaskFragmentTest.java | 1 + .../server/wm/WindowContainerTests.java | 46 +++++++++++++++---- .../android/server/wm/WindowTestsBase.java | 16 +++++++ 7 files changed, 98 insertions(+), 20 deletions(-) diff --git a/services/core/java/com/android/server/wm/SurfaceAnimator.java b/services/core/java/com/android/server/wm/SurfaceAnimator.java index d712bbf0fdef1..50c9b31f425ac 100644 --- a/services/core/java/com/android/server/wm/SurfaceAnimator.java +++ b/services/core/java/com/android/server/wm/SurfaceAnimator.java @@ -188,6 +188,10 @@ class SurfaceAnimator { mAnimation.startAnimation(mLeash, t, type, mInnerAnimationFinishedCallback); if (snapshotAnim != null) { mSnapshot = freezer.takeSnapshotForAnimation(); + if (mSnapshot == null) { + Slog.e(TAG, "No snapshot target to start animation on for " + mAnimatable); + return; + } mSnapshot.startAnimation(t, snapshotAnim, type); } } diff --git a/services/core/java/com/android/server/wm/SurfaceFreezer.java b/services/core/java/com/android/server/wm/SurfaceFreezer.java index 233656ffd48d0..a7ef36b01d91a 100644 --- a/services/core/java/com/android/server/wm/SurfaceFreezer.java +++ b/services/core/java/com/android/server/wm/SurfaceFreezer.java @@ -26,6 +26,7 @@ import android.graphics.PixelFormat; import android.graphics.Point; import android.graphics.Rect; import android.hardware.HardwareBuffer; +import android.util.Slog; import android.view.SurfaceControl; import com.android.internal.annotations.VisibleForTesting; @@ -49,8 +50,10 @@ import com.android.internal.protolog.common.ProtoLog; */ class SurfaceFreezer { - private final Freezable mAnimatable; - private final WindowManagerService mWmService; + private static final String TAG = "SurfaceFreezer"; + + private final @NonNull Freezable mAnimatable; + private final @NonNull WindowManagerService mWmService; @VisibleForTesting SurfaceControl mLeash; Snapshot mSnapshot = null; @@ -59,7 +62,7 @@ class SurfaceFreezer { /** * @param animatable The object to animate. */ - SurfaceFreezer(Freezable animatable, WindowManagerService service) { + SurfaceFreezer(@NonNull Freezable animatable, @NonNull WindowManagerService service) { mAnimatable = animatable; mWmService = service; } @@ -86,11 +89,14 @@ class SurfaceFreezer { freezeTarget = freezeTarget != null ? freezeTarget : mAnimatable.getFreezeSnapshotTarget(); if (freezeTarget != null) { - SurfaceControl.ScreenshotHardwareBuffer screenshotBuffer = createSnapshotBuffer( + SurfaceControl.ScreenshotHardwareBuffer screenshotBuffer = createSnapshotBufferInner( freezeTarget, startBounds); final HardwareBuffer buffer = screenshotBuffer == null ? null : screenshotBuffer.getHardwareBuffer(); if (buffer == null || buffer.getWidth() <= 1 || buffer.getHeight() <= 1) { + // This can happen when display is not ready. + Slog.w(TAG, "Failed to capture screenshot for " + mAnimatable); + unfreeze(t); return; } mSnapshot = new Snapshot(t, screenshotBuffer, mLeash); @@ -124,6 +130,11 @@ class SurfaceFreezer { * snapshot. */ void unfreeze(SurfaceControl.Transaction t) { + unfreezeInner(t); + mAnimatable.onUnfrozen(); + } + + private void unfreezeInner(SurfaceControl.Transaction t) { if (mSnapshot != null) { mSnapshot.cancelAnimation(t, false /* restarting */); mSnapshot = null; @@ -188,6 +199,18 @@ class SurfaceFreezer { return SurfaceControl.captureLayers(captureArgs); } + @VisibleForTesting + SurfaceControl.ScreenshotHardwareBuffer createSnapshotBufferInner( + SurfaceControl target, Rect bounds) { + return createSnapshotBuffer(target, bounds); + } + + @VisibleForTesting + GraphicBuffer createFromHardwareBufferInner( + SurfaceControl.ScreenshotHardwareBuffer screenshotBuffer) { + return GraphicBuffer.createFromHardwareBuffer(screenshotBuffer.getHardwareBuffer()); + } + class Snapshot { private SurfaceControl mSurfaceControl; private AnimationAdapter mAnimation; @@ -198,10 +221,7 @@ class SurfaceFreezer { */ Snapshot(SurfaceControl.Transaction t, SurfaceControl.ScreenshotHardwareBuffer screenshotBuffer, SurfaceControl parent) { - // We can't use a delegating constructor since we need to - // reference this::onAnimationFinished - GraphicBuffer graphicBuffer = GraphicBuffer.createFromHardwareBuffer( - screenshotBuffer.getHardwareBuffer()); + GraphicBuffer graphicBuffer = createFromHardwareBufferInner(screenshotBuffer); mSurfaceControl = mAnimatable.makeAnimationLeash() .setName("snapshot anim: " + mAnimatable.toString()) @@ -275,5 +295,8 @@ class SurfaceFreezer { * will be generated (but the rest of the freezing logic will still happen). */ @Nullable SurfaceControl getFreezeSnapshotTarget(); + + /** Called when the {@link #unfreeze(SurfaceControl.Transaction)} is called. */ + void onUnfrozen(); } } diff --git a/services/core/java/com/android/server/wm/WindowContainer.java b/services/core/java/com/android/server/wm/WindowContainer.java index 7f6b493fc520a..58bc244e92508 100644 --- a/services/core/java/com/android/server/wm/WindowContainer.java +++ b/services/core/java/com/android/server/wm/WindowContainer.java @@ -614,7 +614,6 @@ class WindowContainer extends ConfigurationContainer< final DisplayContent dc = getDisplayContent(); if (dc != null) { mSurfaceFreezer.unfreeze(getSyncTransaction()); - dc.mChangingContainers.remove(this); } while (!mChildren.isEmpty()) { final E child = mChildren.peekLast(); @@ -1067,9 +1066,6 @@ class WindowContainer extends ConfigurationContainer< // part of a change transition. if (!visible) { mSurfaceFreezer.unfreeze(getSyncTransaction()); - if (mDisplayContent != null) { - mDisplayContent.mChangingContainers.remove(this); - } } WindowContainer parent = getParent(); if (parent != null) { @@ -2647,6 +2643,13 @@ class WindowContainer extends ConfigurationContainer< return getSurfaceControl(); } + @Override + public void onUnfrozen() { + if (mDisplayContent != null) { + mDisplayContent.mChangingContainers.remove(this); + } + } + @Override public Builder makeAnimationLeash() { return makeSurface().setContainerLayer(); diff --git a/services/tests/wmtests/src/com/android/server/wm/AppTransitionTests.java b/services/tests/wmtests/src/com/android/server/wm/AppTransitionTests.java index fb8bc7be38cea..c0959d311ed56 100644 --- a/services/tests/wmtests/src/com/android/server/wm/AppTransitionTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/AppTransitionTests.java @@ -419,6 +419,7 @@ public class AppTransitionTests extends WindowTestsBase { task.getBounds(taskBounds); taskFragment.setBounds(0, 0, taskBounds.right / 2, taskBounds.bottom); spyOn(taskFragment); + mockSurfaceFreezerSnapshot(taskFragment.mSurfaceFreezer); assertTrue(mDc.mChangingContainers.isEmpty()); assertFalse(mDc.mAppTransition.isTransitionSet()); diff --git a/services/tests/wmtests/src/com/android/server/wm/TaskFragmentTest.java b/services/tests/wmtests/src/com/android/server/wm/TaskFragmentTest.java index cb209abf6aa9a..42f4d583f5ff7 100644 --- a/services/tests/wmtests/src/com/android/server/wm/TaskFragmentTest.java +++ b/services/tests/wmtests/src/com/android/server/wm/TaskFragmentTest.java @@ -82,6 +82,7 @@ public class TaskFragmentTest extends WindowTestsBase { @Test public void testStartChangeTransition_resetSurface() { + mockSurfaceFreezerSnapshot(mTaskFragment.mSurfaceFreezer); final Rect startBounds = new Rect(0, 0, 1000, 1000); final Rect endBounds = new Rect(500, 500, 1000, 1000); mTaskFragment.setBounds(startBounds); 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 e75a2ce7f8296..6ae3f9447ee2f 100644 --- a/services/tests/wmtests/src/com/android/server/wm/WindowContainerTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/WindowContainerTests.java @@ -1118,16 +1118,15 @@ public class WindowContainerTests extends WindowTestsBase { final SurfaceControl.Transaction t = mock(SurfaceControl.Transaction.class); spyOn(container); spyOn(surfaceAnimator); - spyOn(surfaceFreezer); + mockSurfaceFreezerSnapshot(surfaceFreezer); doReturn(t).when(container).getPendingTransaction(); doReturn(t).when(container).getSyncTransaction(); // Leash and snapshot created for change transition. container.initializeChangeTransition(new Rect(0, 0, 1000, 2000)); - // Can't really take a snapshot, manually set one. - surfaceFreezer.mSnapshot = mock(SurfaceFreezer.Snapshot.class); assertNotNull(surfaceFreezer.mLeash); + assertNotNull(surfaceFreezer.mSnapshot); assertEquals(surfaceFreezer.mLeash, container.getAnimationLeash()); // Start animation: surfaceAnimator take over the leash and snapshot from surfaceFreezer. @@ -1144,9 +1143,9 @@ public class WindowContainerTests extends WindowTestsBase { // Prepare another change transition. container.initializeChangeTransition(new Rect(0, 0, 1000, 2000)); - surfaceFreezer.mSnapshot = mock(SurfaceFreezer.Snapshot.class); assertNotNull(surfaceFreezer.mLeash); + assertNotNull(surfaceFreezer.mSnapshot); assertEquals(surfaceFreezer.mLeash, container.getAnimationLeash()); assertNotEquals(prevLeash, container.getAnimationLeash()); @@ -1173,12 +1172,43 @@ public class WindowContainerTests extends WindowTestsBase { assertNull(surfaceAnimator.mSnapshot); } + @Test + public void testUnfreezeWindow_removeWindowFromChanging() { + final WindowContainer container = createTaskFragmentWithParentTask( + createTask(mDisplayContent), false); + mockSurfaceFreezerSnapshot(container.mSurfaceFreezer); + final SurfaceControl.Transaction t = mock(SurfaceControl.Transaction.class); + + container.initializeChangeTransition(new Rect(0, 0, 1000, 2000)); + + assertTrue(mDisplayContent.mChangingContainers.contains(container)); + + container.mSurfaceFreezer.unfreeze(t); + + assertFalse(mDisplayContent.mChangingContainers.contains(container)); + } + + @Test + public void testFailToTaskSnapshot_unfreezeWindow() { + final WindowContainer container = createTaskFragmentWithParentTask( + createTask(mDisplayContent), false); + final SurfaceControl.Transaction t = mock(SurfaceControl.Transaction.class); + spyOn(container.mSurfaceFreezer); + + container.initializeChangeTransition(new Rect(0, 0, 1000, 2000)); + + verify(container.mSurfaceFreezer).freeze(any(), any(), any(), any()); + verify(container.mSurfaceFreezer).unfreeze(any()); + assertTrue(mDisplayContent.mChangingContainers.isEmpty()); + } + @Test public void testRemoveUnstartedFreezeSurfaceWhenFreezeAgain() { final WindowContainer container = createTaskFragmentWithParentTask( createTask(mDisplayContent), false); container.mSurfaceControl = mock(SurfaceControl.class); final SurfaceFreezer surfaceFreezer = container.mSurfaceFreezer; + mockSurfaceFreezerSnapshot(surfaceFreezer); final SurfaceControl.Transaction t = mock(SurfaceControl.Transaction.class); spyOn(container); doReturn(t).when(container).getPendingTransaction(); @@ -1188,16 +1218,16 @@ public class WindowContainerTests extends WindowTestsBase { container.initializeChangeTransition(new Rect(0, 0, 1000, 2000)); assertNotNull(surfaceFreezer.mLeash); + assertNotNull(surfaceFreezer.mSnapshot); - // Can't really take a snapshot, manually set one. - final SurfaceFreezer.Snapshot snapshot = mock(SurfaceFreezer.Snapshot.class); - surfaceFreezer.mSnapshot = snapshot; final SurfaceControl prevLeash = surfaceFreezer.mLeash; + final SurfaceFreezer.Snapshot prevSnapshot = surfaceFreezer.mSnapshot; + spyOn(prevSnapshot); container.initializeChangeTransition(new Rect(0, 0, 1500, 2500)); verify(t).remove(prevLeash); - verify(snapshot).destroy(t); + verify(prevSnapshot).destroy(t); } /* Used so we can gain access to some protected members of the {@link WindowContainer} class */ diff --git a/services/tests/wmtests/src/com/android/server/wm/WindowTestsBase.java b/services/tests/wmtests/src/com/android/server/wm/WindowTestsBase.java index 115f8a3fecf43..2a0aa96a00ee1 100644 --- a/services/tests/wmtests/src/com/android/server/wm/WindowTestsBase.java +++ b/services/tests/wmtests/src/com/android/server/wm/WindowTestsBase.java @@ -77,6 +77,7 @@ import android.content.Context; import android.content.Intent; import android.content.pm.ActivityInfo; import android.content.pm.ApplicationInfo; +import android.hardware.HardwareBuffer; import android.hardware.display.DisplayManager; import android.os.Build; import android.os.Bundle; @@ -878,6 +879,21 @@ class WindowTestsBase extends SystemServiceTestsBase { mAtm.mRootWindowContainer.mDefaultMinSizeOfResizeableTaskDp = 1; } + /** Mocks the behavior of taking a snapshot. */ + void mockSurfaceFreezerSnapshot(SurfaceFreezer surfaceFreezer) { + final SurfaceControl.ScreenshotHardwareBuffer screenshotBuffer = + mock(SurfaceControl.ScreenshotHardwareBuffer.class); + final HardwareBuffer hardwareBuffer = mock(HardwareBuffer.class); + spyOn(surfaceFreezer); + doReturn(screenshotBuffer).when(surfaceFreezer) + .createSnapshotBufferInner(any(), any()); + doReturn(null).when(surfaceFreezer) + .createFromHardwareBufferInner(any()); + doReturn(hardwareBuffer).when(screenshotBuffer).getHardwareBuffer(); + doReturn(100).when(hardwareBuffer).getWidth(); + doReturn(100).when(hardwareBuffer).getHeight(); + } + /** * Builder for creating new activities. */