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

Commit 94cdcb79 authored by Joe Bolinger's avatar Joe Bolinger
Browse files

Wrap start and stop user clients in an operation. 

Addresses a race condition where newly queued operations can start before user switches complete.

Fix: 191798733
Test: atest UserAwareBiometricSchedulerTest
Test: manual (reboot -> Settings -> search Face & Fingerprint -> select Fingerprint -> menu opens successfully)
Change-Id: I425c84061a2aa5390d23e88d46155710988394b5
parent 472d9624

services/core/java/com/android/server/biometrics/sensors/BiometricScheduler.java

+15 −5
Original line number Diff line number Diff line
@@ -110,11 +110,21 @@ public class BiometricScheduler { 
        @Nullable final BaseClientMonitor.Callback mClientCallback;

        @OperationState int mState;



        Operation(@NonNull BaseClientMonitor clientMonitor,

                @Nullable BaseClientMonitor.Callback callback) {

            this.mClientMonitor = clientMonitor;

            this.mClientCallback = callback;

            mState = STATE_WAITING_IN_QUEUE;

        Operation(

                @NonNull BaseClientMonitor clientMonitor,

                @Nullable BaseClientMonitor.Callback callback

        ) {

            this(clientMonitor, callback, STATE_WAITING_IN_QUEUE);

        }



        protected Operation(

                @NonNull BaseClientMonitor clientMonitor,

                @Nullable BaseClientMonitor.Callback callback,

                @OperationState int state

        ) {

            mClientMonitor = clientMonitor;

            mClientCallback = callback;

            mState = state;

        }



        public boolean isHalOperation() {

services/core/java/com/android/server/biometrics/sensors/UserAwareBiometricScheduler.java

+27 −8
Original line number Diff line number Diff line
@@ -50,16 +50,26 @@ public class UserAwareBiometricScheduler extends BiometricScheduler { 


    @NonNull private final CurrentUserRetriever mCurrentUserRetriever;

    @NonNull private final UserSwitchCallback mUserSwitchCallback;

    @NonNull @VisibleForTesting final ClientFinishedCallback mClientFinishedCallback;



    @Nullable private StopUserClient<?> mStopUserClient;



    @VisibleForTesting

    class ClientFinishedCallback implements BaseClientMonitor.Callback {

    private class ClientFinishedCallback implements BaseClientMonitor.Callback {

        private final BaseClientMonitor mOwner;



        ClientFinishedCallback(BaseClientMonitor owner) {

            mOwner = owner;

        }



        @Override

        public void onClientFinished(@NonNull BaseClientMonitor clientMonitor, boolean success) {

            mHandler.post(() -> {

                Slog.d(getTag(), "[Client finished] " + clientMonitor + ", success: " + success);

                if (mOwner == clientMonitor && mOwner == mCurrentOperation.mClientMonitor) {

                    Slog.d(getTag(), "[Client finished] "

                            + clientMonitor + ", success: " + success);

                    mCurrentOperation = null;

                } else {

                    Slog.e(getTag(), "[Client finished, but not current operation], actual: "

                            + mCurrentOperation + ", expected: " + mOwner);

                }



                startNextOperationIfIdle();

            });
@@ -76,7 +86,6 @@ public class UserAwareBiometricScheduler extends BiometricScheduler { 


        mCurrentUserRetriever = currentUserRetriever;

        mUserSwitchCallback = userSwitchCallback;

        mClientFinishedCallback = new ClientFinishedCallback();

    }



    public UserAwareBiometricScheduler(@NonNull String tag,
@@ -112,17 +121,27 @@ public class UserAwareBiometricScheduler extends BiometricScheduler { 
        } else if (currentUserId == UserHandle.USER_NULL) {

            final BaseClientMonitor startClient =

                    mUserSwitchCallback.getStartUserClient(nextUserId);

            final ClientFinishedCallback finishedCallback =

                    new ClientFinishedCallback(startClient);



            Slog.d(getTag(), "[Starting User] " + startClient);

            startClient.start(mClientFinishedCallback);

            startClient.start(finishedCallback);

            mCurrentOperation = new Operation(

                    startClient, finishedCallback, Operation.STATE_STARTED);

        } else {

            if (mStopUserClient != null) {

                Slog.d(getTag(), "[Waiting for StopUser] " + mStopUserClient);

            } else {

                mStopUserClient = mUserSwitchCallback

                        .getStopUserClient(currentUserId);

                final ClientFinishedCallback finishedCallback =

                        new ClientFinishedCallback(mStopUserClient);



                Slog.d(getTag(), "[Stopping User] current: " + currentUserId

                        + ", next: " + nextUserId + ". " + mStopUserClient);

                mStopUserClient.start(mClientFinishedCallback);

                mStopUserClient.start(finishedCallback);

                mCurrentOperation = new Operation(

                        mStopUserClient, finishedCallback, Operation.STATE_STARTED);

            }

        }

    }

services/tests/servicestests/src/com/android/server/biometrics/sensors/UserAwareBiometricSchedulerTest.java

+42 −10
Original line number Diff line number Diff line
@@ -19,6 +19,7 @@ package com.android.server.biometrics.sensors; 
import static org.junit.Assert.assertEquals;

import static org.mockito.ArgumentMatchers.any;

import static org.mockito.Mockito.mock;

import static org.mockito.Mockito.never;

import static org.mockito.Mockito.verify;

import static org.mockito.Mockito.when;
@@ -57,12 +58,16 @@ public class UserAwareBiometricSchedulerTest { 
    private TestUserStartedCallback mUserStartedCallback;

    private TestUserStoppedCallback mUserStoppedCallback;

    private int mCurrentUserId = UserHandle.USER_NULL;

    private boolean mStartOperationsFinish;

    private int mStartUserClientCount;



    @Before

    public void setUp() {

        MockitoAnnotations.initMocks(this);



        mToken = new Binder();

        mStartOperationsFinish = true;

        mStartUserClientCount = 0;

        mUserStartedCallback = new TestUserStartedCallback();

        mUserStoppedCallback = new TestUserStoppedCallback();
@@ -81,8 +86,9 @@ public class UserAwareBiometricSchedulerTest { 
                    @NonNull

                    @Override

                    public StartUserClient<?, ?> getStartUserClient(int newUserId) {

                        mStartUserClientCount++;

                        return new TestStartUserClient(mContext, Object::new, mToken, newUserId,

                                TEST_SENSOR_ID, mUserStartedCallback);

                                TEST_SENSOR_ID, mUserStartedCallback, mStartOperationsFinish);

                    }

                });

    }
@@ -91,18 +97,39 @@ public class UserAwareBiometricSchedulerTest { 
    public void testScheduleOperation_whenNoUser() {

        mCurrentUserId = UserHandle.USER_NULL;



        final int nextUserId = 0;



        BaseClientMonitor nextClient = mock(BaseClientMonitor.class);

        when(nextClient.getTargetUserId()).thenReturn(nextUserId);

        final BaseClientMonitor nextClient = mock(BaseClientMonitor.class);

        when(nextClient.getTargetUserId()).thenReturn(0);



        mScheduler.scheduleClientMonitor(nextClient);

        waitForIdle();



        assertEquals(0, mUserStoppedCallback.numInvocations);

        assertEquals(1, mUserStartedCallback.numInvocations);

        verify(nextClient).start(any());

    }



    @Test

    public void testScheduleOperation_whenNoUser_notStarted() {

        mCurrentUserId = UserHandle.USER_NULL;

        mStartOperationsFinish = false;



        final BaseClientMonitor[] nextClients = new BaseClientMonitor[] {

                mock(BaseClientMonitor.class),

                mock(BaseClientMonitor.class),

                mock(BaseClientMonitor.class)

        };

        for (BaseClientMonitor client : nextClients) {

            when(client.getTargetUserId()).thenReturn(5);

            mScheduler.scheduleClientMonitor(client);

            waitForIdle();

        verify(nextClient).start(any());

        }



        assertEquals(0, mUserStoppedCallback.numInvocations);

        assertEquals(0, mUserStartedCallback.numInvocations);

        assertEquals(1, mStartUserClientCount);

        for (BaseClientMonitor client : nextClients) {

            verify(client, never()).start(any());

        }

    }



    @Test
@@ -192,10 +219,13 @@ public class UserAwareBiometricSchedulerTest { 
    }



    private static class TestStartUserClient extends StartUserClient<Object, Object> {

        private final boolean mShouldFinish;



        public TestStartUserClient(@NonNull Context context,

                @NonNull LazyDaemon<Object> lazyDaemon, @Nullable IBinder token, int userId,

                int sensorId, @NonNull UserStartedCallback<Object> callback) {

                int sensorId, @NonNull UserStartedCallback<Object> callback, boolean shouldFinish) {

            super(context, lazyDaemon, token, userId, sensorId, callback);

            mShouldFinish = shouldFinish;

        }



        @Override
@@ -206,9 +236,11 @@ public class UserAwareBiometricSchedulerTest { 
        @Override

        public void start(@NonNull Callback callback) {

            super.start(callback);

            if (mShouldFinish) {

                mUserStartedCallback.onUserStarted(getTargetUserId(), new Object());

                callback.onClientFinished(this, true /* success */);

            }

        }



        @Override

        public void unableToStart() {