Merge changes I171604ee,I81d648e5 into tm-dev

* changes:
  Do nothing in duplicate onDialogAnimatedIn calls
  Don't crash on illegal biometric states

services/core/java/com/android/server/biometrics/AuthSession.java

@@ -538,13 +538,12 @@ public final class AuthSession implements IBinder.DeathRecipient {
 
     void onDialogAnimatedIn() {
         if (mState != STATE_AUTH_STARTED) {
-            Slog.w(TAG, "onDialogAnimatedIn, unexpected state: " + mState);
+            Slog.e(TAG, "onDialogAnimatedIn, unexpected state: " + mState);
+            return;
         }
 
         mState = STATE_AUTH_STARTED_UI_SHOWING;
-
         startAllPreparedFingerprintSensors();
-        mState = STATE_AUTH_STARTED_UI_SHOWING;
     }
 
     void onTryAgainPressed() {

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

@@ -20,14 +20,18 @@ import android.annotation.IntDef;
 import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.hardware.biometrics.BiometricConstants;
+import android.os.Build;
 import android.os.Handler;
 import android.os.IBinder;
 import android.util.Slog;
 
 import com.android.internal.annotations.VisibleForTesting;
+import com.android.internal.util.ArrayUtils;
 
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
+import java.util.Arrays;
+import java.util.function.BooleanSupplier;
 
 /**
  * Contains all the necessary information for a HAL operation.
@@ -84,6 +88,8 @@ public class BiometricSchedulerOperation {
     private final BaseClientMonitor mClientMonitor;
     @Nullable
     private final ClientMonitorCallback mClientCallback;
+    @NonNull
+    private final BooleanSupplier mIsDebuggable;
     @Nullable
     private ClientMonitorCallback mOnStartCallback;
     @OperationState
@@ -99,14 +105,33 @@ public class BiometricSchedulerOperation {
         this(clientMonitor, callback, STATE_WAITING_IN_QUEUE);
     }
 
+    @VisibleForTesting
+    BiometricSchedulerOperation(
+            @NonNull BaseClientMonitor clientMonitor,
+            @Nullable ClientMonitorCallback callback,
+            @NonNull BooleanSupplier isDebuggable
+    ) {
+        this(clientMonitor, callback, STATE_WAITING_IN_QUEUE, isDebuggable);
+    }
+
     protected BiometricSchedulerOperation(
             @NonNull BaseClientMonitor clientMonitor,
             @Nullable ClientMonitorCallback callback,
             @OperationState int state
+    ) {
+        this(clientMonitor, callback, state, Build::isDebuggable);
+    }
+
+    private BiometricSchedulerOperation(
+            @NonNull BaseClientMonitor clientMonitor,
+            @Nullable ClientMonitorCallback callback,
+            @OperationState int state,
+            @NonNull BooleanSupplier isDebuggable
     ) {
         mClientMonitor = clientMonitor;
         mClientCallback = callback;
         mState = state;
+        mIsDebuggable = isDebuggable;
         mCancelWatchdog = () -> {
             if (!isFinished()) {
                 Slog.e(TAG, "[Watchdog Triggered]: " + this);
@@ -144,13 +169,19 @@ public class BiometricSchedulerOperation {
      * @return if this operation started
      */
     public boolean start(@NonNull ClientMonitorCallback callback) {
-        checkInState("start",
+        if (errorWhenNoneOf("start",
                 STATE_WAITING_IN_QUEUE,
                 STATE_WAITING_FOR_COOKIE,
-                STATE_WAITING_IN_QUEUE_CANCELING);
+                STATE_WAITING_IN_QUEUE_CANCELING)) {
+            return false;
+        }
 
         if (mClientMonitor.getCookie() != 0) {
-            throw new IllegalStateException("operation requires cookie");
+            String err = "operation requires cookie";
+            if (mIsDebuggable.getAsBoolean()) {
+                throw new IllegalStateException(err);
+            }
+            Slog.e(TAG, err);
         }
 
         return doStart(callback);
@@ -164,16 +195,18 @@ public class BiometricSchedulerOperation {
      * @return if this operation started
      */
     public boolean startWithCookie(@NonNull ClientMonitorCallback callback, int cookie) {
-        checkInState("start",
-                STATE_WAITING_IN_QUEUE,
-                STATE_WAITING_FOR_COOKIE,
-                STATE_WAITING_IN_QUEUE_CANCELING);
-
         if (mClientMonitor.getCookie() != cookie) {
             Slog.e(TAG, "Mismatched cookie for operation: " + this + ", received: " + cookie);
             return false;
         }
 
+        if (errorWhenNoneOf("start",
+                STATE_WAITING_IN_QUEUE,
+                STATE_WAITING_FOR_COOKIE,
+                STATE_WAITING_IN_QUEUE_CANCELING)) {
+            return false;
+        }
+
         return doStart(callback);
     }
 
@@ -217,10 +250,12 @@ public class BiometricSchedulerOperation {
      * immediately abort the operation and notify the client that it has finished unsuccessfully.
      */
     public void abort() {
-        checkInState("cannot abort a non-pending operation",
+        if (errorWhenNoneOf("abort",
                 STATE_WAITING_IN_QUEUE,
                 STATE_WAITING_FOR_COOKIE,
-                STATE_WAITING_IN_QUEUE_CANCELING);
+                STATE_WAITING_IN_QUEUE_CANCELING)) {
+            return;
+        }
 
         if (isHalOperation()) {
             ((HalClientMonitor<?>) mClientMonitor).unableToStart();
@@ -247,7 +282,9 @@ public class BiometricSchedulerOperation {
      *                 the callback used from {@link #start(ClientMonitorCallback)} is used)
      */
     public void cancel(@NonNull Handler handler, @NonNull ClientMonitorCallback callback) {
-        checkNotInState("cancel", STATE_FINISHED);
+        if (errorWhenOneOf("cancel", STATE_FINISHED)) {
+            return;
+        }
 
         final int currentState = mState;
         if (!isInterruptable()) {
@@ -402,21 +439,28 @@ public class BiometricSchedulerOperation {
         return mClientMonitor;
     }
 
-    private void checkNotInState(String message, @OperationState int... states) {
+    private boolean errorWhenOneOf(String op, @OperationState int... states) {
-        for (int state : states) {
+        final boolean isError = ArrayUtils.contains(states, mState);
-            if (mState == state) {
+        if (isError) {
-                throw new IllegalStateException(message + ": illegal state= " + state);
+            String err = op + ": mState must not be " + mState;
+            if (mIsDebuggable.getAsBoolean()) {
+                throw new IllegalStateException(err);
             }
+            Slog.e(TAG, err);
         }
+        return isError;
     }
 
-    private void checkInState(String message, @OperationState int... states) {
+    private boolean errorWhenNoneOf(String op, @OperationState int... states) {
-        for (int state : states) {
+        final boolean isError = !ArrayUtils.contains(states, mState);
-            if (mState == state) {
+        if (isError) {
-                return;
+            String err = op + ": mState=" + mState + " must be one of " + Arrays.toString(states);
+            if (mIsDebuggable.getAsBoolean()) {
+                throw new IllegalStateException(err);
             }
+            Slog.e(TAG, err);
         }
-        throw new IllegalStateException(message + ": illegal state= " + mState);
+        return isError;
     }
 
     @Override

services/tests/servicestests/src/com/android/server/biometrics/AuthSessionTest.java

@@ -20,7 +20,9 @@ import static android.hardware.biometrics.BiometricAuthenticator.TYPE_FACE;
 import static android.hardware.biometrics.BiometricAuthenticator.TYPE_FINGERPRINT;
 import static android.hardware.biometrics.BiometricPrompt.DISMISSED_REASON_NEGATIVE;
 
-import static com.android.server.biometrics.BiometricServiceStateProto.*;
+import static com.android.server.biometrics.BiometricServiceStateProto.STATE_AUTH_CALLED;
+import static com.android.server.biometrics.BiometricServiceStateProto.STATE_AUTH_STARTED;
+import static com.android.server.biometrics.BiometricServiceStateProto.STATE_AUTH_STARTED_UI_SHOWING;
 
 import static junit.framework.Assert.assertEquals;
 import static junit.framework.Assert.assertFalse;
@@ -32,6 +34,8 @@ import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
@@ -279,6 +283,43 @@ public class AuthSessionTest {
                 session.mPreAuthInfo.eligibleSensors.get(fingerprintSensorId).getSensorState());
     }
 
+    @Test
+    public void testOnDialogAnimatedInDoesNothingDuringInvalidState() throws Exception {
+        setupFingerprint(0 /* id */, FingerprintSensorProperties.TYPE_UDFPS_OPTICAL);
+        final long operationId = 123;
+        final int userId = 10;
+
+        final AuthSession session = createAuthSession(mSensors,
+                false /* checkDevicePolicyManager */,
+                Authenticators.BIOMETRIC_STRONG,
+                TEST_REQUEST_ID,
+                operationId,
+                userId);
+        final IBiometricAuthenticator impl = session.mPreAuthInfo.eligibleSensors.get(0).impl;
+
+        session.goToInitialState();
+        for (BiometricSensor sensor : session.mPreAuthInfo.eligibleSensors) {
+            assertEquals(BiometricSensor.STATE_WAITING_FOR_COOKIE, sensor.getSensorState());
+            session.onCookieReceived(
+                    session.mPreAuthInfo.eligibleSensors.get(sensor.id).getCookie());
+        }
+        assertTrue(session.allCookiesReceived());
+        assertEquals(STATE_AUTH_STARTED, session.getState());
+        verify(impl, never()).startPreparedClient(anyInt());
+
+        // First invocation should start the client monitor.
+        session.onDialogAnimatedIn();
+        assertEquals(STATE_AUTH_STARTED_UI_SHOWING, session.getState());
+        verify(impl).startPreparedClient(anyInt());
+
+        // Subsequent invocations should not start the client monitor again.
+        session.onDialogAnimatedIn();
+        session.onDialogAnimatedIn();
+        session.onDialogAnimatedIn();
+        assertEquals(STATE_AUTH_STARTED_UI_SHOWING, session.getState());
+        verify(impl, times(1)).startPreparedClient(anyInt());
+    }
+
     @Test
     public void testCancelAuthentication_whenStateAuthCalled_invokesCancel()
             throws RemoteException {

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

@@ -80,11 +80,14 @@ public class BiometricSchedulerOperationTest {
 
     private Handler mHandler;
     private BiometricSchedulerOperation mOperation;
+    private boolean mIsDebuggable;
 
     @Before
     public void setUp() {
         mHandler = new Handler(TestableLooper.get(this).getLooper());
-        mOperation = new BiometricSchedulerOperation(mClientMonitor, mClientCallback);
+        mIsDebuggable = false;
+        mOperation = new BiometricSchedulerOperation(mClientMonitor, mClientCallback,
+                () -> mIsDebuggable);
     }
 
     @Test
@@ -125,6 +128,34 @@ public class BiometricSchedulerOperationTest {
         verify(mClientMonitor, never()).start(any());
     }
 
+    @Test
+    public void testSecondStartWithCookieCrashesWhenDebuggable() {
+        final int cookie = 5;
+        mIsDebuggable = true;
+        when(mClientMonitor.getCookie()).thenReturn(cookie);
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        final boolean started = mOperation.startWithCookie(mOnStartCallback, cookie);
+        assertThat(started).isTrue();
+
+        assertThrows(IllegalStateException.class,
+                () -> mOperation.startWithCookie(mOnStartCallback, cookie));
+    }
+
+    @Test
+    public void testSecondStartWithCookieFailsNicelyWhenNotDebuggable() {
+        final int cookie = 5;
+        mIsDebuggable = false;
+        when(mClientMonitor.getCookie()).thenReturn(cookie);
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        final boolean started = mOperation.startWithCookie(mOnStartCallback, cookie);
+        assertThat(started).isTrue();
+
+        final boolean startedAgain = mOperation.startWithCookie(mOnStartCallback, cookie);
+        assertThat(startedAgain).isFalse();
+    }
+
     @Test
     public void startsWhenReadyAndHalAvailable() {
         when(mClientMonitor.getCookie()).thenReturn(0);
@@ -169,8 +200,35 @@ public class BiometricSchedulerOperationTest {
         verify(mOnStartCallback).onClientFinished(eq(mClientMonitor), eq(false));
     }
 
+    @Test
+    public void secondStartCrashesWhenDebuggable() {
+        mIsDebuggable = true;
+        when(mClientMonitor.getCookie()).thenReturn(0);
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        final boolean started = mOperation.start(mOnStartCallback);
+        assertThat(started).isTrue();
+
+        assertThrows(IllegalStateException.class, () -> mOperation.start(mOnStartCallback));
+    }
+
+    @Test
+    public void secondStartFailsNicelyWhenNotDebuggable() {
+        mIsDebuggable = false;
+        when(mClientMonitor.getCookie()).thenReturn(0);
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        final boolean started = mOperation.start(mOnStartCallback);
+        assertThat(started).isTrue();
+
+        final boolean startedAgain = mOperation.start(mOnStartCallback);
+        assertThat(startedAgain).isFalse();
+    }
+
     @Test
     public void doesNotStartWithCookie() {
+        // This class only throws exceptions when debuggable.
+        mIsDebuggable = true;
         when(mClientMonitor.getCookie()).thenReturn(9);
         assertThrows(IllegalStateException.class,
                 () -> mOperation.start(mock(ClientMonitorCallback.class)));
@@ -178,6 +236,8 @@ public class BiometricSchedulerOperationTest {
 
     @Test
     public void cannotRestart() {
+        // This class only throws exceptions when debuggable.
+        mIsDebuggable = true;
         when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
 
         mOperation.start(mOnStartCallback);
@@ -188,6 +248,8 @@ public class BiometricSchedulerOperationTest {
 
     @Test
     public void abortsNotRunning() {
+        // This class only throws exceptions when debuggable.
+        mIsDebuggable = true;
         when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
 
         mOperation.abort();
@@ -200,7 +262,8 @@ public class BiometricSchedulerOperationTest {
     }
 
     @Test
-    public void cannotAbortRunning() {
+    public void abortCrashesWhenDebuggableIfOperationIsRunning() {
+        mIsDebuggable = true;
         when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
 
         mOperation.start(mOnStartCallback);
@@ -208,6 +271,16 @@ public class BiometricSchedulerOperationTest {
         assertThrows(IllegalStateException.class, () -> mOperation.abort());
     }
 
+    @Test
+    public void abortFailsNicelyWhenNotDebuggableIfOperationIsRunning() {
+        mIsDebuggable = false;
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        mOperation.start(mOnStartCallback);
+
+        mOperation.abort();
+    }
+
     @Test
     public void cancel() {
         when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
@@ -253,6 +326,30 @@ public class BiometricSchedulerOperationTest {
         verify(mClientMonitor).destroy();
     }
 
+    @Test
+    public void cancelCrashesWhenDebuggableIfOperationIsFinished() {
+        mIsDebuggable = true;
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        mOperation.abort();
+        assertThat(mOperation.isFinished()).isTrue();
+
+        final ClientMonitorCallback cancelCb = mock(ClientMonitorCallback.class);
+        assertThrows(IllegalStateException.class, () -> mOperation.cancel(mHandler, cancelCb));
+    }
+
+    @Test
+    public void cancelFailsNicelyWhenNotDebuggableIfOperationIsFinished() {
+        mIsDebuggable = false;
+        when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);
+
+        mOperation.abort();
+        assertThat(mOperation.isFinished()).isTrue();
+
+        final ClientMonitorCallback cancelCb = mock(ClientMonitorCallback.class);
+        mOperation.cancel(mHandler, cancelCb);
+    }
+
     @Test
     public void markCanceling() {
         when(mClientMonitor.getFreshDaemon()).thenReturn(mHal);