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

Commit a19bbf2b authored by Winson Chung's avatar Winson Chung
Browse files

Disallow non-primary SysUI users from trying to connect to overview service 



- This CL does not address the root issue of SysUI components being
  started for profile or secondary users (which should never happen, but
  does based on logs and local testing).  For OPS at least, we should
  not allow these users to try to bind to the OverviewProxyService since
  it will clobber the connection from the primary user preventing
  recents/gesture nav/etc from working
- Also fix a dependency loop which can occur from NavBarHelper/
  AssistManager registering an OPS callback, which is called immediately
  called and triggers creating AssistManager again
- Add some wtfs to monitor when/how these happen later

Bug: 319489709
Test: atest SystemUITests
Test: Constantly enable/disable work profile, verify no connections to TIS
Test: Start switch to secondary user, cancel and verify primary user
      connection to TIS is restored

Change-Id: I784f16e267c727afcbdb37c3d9191759a14c720a
Signed-off-by: default avatarWinson Chung <winsonc@google.com>
parent 684e0da6

packages/SystemUI/src/com/android/systemui/navigationbar/NavBarHelper.java

+16 −2
Original line number Diff line number Diff line
@@ -41,6 +41,7 @@ import android.net.Uri; 
import android.os.Bundle;

import android.os.Handler;

import android.os.Looper;

import android.os.Process;

import android.os.RemoteException;

import android.os.UserHandle;

import android.provider.Settings;
@@ -61,6 +62,7 @@ import com.android.systemui.accessibility.AccessibilityButtonTargetsObserver; 
import com.android.systemui.accessibility.SystemActions;

import com.android.systemui.assist.AssistManager;

import com.android.systemui.dagger.SysUISingleton;

import com.android.systemui.dagger.qualifiers.Main;

import com.android.systemui.dump.DumpManager;

import com.android.systemui.navigationbar.gestural.EdgeBackGestureHandler;

import com.android.systemui.recents.OverviewProxyService;
@@ -79,6 +81,7 @@ import java.io.PrintWriter; 
import java.util.ArrayList;

import java.util.List;

import java.util.Optional;

import java.util.concurrent.Executor;



import javax.inject.Inject;
@@ -101,6 +104,7 @@ public final class NavBarHelper implements 
    private static final String TAG = NavBarHelper.class.getSimpleName();



    private final Handler mHandler = new Handler(Looper.getMainLooper());

    private final Executor mMainExecutor;

    private final AccessibilityManager mAccessibilityManager;

    private final Lazy<AssistManager> mAssistManagerLazy;

    private final Lazy<Optional<CentralSurfaces>> mCentralSurfacesOptionalLazy;
@@ -185,7 +189,12 @@ public final class NavBarHelper implements 
            DisplayTracker displayTracker,

            NotificationShadeWindowController notificationShadeWindowController,

            DumpManager dumpManager,

            CommandQueue commandQueue) {

            CommandQueue commandQueue,

            @Main Executor mainExecutor) {

        // b/319489709: This component shouldn't be running for a non-primary user

        if (!Process.myUserHandle().equals(UserHandle.SYSTEM)) {

            Log.wtf(TAG, "Unexpected initialization for non-primary user", new Throwable());

        }

        mContext = context;

        mNotificationShadeWindowController = notificationShadeWindowController;

        mCommandQueue = commandQueue;
@@ -201,6 +210,7 @@ public final class NavBarHelper implements 
        mWm = wm;

        mDefaultDisplayId = displayTracker.getDefaultDisplayId();

        mEdgeBackGestureHandler = edgeBackGestureHandlerFactory.create(context);

        mMainExecutor = mainExecutor;



        mNavBarMode = navigationModeController.addListener(this);

        mCommandQueue.addCallback(this);
@@ -418,7 +428,11 @@ public final class NavBarHelper implements 
    @Override

    public void onConnectionChanged(boolean isConnected) {

        if (isConnected) {

            updateAssistantAvailability();

            // We add the OPS callback during construction, so if the service is already connected

            // then we will try to get the AssistManager dependency which itself has an indirect

            // dependency on NavBarHelper leading to a cycle. For now, we can defer updating the

            // assistant availability.

            mMainExecutor.execute(this::updateAssistantAvailability);

        }

    }

packages/SystemUI/src/com/android/systemui/recents/OverviewProxyService.java

+12 −2
Original line number Diff line number Diff line
@@ -177,6 +177,8 @@ public class OverviewProxyService implements CallbackController<OverviewProxyLis 
    private int mConnectionBackoffAttempts;

    private boolean mBound;

    private boolean mIsEnabled;



    private boolean mIsNonPrimaryUser;

    private int mCurrentBoundedUserId = -1;

    private boolean mInputFocusTransferStarted;

    private float mInputFocusTransferStartY;
@@ -608,8 +610,9 @@ public class OverviewProxyService implements CallbackController<OverviewProxyLis 
            BroadcastDispatcher broadcastDispatcher

    ) {

        // b/241601880: This component shouldn't be running for a non-primary user

        if (!Process.myUserHandle().equals(UserHandle.SYSTEM)) {

            Log.e(TAG_OPS, "Unexpected initialization for non-primary user", new Throwable());

        mIsNonPrimaryUser = !Process.myUserHandle().equals(UserHandle.SYSTEM);

        if (mIsNonPrimaryUser) {

            Log.wtf(TAG_OPS, "Unexpected initialization for non-primary user", new Throwable());

        }



        mContext = context;
@@ -798,6 +801,13 @@ public class OverviewProxyService implements CallbackController<OverviewProxyLis 
    }



    private void internalConnectToCurrentUser(String reason) {

        if (mIsNonPrimaryUser) {

            // This should not happen, but if any per-user SysUI component has a dependency on OPS,

            // then this could get triggered

            Log.w(TAG_OPS, "Skipping connection to overview service due to non-primary user "

                    + "caller");

            return;

        }

        disconnectFromLauncherService(reason);



        // If user has not setup yet or already connected, do not try to connect

packages/SystemUI/tests/src/com/android/systemui/navigationbar/NavBarHelperTest.java

+5 −1
Original line number Diff line number Diff line
@@ -65,6 +65,7 @@ import org.mockito.MockitoAnnotations; 
import java.util.ArrayList;

import java.util.List;

import java.util.Optional;

import java.util.concurrent.Executor;



import dagger.Lazy;
@@ -123,6 +124,8 @@ public class NavBarHelperTest extends SysuiTestCase { 
            SYSUI_STATE_A11Y_BUTTON_CLICKABLE | SYSUI_STATE_A11Y_BUTTON_LONG_CLICKABLE;

    private NavBarHelper mNavBarHelper;



    private final Executor mSynchronousExecutor = runnable -> runnable.run();



    @Before

    public void setup() {

        MockitoAnnotations.initMocks(this);
@@ -140,7 +143,8 @@ public class NavBarHelperTest extends SysuiTestCase { 
                mSystemActions, mOverviewProxyService, mAssistManagerLazy,

                () -> Optional.of(mock(CentralSurfaces.class)), mock(KeyguardStateController.class),

                mNavigationModeController, mEdgeBackGestureHandlerFactory, mWm, mUserTracker,

                mDisplayTracker, mNotificationShadeWindowController, mDumpManager, mCommandQueue);

                mDisplayTracker, mNotificationShadeWindowController, mDumpManager, mCommandQueue,

                mSynchronousExecutor);



    }

packages/SystemUI/tests/src/com/android/systemui/navigationbar/NavigationBarTest.java

+3 −1
Original line number Diff line number Diff line
@@ -228,6 +228,8 @@ public class NavigationBarTest extends SysuiTestCase { 
    @Rule

    public final LeakCheckedTest.SysuiLeakCheck mLeakCheck = new LeakCheckedTest.SysuiLeakCheck();



    private final Executor mSynchronousExecutor = runnable -> runnable.run();



    @Before

    public void setup() throws Exception {

        MockitoAnnotations.initMocks(this);
@@ -269,7 +271,7 @@ public class NavigationBarTest extends SysuiTestCase { 
                    mEdgeBackGestureHandlerFactory, mock(IWindowManager.class),

                    mock(UserTracker.class), mock(DisplayTracker.class),

                    mNotificationShadeWindowController, mock(DumpManager.class),

                    mock(CommandQueue.class)));

                    mock(CommandQueue.class), mSynchronousExecutor));

            mNavigationBar = createNavBar(mContext);

            mExternalDisplayNavigationBar = createNavBar(mSysuiTestableContextExternal);

        });

packages/SystemUI/tests/src/com/android/systemui/recents/OverviewProxyServiceTest.kt

+72 −27
Original line number Diff line number Diff line
@@ -17,13 +17,17 @@ 
package com.android.systemui.recents



import android.content.ComponentName

import android.content.Context

import android.content.pm.PackageManager

import android.content.pm.ResolveInfo

import android.os.PowerManager

import android.os.Process;

import android.os.UserHandle

import android.testing.AndroidTestingRunner

import android.testing.TestableContext

import android.testing.TestableLooper

import androidx.test.filters.SmallTest

import com.android.dx.mockito.inline.extended.ExtendedMockito

import com.android.internal.app.AssistUtils

import com.android.internal.logging.UiEventLogger

import com.android.systemui.SysuiTestCase
@@ -67,10 +71,14 @@ import org.mockito.ArgumentMatchers 
import org.mockito.Mock

import org.mockito.Mockito.any

import org.mockito.Mockito.anyInt

import org.mockito.Mockito.atLeast

import org.mockito.Mockito.clearInvocations

import org.mockito.Mockito.intThat

import org.mockito.Mockito.mock

import org.mockito.Mockito.spy

import org.mockito.Mockito.times

import org.mockito.Mockito.verify

import org.mockito.Mockito.`when`

import org.mockito.MockitoAnnotations



@SmallTest
@@ -81,7 +89,7 @@ class OverviewProxyServiceTest : SysuiTestCase() { 
    @Main private val executor: Executor = MoreExecutors.directExecutor()



    private lateinit var subject: OverviewProxyService

    private val dumpManager = DumpManager()

    @Mock private val dumpManager = DumpManager()

    private val displayTracker = FakeDisplayTracker(mContext)

    private val fakeSystemClock = FakeSystemClock()

    private val sysUiState = SysUiState(displayTracker)
@@ -138,32 +146,7 @@ class OverviewProxyServiceTest : SysuiTestCase() { 
            com.android.systemui.Flags.FLAG_KEYGUARD_WM_STATE_REFACTOR,

        )



        subject =

            OverviewProxyService(

                context,

                executor,

                commandQueue,

                shellInterface,

                { navBarController },

                { shadeViewController },

                screenPinningRequest,

                navModeController,

                statusBarWinController,

                sysUiState,

                mock(),

                userTracker,

                wakefulnessLifecycle,

                uiEventLogger,

                displayTracker,

                sysuiUnlockAnimationController,

                inWindowLauncherUnlockAnimationManager,

                assistUtils,

                featureFlags,

                FakeSceneContainerFlags(),

                dumpManager,

                unfoldTransitionProgressForwarder,

                broadcastDispatcher

            )

        subject = createOverviewProxyService(context)

    }



    @After
@@ -216,4 +199,66 @@ class OverviewProxyServiceTest : SysuiTestCase() { 
                intThat { it and SYSUI_STATE_WAKEFULNESS_MASK == WAKEFULNESS_GOING_TO_SLEEP }

            )

    }



    @Test

    fun connectToOverviewService_primaryUser_expectBindService() {

        val mockitoSession = ExtendedMockito.mockitoSession()

                .spyStatic(Process::class.java)

                .startMocking()

        try {

            `when`(Process.myUserHandle()).thenReturn(UserHandle.SYSTEM)

            val spyContext = spy(context)

            val ops = createOverviewProxyService(spyContext)

            ops.startConnectionToCurrentUser()

            verify(spyContext, atLeast(1)).bindServiceAsUser(any(), any(),

                anyInt(), any())

        } finally {

            mockitoSession.finishMocking()

        }

    }



    @Test

    fun connectToOverviewService_nonPrimaryUser_expectNoBindService() {

        val mockitoSession = ExtendedMockito.mockitoSession()

                .spyStatic(Process::class.java)

                .startMocking()

        try {

            `when`(Process.myUserHandle()).thenReturn(UserHandle.of(12345))

            val spyContext = spy(context)

            val ops = createOverviewProxyService(spyContext)

            ops.startConnectionToCurrentUser()

            verify(spyContext, times(0)).bindServiceAsUser(any(), any(),

                anyInt(), any())

        } finally {

            mockitoSession.finishMocking()

        }

    }



    private fun createOverviewProxyService(ctx: Context) : OverviewProxyService {

        return OverviewProxyService(

            ctx,

            executor,

            commandQueue,

            shellInterface,

            { navBarController },

            { shadeViewController },

            screenPinningRequest,

            navModeController,

            statusBarWinController,

            sysUiState,

            mock(),

            userTracker,

            wakefulnessLifecycle,

            uiEventLogger,

            displayTracker,

            sysuiUnlockAnimationController,

            inWindowLauncherUnlockAnimationManager,

            assistUtils,

            featureFlags,

            FakeSceneContainerFlags(),

            dumpManager,

            unfoldTransitionProgressForwarder,

            broadcastDispatcher

        )

    }

}