diff options
author | Winson Chung <winsonc@google.com> | 2023-01-06 08:38:40 +0000 |
---|---|---|
committer | Android Build Coastguard Worker <android-build-coastguard-worker@google.com> | 2023-01-19 17:05:51 +0000 |
commit | 31817f0abeee786363dce3d13006c9dcd831475a (patch) | |
tree | 4abf9f18b73310c610fa4139d81edc7edce97c06 | |
parent | d002331a272950a19f68e136ee06b1ab6870fb9a (diff) | |
download | base-31817f0abeee786363dce3d13006c9dcd831475a.tar.gz |
Fix issue with leaking death recipients
- When invalidating an existing external interface, we also need to
unregister any listeners to ensure that binder death recipients are
unlinked. Otherwise, a crash in the process for the old listener
can cause the current listener to be unregistered.
Bug: 242377577
Test: atest WMShellUnitTests
Change-Id: Ibb77ac19e4ebc616b8977db70d6269e3b930848e
(cherry picked from commit 9a81477e866ad1837cc7fdbaad80b15cdbafc936)
Merged-In: Ibb77ac19e4ebc616b8977db70d6269e3b930848e
10 files changed, 130 insertions, 18 deletions
diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/common/SingleInstanceRemoteListener.java b/libs/WindowManager/Shell/src/com/android/wm/shell/common/SingleInstanceRemoteListener.java index b77ac8a2b951..e46ee28b3ddb 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/common/SingleInstanceRemoteListener.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/common/SingleInstanceRemoteListener.java @@ -29,6 +29,9 @@ import java.util.function.Consumer; * Manages the lifecycle of a single instance of a remote listener, including the clean up if the * remote process dies. All calls on this class should happen on the main shell thread. * + * Any external interface using this listener should also unregister the listener when it is + * invalidated, otherwise it may leak binder death recipients. + * * @param <C> The controller (must be RemoteCallable) * @param <L> The remote listener interface type */ diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/pip/phone/PipController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/pip/phone/PipController.java index efe938f0a274..01d81ff4e436 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/pip/phone/PipController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/pip/phone/PipController.java @@ -207,7 +207,8 @@ public class PipController implements PipTransitionController.PipTransitionCallb private Consumer<Boolean> mOnIsInPipStateChangedListener; - private interface PipAnimationListener { + @VisibleForTesting + interface PipAnimationListener { /** * Notifies the listener that the Pip animation is started. */ @@ -434,11 +435,7 @@ public class PipController implements PipTransitionController.PipTransitionCallb Optional<OneHandedController> oneHandedController, ShellExecutor mainExecutor ) { - // Ensure that we are the primary user's SystemUI. - final int processUser = UserManager.get(context).getProcessUserId(); - if (processUser != UserHandle.USER_SYSTEM) { - throw new IllegalStateException("Non-primary Pip component not currently supported."); - } + mContext = context; mShellCommandHandler = shellCommandHandler; @@ -872,11 +869,17 @@ public class PipController implements PipTransitionController.PipTransitionCallb animationType == PipAnimationController.ANIM_TYPE_BOUNDS); } - private void setPinnedStackAnimationListener(PipAnimationListener callback) { + @VisibleForTesting + void setPinnedStackAnimationListener(PipAnimationListener callback) { mPinnedStackAnimationRecentsCallback = callback; onPipResourceDimensionsChanged(); } + @VisibleForTesting + boolean hasPinnedStackAnimationListener() { + return mPinnedStackAnimationRecentsCallback != null; + } + private void onPipResourceDimensionsChanged() { if (mPinnedStackAnimationRecentsCallback != null) { mPinnedStackAnimationRecentsCallback.onPipResourceDimensionsChanged( @@ -1166,6 +1169,8 @@ public class PipController implements PipTransitionController.PipTransitionCallb @Override public void invalidate() { mController = null; + // Unregister the listener to ensure any registered binder death recipients are unlinked + mListener.unregister(); } @Override diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/recents/RecentTasksController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/recents/RecentTasksController.java index f9172ba183de..502e559a8e0a 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/recents/RecentTasksController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/recents/RecentTasksController.java @@ -280,15 +280,22 @@ public class RecentTasksController implements TaskStackListenerCallback, } } - private void registerRecentTasksListener(IRecentTasksListener listener) { + @VisibleForTesting + void registerRecentTasksListener(IRecentTasksListener listener) { mListener = listener; } - private void unregisterRecentTasksListener() { + @VisibleForTesting + void unregisterRecentTasksListener() { mListener = null; } @VisibleForTesting + boolean hasRecentTasksListener() { + return mListener != null; + } + + @VisibleForTesting ArrayList<GroupedRecentTaskInfo> getRecentTasks(int maxNum, int flags, int userId) { // Note: the returned task list is from the most-recent to least-recent order final List<ActivityManager.RecentTaskInfo> rawList = mActivityTaskManager.getRecentTasks( @@ -430,6 +437,8 @@ public class RecentTasksController implements TaskStackListenerCallback, @Override public void invalidate() { mController = null; + // Unregister the listener to ensure any registered binder death recipients are unlinked + mListener.unregister(); } @Override diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/splitscreen/SplitScreenController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/splitscreen/SplitScreenController.java index a79ac45228ca..5bbc42fb75b9 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/splitscreen/SplitScreenController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/splitscreen/SplitScreenController.java @@ -904,6 +904,8 @@ public class SplitScreenController implements DragAndDropPolicy.Starter, @Override public void invalidate() { mController = null; + // Unregister the listener to ensure any registered binder death recipients are unlinked + mListener.unregister(); } @Override diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/startingsurface/StartingWindowController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/startingsurface/StartingWindowController.java index 0c23f109feaf..be2e79342d07 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/startingsurface/StartingWindowController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/startingsurface/StartingWindowController.java @@ -39,6 +39,7 @@ import android.window.TaskOrganizer; import android.window.TaskSnapshot; import androidx.annotation.BinderThread; +import androidx.annotation.VisibleForTesting; import com.android.internal.annotations.GuardedBy; import com.android.internal.util.function.TriConsumer; @@ -138,10 +139,16 @@ public class StartingWindowController implements RemoteCallable<StartingWindowCo * * @param listener The callback when need a starting window. */ + @VisibleForTesting void setStartingWindowListener(TriConsumer<Integer, Integer, Integer> listener) { mTaskLaunchingCallback = listener; } + @VisibleForTesting + boolean hasStartingWindowListener() { + return mTaskLaunchingCallback != null; + } + /** * Called when a task need a starting window. */ @@ -281,6 +288,8 @@ public class StartingWindowController implements RemoteCallable<StartingWindowCo @Override public void invalidate() { mController = null; + // Unregister the listener to ensure any registered binder death recipients are unlinked + mListener.unregister(); } @Override diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/sysui/ShellController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/sysui/ShellController.java index fdf073f0bf26..3f944cb6d628 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/sysui/ShellController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/sysui/ShellController.java @@ -164,7 +164,8 @@ public class ShellController { * Updates the given bundle with the set of external interfaces, invalidating the old set of * binders. */ - private void createExternalInterfaces(Bundle output) { + @VisibleForTesting + public void createExternalInterfaces(Bundle output) { // Invalidate the old binders for (int i = 0; i < mExternalInterfaces.size(); i++) { mExternalInterfaces.valueAt(i).invalidate(); diff --git a/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/pip/phone/PipControllerTest.java b/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/pip/phone/PipControllerTest.java index d06fb55a5769..c6bf55e8e31a 100644 --- a/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/pip/phone/PipControllerTest.java +++ b/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/pip/phone/PipControllerTest.java @@ -18,7 +18,9 @@ package com.android.wm.shell.pip.phone; import static android.content.pm.PackageManager.FEATURE_PICTURE_IN_PICTURE; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; @@ -35,6 +37,7 @@ import android.content.Context; import android.content.pm.PackageManager; import android.content.res.Configuration; import android.graphics.Rect; +import android.os.Bundle; import android.os.RemoteException; import android.test.suitebuilder.annotation.SmallTest; import android.testing.AndroidTestingRunner; @@ -59,6 +62,7 @@ import com.android.wm.shell.pip.PipSnapAlgorithm; import com.android.wm.shell.pip.PipTaskOrganizer; import com.android.wm.shell.pip.PipTransitionController; import com.android.wm.shell.pip.PipTransitionState; +import com.android.wm.shell.recents.IRecentTasksListener; import com.android.wm.shell.sysui.ShellCommandHandler; import com.android.wm.shell.sysui.ShellController; import com.android.wm.shell.sysui.ShellInit; @@ -185,6 +189,24 @@ public class PipControllerTest extends ShellTestCase { } @Test + public void testInvalidateExternalInterface_unregistersListener() { + mPipController.setPinnedStackAnimationListener(new PipController.PipAnimationListener() { + @Override + public void onPipAnimationStarted() {} + @Override + public void onPipResourceDimensionsChanged(int cornerRadius, int shadowRadius) {} + @Override + public void onExpandPip() {} + }); + assertTrue(mPipController.hasPinnedStackAnimationListener()); + // Create initial interface + mShellController.createExternalInterfaces(new Bundle()); + // Recreate the interface to trigger invalidation of the previous instance + mShellController.createExternalInterfaces(new Bundle()); + assertFalse(mPipController.hasPinnedStackAnimationListener()); + } + + @Test public void createPip_notSupported_returnsNull() { Context spyContext = spy(mContext); PackageManager mockPackageManager = mock(PackageManager.class); diff --git a/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/recents/RecentTasksControllerTest.java b/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/recents/RecentTasksControllerTest.java index f6ac3ee0a8e4..82392ad9a3eb 100644 --- a/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/recents/RecentTasksControllerTest.java +++ b/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/recents/RecentTasksControllerTest.java @@ -23,6 +23,7 @@ import static android.app.WindowConfiguration.WINDOWING_MODE_MULTI_WINDOW; import static com.android.dx.mockito.inline.extended.ExtendedMockito.mockitoSession; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -45,6 +46,7 @@ import android.app.ActivityTaskManager; import android.content.Context; import android.content.pm.PackageManager; import android.graphics.Rect; +import android.os.Bundle; import android.view.SurfaceControl; import androidx.test.filters.SmallTest; @@ -87,8 +89,6 @@ public class RecentTasksControllerTest extends ShellTestCase { @Mock private TaskStackListenerImpl mTaskStackListener; @Mock - private ShellController mShellController; - @Mock private ShellCommandHandler mShellCommandHandler; @Mock private DesktopModeTaskRepository mDesktopModeTaskRepository; @@ -97,7 +97,9 @@ public class RecentTasksControllerTest extends ShellTestCase { private ShellTaskOrganizer mShellTaskOrganizer; private RecentTasksController mRecentTasksController; + private RecentTasksController mRecentTasksControllerReal; private ShellInit mShellInit; + private ShellController mShellController; private TestShellExecutor mMainExecutor; @Before @@ -105,9 +107,12 @@ public class RecentTasksControllerTest extends ShellTestCase { mMainExecutor = new TestShellExecutor(); when(mContext.getPackageManager()).thenReturn(mock(PackageManager.class)); mShellInit = spy(new ShellInit(mMainExecutor)); - mRecentTasksController = spy(new RecentTasksController(mContext, mShellInit, + mShellController = spy(new ShellController(mShellInit, mShellCommandHandler, + mMainExecutor)); + mRecentTasksControllerReal = new RecentTasksController(mContext, mShellInit, mShellController, mShellCommandHandler, mTaskStackListener, mActivityTaskManager, - Optional.of(mDesktopModeTaskRepository), mMainExecutor)); + Optional.of(mDesktopModeTaskRepository), mMainExecutor); + mRecentTasksController = spy(mRecentTasksControllerReal); mShellTaskOrganizer = new ShellTaskOrganizer(mShellInit, mShellCommandHandler, null /* sizeCompatUI */, Optional.empty(), Optional.of(mRecentTasksController), mMainExecutor); @@ -132,6 +137,20 @@ public class RecentTasksControllerTest extends ShellTestCase { } @Test + public void testInvalidateExternalInterface_unregistersListener() { + // Note: We have to use the real instance of the controller here since that is the instance + // that is passed to ShellController internally, and the instance that the listener will be + // unregistered from + mRecentTasksControllerReal.registerRecentTasksListener(new IRecentTasksListener.Default()); + assertTrue(mRecentTasksControllerReal.hasRecentTasksListener()); + // Create initial interface + mShellController.createExternalInterfaces(new Bundle()); + // Recreate the interface to trigger invalidation of the previous instance + mShellController.createExternalInterfaces(new Bundle()); + assertFalse(mRecentTasksControllerReal.hasRecentTasksListener()); + } + + @Test public void testAddRemoveSplitNotifyChange() { ActivityManager.RecentTaskInfo t1 = makeTaskInfo(1); ActivityManager.RecentTaskInfo t2 = makeTaskInfo(2); diff --git a/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/splitscreen/SplitScreenControllerTests.java b/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/splitscreen/SplitScreenControllerTests.java index 38b75f81171f..259b410f8a68 100644 --- a/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/splitscreen/SplitScreenControllerTests.java +++ b/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/splitscreen/SplitScreenControllerTests.java @@ -27,11 +27,14 @@ import static com.android.wm.shell.common.split.SplitScreenConstants.SPLIT_POSIT import static com.android.wm.shell.common.split.SplitScreenConstants.SPLIT_POSITION_TOP_OR_LEFT; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.junit.Assume.assumeTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isA; import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; @@ -46,6 +49,7 @@ import android.app.PendingIntent; import android.content.ComponentName; import android.content.Intent; import android.content.pm.ActivityInfo; +import android.os.Bundle; import androidx.test.annotation.UiThreadTest; import androidx.test.ext.junit.runners.AndroidJUnit4; @@ -86,7 +90,6 @@ import org.mockito.MockitoAnnotations; public class SplitScreenControllerTests extends ShellTestCase { @Mock ShellInit mShellInit; - @Mock ShellController mShellController; @Mock ShellCommandHandler mShellCommandHandler; @Mock ShellTaskOrganizer mTaskOrganizer; @Mock SyncTransactionQueue mSyncQueue; @@ -103,12 +106,15 @@ public class SplitScreenControllerTests extends ShellTestCase { @Mock RecentTasksController mRecentTasks; @Captor ArgumentCaptor<Intent> mIntentCaptor; + private ShellController mShellController; private SplitScreenController mSplitScreenController; @Before public void setup() { assumeTrue(ActivityTaskManager.supportsSplitScreenMultiWindow(mContext)); MockitoAnnotations.initMocks(this); + mShellController = spy(new ShellController(mShellInit, mShellCommandHandler, + mMainExecutor)); mSplitScreenController = spy(new SplitScreenController(mContext, mShellInit, mShellCommandHandler, mShellController, mTaskOrganizer, mSyncQueue, mRootTDAOrganizer, mDisplayController, mDisplayImeController, @@ -118,7 +124,7 @@ public class SplitScreenControllerTests extends ShellTestCase { @Test public void instantiateController_addInitCallback() { - verify(mShellInit, times(1)).addInitCallback(any(), any()); + verify(mShellInit, times(1)).addInitCallback(any(), isA(SplitScreenController.class)); } @Test @@ -159,6 +165,19 @@ public class SplitScreenControllerTests extends ShellTestCase { } @Test + public void testInvalidateExternalInterface_unregistersListener() { + mSplitScreenController.onInit(); + mSplitScreenController.registerSplitScreenListener( + new SplitScreen.SplitScreenListener() {}); + verify(mStageCoordinator).registerSplitScreenListener(any()); + // Create initial interface + mShellController.createExternalInterfaces(new Bundle()); + // Recreate the interface to trigger invalidation of the previous instance + mShellController.createExternalInterfaces(new Bundle()); + verify(mStageCoordinator).unregisterSplitScreenListener(any()); + } + + @Test public void testStartIntent_appendsNoUserActionFlag() { Intent startIntent = createStartIntent("startActivity"); PendingIntent pendingIntent = diff --git a/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/startingsurface/StartingWindowControllerTests.java b/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/startingsurface/StartingWindowControllerTests.java index 90165d1cd1b2..10dec9ef12f9 100644 --- a/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/startingsurface/StartingWindowControllerTests.java +++ b/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/startingsurface/StartingWindowControllerTests.java @@ -16,9 +16,12 @@ package com.android.wm.shell.startingsurface; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isA; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; @@ -27,16 +30,19 @@ import static org.mockito.Mockito.verify; import android.content.Context; import android.hardware.display.DisplayManager; +import android.os.Bundle; import android.view.Display; import androidx.test.ext.junit.runners.AndroidJUnit4; import androidx.test.filters.SmallTest; +import com.android.internal.util.function.TriConsumer; import com.android.launcher3.icons.IconProvider; import com.android.wm.shell.ShellTaskOrganizer; import com.android.wm.shell.ShellTestCase; import com.android.wm.shell.common.ShellExecutor; import com.android.wm.shell.common.TransactionPool; +import com.android.wm.shell.sysui.ShellCommandHandler; import com.android.wm.shell.sysui.ShellController; import com.android.wm.shell.sysui.ShellInit; import com.android.wm.shell.sysui.ShellSharedConstants; @@ -59,7 +65,7 @@ public class StartingWindowControllerTests extends ShellTestCase { private @Mock Context mContext; private @Mock DisplayManager mDisplayManager; - private @Mock ShellController mShellController; + private @Mock ShellCommandHandler mShellCommandHandler; private @Mock ShellTaskOrganizer mTaskOrganizer; private @Mock ShellExecutor mMainExecutor; private @Mock StartingWindowTypeAlgorithm mTypeAlgorithm; @@ -67,6 +73,7 @@ public class StartingWindowControllerTests extends ShellTestCase { private @Mock TransactionPool mTransactionPool; private StartingWindowController mController; private ShellInit mShellInit; + private ShellController mShellController; @Before public void setUp() { @@ -74,6 +81,8 @@ public class StartingWindowControllerTests extends ShellTestCase { doReturn(mock(Display.class)).when(mDisplayManager).getDisplay(anyInt()); doReturn(mDisplayManager).when(mContext).getSystemService(eq(DisplayManager.class)); mShellInit = spy(new ShellInit(mMainExecutor)); + mShellController = spy(new ShellController(mShellInit, mShellCommandHandler, + mMainExecutor)); mController = new StartingWindowController(mContext, mShellInit, mShellController, mTaskOrganizer, mMainExecutor, mTypeAlgorithm, mIconProvider, mTransactionPool); mShellInit.init(); @@ -81,7 +90,7 @@ public class StartingWindowControllerTests extends ShellTestCase { @Test public void instantiateController_addInitCallback() { - verify(mShellInit, times(1)).addInitCallback(any(), any()); + verify(mShellInit, times(1)).addInitCallback(any(), isA(StartingWindowController.class)); } @Test @@ -89,4 +98,18 @@ public class StartingWindowControllerTests extends ShellTestCase { verify(mShellController, times(1)).addExternalInterface( eq(ShellSharedConstants.KEY_EXTRA_SHELL_STARTING_WINDOW), any(), any()); } + + @Test + public void testInvalidateExternalInterface_unregistersListener() { + mController.setStartingWindowListener(new TriConsumer<Integer, Integer, Integer>() { + @Override + public void accept(Integer integer, Integer integer2, Integer integer3) {} + }); + assertTrue(mController.hasStartingWindowListener()); + // Create initial interface + mShellController.createExternalInterfaces(new Bundle()); + // Recreate the interface to trigger invalidation of the previous instance + mShellController.createExternalInterfaces(new Bundle()); + assertFalse(mController.hasStartingWindowListener()); + } } |