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

Commit 69813c5b authored by William Xiao's avatar William Xiao
Browse files

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
parent 55395739

core/java/android/service/dreams/DreamOverlayService.java

+55 −9
Original line number Diff line number Diff line
@@ -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);

        // 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) {

        // Run on executor as this is a binder call from OverlayClient.

        mExecutor.execute(() -> {

            if (mCurrentClient != client) {

                return;

            }



            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() {

    }

packages/SystemUI/src/com/android/systemui/dreams/DreamOverlayService.java

+37 −43
Original line number Diff line number Diff line
@@ -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,7 +177,6 @@ public class DreamOverlayService extends android.service.dreams.DreamOverlayServ 


    @Override

    public void onStartDream(@NonNull WindowManager.LayoutParams layoutParams) {

        mExecutor.execute(() -> {

        setCurrentStateLocked(Lifecycle.State.STARTED);



        mUiEventLogger.log(DreamOverlayEvent.DREAM_OVERLAY_ENTER_START);
@@ -207,7 +207,6 @@ public class DreamOverlayService extends android.service.dreams.DreamOverlayServ 
            return;

        }





        setCurrentStateLocked(Lifecycle.State.RESUMED);

        mStateController.setOverlayActive(true);

        final ComponentName dreamComponent = getDreamComponent();
@@ -217,14 +216,11 @@ public class DreamOverlayService extends android.service.dreams.DreamOverlayServ 


        mDreamOverlayCallbackController.onStartDream();

        mStarted = true;

        });

    }



    @Override

    public void onEndDream() {

        mExecutor.execute(() -> {

        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);

        }

        });

    }



    /**

packages/SystemUI/tests/src/com/android/systemui/dreams/DreamOverlayServiceTest.java

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

services/tests/servicestests/src/com/android/server/dreams/DreamOverlayServiceTest.java

+61 −3
Original line number Diff line number Diff line
@@ -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<Runnable> 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);