From 69813c5b457da8a115f6f77677488c0d7eba0fcb Mon Sep 17 00:00:00 2001 From: William Xiao Date: Tue, 7 Mar 2023 18:01:56 -0800 Subject: [PATCH] Ensure ordering of overlay callbacks Before this CL, the DreamOverlayService in SysUI used an executor to run code in callbacks from the base class. However, this was not synchronized with the endDream call coming from OverlayClient in the base class. This meant that an endDream call could come in before or during the onStartDream implementation in SysUI, causing calls like shouldShowComplications and getDreamComponent to throw an exception since the overlay client was already disconnected. This change allows providing an executor to the base DreamOverlayService class that it will use to run callbacks coming from the OverlayClient over binder. This makes it so that endDream will not run before the onStartDream callback's contents run. Bug: 271374409 Test: atest DreamOverlayServiceTest Change-Id: I35a0badfc401d5224943e71fcc241b8a32698bea --- .../service/dreams/DreamOverlayService.java | 64 ++++++++++++--- .../systemui/dreams/DreamOverlayService.java | 80 +++++++++---------- .../dreams/DreamOverlayServiceTest.java | 46 +++++++++++ .../dreams/DreamOverlayServiceTest.java | 64 ++++++++++++++- 4 files changed, 199 insertions(+), 55 deletions(-) diff --git a/core/java/android/service/dreams/DreamOverlayService.java b/core/java/android/service/dreams/DreamOverlayService.java index 6e4535b7218a6..5469916bea4e0 100644 --- a/core/java/android/service/dreams/DreamOverlayService.java +++ b/core/java/android/service/dreams/DreamOverlayService.java @@ -27,6 +27,8 @@ import android.os.RemoteException; import android.util.Log; import android.view.WindowManager; +import java.util.concurrent.Executor; + /** * Basic implementation of for {@link IDreamOverlay} for testing. @@ -40,6 +42,12 @@ public abstract class DreamOverlayService extends Service { // The last client that started dreaming and hasn't ended private OverlayClient mCurrentClient; + /** + * Executor used to run callbacks that subclasses will implement. Any calls coming over Binder + * from {@link OverlayClient} should perform the work they need to do on this executor. + */ + private Executor mExecutor; + // An {@link IDreamOverlayClient} implementation that identifies itself when forwarding // requests to the {@link DreamOverlayService} private static class OverlayClient extends IDreamOverlayClient.Stub { @@ -61,8 +69,6 @@ public abstract class DreamOverlayService extends Service { mService.startDream(this, params); } - - @Override public void wakeUp() { mService.wakeUp(this, () -> { @@ -97,12 +103,20 @@ public abstract class DreamOverlayService extends Service { } private void startDream(OverlayClient client, WindowManager.LayoutParams params) { - endDream(mCurrentClient); - mCurrentClient = client; - onStartDream(params); + // Run on executor as this is a binder call from OverlayClient. + mExecutor.execute(() -> { + endDreamInternal(mCurrentClient); + mCurrentClient = client; + onStartDream(params); + }); } private void endDream(OverlayClient client) { + // Run on executor as this is a binder call from OverlayClient. + mExecutor.execute(() -> endDreamInternal(client)); + } + + private void endDreamInternal(OverlayClient client) { if (client == null || client != mCurrentClient) { return; } @@ -112,11 +126,14 @@ public abstract class DreamOverlayService extends Service { } private void wakeUp(OverlayClient client, Runnable callback) { - if (mCurrentClient != client) { - return; - } + // Run on executor as this is a binder call from OverlayClient. + mExecutor.execute(() -> { + if (mCurrentClient != client) { + return; + } - onWakeUp(callback); + onWakeUp(callback); + }); } private IDreamOverlay mDreamOverlay = new IDreamOverlay.Stub() { @@ -134,6 +151,25 @@ public abstract class DreamOverlayService extends Service { public DreamOverlayService() { } + /** + * This constructor allows providing an executor to run callbacks on. + * + * @hide + */ + public DreamOverlayService(@NonNull Executor executor) { + mExecutor = executor; + } + + @Override + public void onCreate() { + super.onCreate(); + if (mExecutor == null) { + // If no executor was provided, use the main executor. onCreate is the earliest time + // getMainExecutor is available. + mExecutor = getMainExecutor(); + } + } + @Nullable @Override public final IBinder onBind(@NonNull Intent intent) { @@ -143,6 +179,10 @@ public abstract class DreamOverlayService extends Service { /** * This method is overridden by implementations to handle when the dream has started and the * window is ready to be interacted with. + * + * This callback will be run on the {@link Executor} provided in the constructor if provided, or + * on the main executor if none was provided. + * * @param layoutParams The {@link android.view.WindowManager.LayoutParams} associated with the * dream window. */ @@ -153,6 +193,9 @@ public abstract class DreamOverlayService extends Service { * to wakeup. This allows any overlay animations to run. By default, the method will invoke * the callback immediately. * + * This callback will be run on the {@link Executor} provided in the constructor if provided, or + * on the main executor if none was provided. + * * @param onCompleteCallback The callback to trigger to notify the dream service that the * overlay has completed waking up. * @hide @@ -164,6 +207,9 @@ public abstract class DreamOverlayService extends Service { /** * This method is overridden by implementations to handle when the dream has ended. There may * be earlier signals leading up to this step, such as @{@link #onWakeUp(Runnable)}. + * + * This callback will be run on the {@link Executor} provided in the constructor if provided, or + * on the main executor if none was provided. */ public void onEndDream() { } diff --git a/packages/SystemUI/src/com/android/systemui/dreams/DreamOverlayService.java b/packages/SystemUI/src/com/android/systemui/dreams/DreamOverlayService.java index 854323f51555e..a0fef758da519 100644 --- a/packages/SystemUI/src/com/android/systemui/dreams/DreamOverlayService.java +++ b/packages/SystemUI/src/com/android/systemui/dreams/DreamOverlayService.java @@ -139,6 +139,7 @@ public class DreamOverlayService extends android.service.dreams.DreamOverlayServ ComponentName lowLightDreamComponent, DreamOverlayCallbackController dreamOverlayCallbackController, @Named(DREAM_OVERLAY_WINDOW_TITLE) String windowTitle) { + super(executor); mContext = context; mExecutor = executor; mWindowManager = windowManager; @@ -176,55 +177,50 @@ public class DreamOverlayService extends android.service.dreams.DreamOverlayServ @Override public void onStartDream(@NonNull WindowManager.LayoutParams layoutParams) { - mExecutor.execute(() -> { - setCurrentStateLocked(Lifecycle.State.STARTED); + setCurrentStateLocked(Lifecycle.State.STARTED); - mUiEventLogger.log(DreamOverlayEvent.DREAM_OVERLAY_ENTER_START); + mUiEventLogger.log(DreamOverlayEvent.DREAM_OVERLAY_ENTER_START); - if (mDestroyed) { - // The task could still be executed after the service has been destroyed. Bail if - // that is the case. - return; - } + if (mDestroyed) { + // The task could still be executed after the service has been destroyed. Bail if + // that is the case. + return; + } - if (mStarted) { - // Reset the current dream overlay before starting a new one. This can happen - // when two dreams overlap (briefly, for a smoother dream transition) and both - // dreams are bound to the dream overlay service. - resetCurrentDreamOverlayLocked(); - } + if (mStarted) { + // Reset the current dream overlay before starting a new one. This can happen + // when two dreams overlap (briefly, for a smoother dream transition) and both + // dreams are bound to the dream overlay service. + resetCurrentDreamOverlayLocked(); + } - mDreamOverlayContainerViewController = - mDreamOverlayComponent.getDreamOverlayContainerViewController(); - mDreamOverlayTouchMonitor = mDreamOverlayComponent.getDreamOverlayTouchMonitor(); - mDreamOverlayTouchMonitor.init(); + mDreamOverlayContainerViewController = + mDreamOverlayComponent.getDreamOverlayContainerViewController(); + mDreamOverlayTouchMonitor = mDreamOverlayComponent.getDreamOverlayTouchMonitor(); + mDreamOverlayTouchMonitor.init(); - mStateController.setShouldShowComplications(shouldShowComplications()); + mStateController.setShouldShowComplications(shouldShowComplications()); - // If we are not able to add the overlay window, reset the overlay. - if (!addOverlayWindowLocked(layoutParams)) { - resetCurrentDreamOverlayLocked(); - return; - } + // If we are not able to add the overlay window, reset the overlay. + if (!addOverlayWindowLocked(layoutParams)) { + resetCurrentDreamOverlayLocked(); + return; + } + setCurrentStateLocked(Lifecycle.State.RESUMED); + mStateController.setOverlayActive(true); + final ComponentName dreamComponent = getDreamComponent(); + mStateController.setLowLightActive( + dreamComponent != null && dreamComponent.equals(mLowLightDreamComponent)); + mUiEventLogger.log(DreamOverlayEvent.DREAM_OVERLAY_COMPLETE_START); - setCurrentStateLocked(Lifecycle.State.RESUMED); - mStateController.setOverlayActive(true); - final ComponentName dreamComponent = getDreamComponent(); - mStateController.setLowLightActive( - dreamComponent != null && dreamComponent.equals(mLowLightDreamComponent)); - mUiEventLogger.log(DreamOverlayEvent.DREAM_OVERLAY_COMPLETE_START); - - mDreamOverlayCallbackController.onStartDream(); - mStarted = true; - }); + mDreamOverlayCallbackController.onStartDream(); + mStarted = true; } @Override public void onEndDream() { - mExecutor.execute(() -> { - resetCurrentDreamOverlayLocked(); - }); + resetCurrentDreamOverlayLocked(); } private Lifecycle.State getCurrentStateLocked() { @@ -237,12 +233,10 @@ public class DreamOverlayService extends android.service.dreams.DreamOverlayServ @Override public void onWakeUp(@NonNull Runnable onCompletedCallback) { - mExecutor.execute(() -> { - if (mDreamOverlayContainerViewController != null) { - mDreamOverlayCallbackController.onWakeUp(); - mDreamOverlayContainerViewController.wakeUp(onCompletedCallback, mExecutor); - } - }); + if (mDreamOverlayContainerViewController != null) { + mDreamOverlayCallbackController.onWakeUp(); + mDreamOverlayContainerViewController.wakeUp(onCompletedCallback, mExecutor); + } } /** diff --git a/packages/SystemUI/tests/src/com/android/systemui/dreams/DreamOverlayServiceTest.java b/packages/SystemUI/tests/src/com/android/systemui/dreams/DreamOverlayServiceTest.java index de70033664691..6443621a67f44 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/dreams/DreamOverlayServiceTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/dreams/DreamOverlayServiceTest.java @@ -20,7 +20,9 @@ import static com.google.common.truth.Truth.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.clearInvocations; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -60,6 +62,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Captor; +import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.MockitoAnnotations; @@ -254,6 +257,7 @@ public class DreamOverlayServiceTest extends SysuiTestCase { // Inform the overlay service of dream starting. client.startDream(mWindowParams, mDreamOverlayCallback, DREAM_COMPONENT, true /*shouldShowComplication*/); + mMainExecutor.runAllReady(); assertThat(mService.shouldShowComplications()).isTrue(); } @@ -296,6 +300,48 @@ public class DreamOverlayServiceTest extends SysuiTestCase { verify(mStateController).setEntryAnimationsFinished(false); } + @Test + public void testImmediateEndDream() throws Exception { + final IDreamOverlayClient client = getClient(); + + // Start the dream, but don't execute any Runnables put on the executor yet. We delay + // executing Runnables as the timing isn't guaranteed and we want to verify that the overlay + // starts and finishes in the proper order even if Runnables are delayed. + client.startDream(mWindowParams, mDreamOverlayCallback, DREAM_COMPONENT, + false /*shouldShowComplication*/); + // Immediately end the dream. + client.endDream(); + // Run any scheduled Runnables. + mMainExecutor.runAllReady(); + + // The overlay starts then finishes. + InOrder inOrder = inOrder(mWindowManager); + inOrder.verify(mWindowManager).addView(mViewCaptor.capture(), any()); + inOrder.verify(mWindowManager).removeView(mViewCaptor.getValue()); + } + + @Test + public void testEndDreamDuringStartDream() throws Exception { + final IDreamOverlayClient client = getClient(); + + // Schedule the endDream call in the middle of the startDream implementation, as any + // ordering is possible. + doAnswer(invocation -> { + client.endDream(); + return null; + }).when(mStateController).setOverlayActive(true); + + // Start the dream. + client.startDream(mWindowParams, mDreamOverlayCallback, DREAM_COMPONENT, + false /*shouldShowComplication*/); + mMainExecutor.runAllReady(); + + // The overlay starts then finishes. + InOrder inOrder = inOrder(mWindowManager); + inOrder.verify(mWindowManager).addView(mViewCaptor.capture(), any()); + inOrder.verify(mWindowManager).removeView(mViewCaptor.getValue()); + } + @Test public void testDestroy() throws RemoteException { final IDreamOverlayClient client = getClient(); diff --git a/services/tests/servicestests/src/com/android/server/dreams/DreamOverlayServiceTest.java b/services/tests/servicestests/src/com/android/server/dreams/DreamOverlayServiceTest.java index 6c73f716493ce..851d8f94d2c04 100644 --- a/services/tests/servicestests/src/com/android/server/dreams/DreamOverlayServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/dreams/DreamOverlayServiceTest.java @@ -18,6 +18,8 @@ package com.android.server.dreams; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -39,10 +41,13 @@ import androidx.test.runner.AndroidJUnit4; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.MockitoAnnotations; +import java.util.concurrent.Executor; + /** * A collection of tests to exercise {@link DreamOverlayService}. */ @@ -60,6 +65,9 @@ public class DreamOverlayServiceTest { @Mock IDreamOverlayCallback mOverlayCallback; + @Mock + Executor mExecutor; + /** * {@link TestDreamOverlayService} is a simple {@link DreamOverlayService} implementation for * tracking interactions across {@link IDreamOverlay} binder interface. The service reports @@ -78,8 +86,8 @@ public class DreamOverlayServiceTest { private final Monitor mMonitor; - TestDreamOverlayService(Monitor monitor) { - super(); + TestDreamOverlayService(Monitor monitor, Executor executor) { + super(executor); mMonitor = monitor; } @@ -117,14 +125,64 @@ public class DreamOverlayServiceTest { MockitoAnnotations.initMocks(this); } + /** + * Verifies that callbacks for subclasses are run on the provided executor. + */ + @Test + public void testCallbacksRunOnExecutor() throws RemoteException { + final TestDreamOverlayService.Monitor monitor = Mockito.mock( + TestDreamOverlayService.Monitor.class); + final TestDreamOverlayService service = new TestDreamOverlayService(monitor, mExecutor); + final IBinder binder = service.onBind(new Intent()); + final IDreamOverlay overlay = IDreamOverlay.Stub.asInterface(binder); + + final IDreamOverlayClient client = getClient(overlay); + + // Start the dream. + client.startDream(mLayoutParams, mOverlayCallback, + FIRST_DREAM_COMPONENT.flattenToString(), false); + + // The callback should not have run yet. + verify(monitor, never()).onStartDream(); + + // Run the Runnable sent to the executor. + ArgumentCaptor mRunnableCaptor = ArgumentCaptor.forClass(Runnable.class); + verify(mExecutor).execute(mRunnableCaptor.capture()); + mRunnableCaptor.getValue().run(); + + // Callback is run. + verify(monitor).onStartDream(); + + // Verify onWakeUp is run on the executor. + client.wakeUp(); + verify(monitor, never()).onWakeUp(); + mRunnableCaptor = ArgumentCaptor.forClass(Runnable.class); + verify(mExecutor).execute(mRunnableCaptor.capture()); + mRunnableCaptor.getValue().run(); + verify(monitor).onWakeUp(); + + // Verify onEndDream is run on the executor. + client.endDream(); + verify(monitor, never()).onEndDream(); + mRunnableCaptor = ArgumentCaptor.forClass(Runnable.class); + verify(mExecutor).execute(mRunnableCaptor.capture()); + mRunnableCaptor.getValue().run(); + verify(monitor).onEndDream(); + } + /** * Verifies that only the currently started dream is able to affect the overlay. */ @Test public void testOverlayClientInteraction() throws RemoteException { + doAnswer(invocation -> { + ((Runnable) invocation.getArgument(0)).run(); + return null; + }).when(mExecutor).execute(any()); + final TestDreamOverlayService.Monitor monitor = Mockito.mock( TestDreamOverlayService.Monitor.class); - final TestDreamOverlayService service = new TestDreamOverlayService(monitor); + final TestDreamOverlayService service = new TestDreamOverlayService(monitor, mExecutor); final IBinder binder = service.onBind(new Intent()); final IDreamOverlay overlay = IDreamOverlay.Stub.asInterface(binder);