From bd4cd38772f2881b2bda97e74d6435de202acab2 Mon Sep 17 00:00:00 2001 From: Joe Bolinger Date: Thu, 13 Jan 2022 12:57:50 -0800 Subject: [PATCH] Check current user when the operations runs for fingerprint hidl. The current and target user are both provided to the switch client constructor which is brittle unless operations are scheduled and run synchronously. Commit 9a99503870074d40d794245d9e3ac7a076f5b2f2 changed the handlers and somehow caused a bug where the cached current user was out of sync. The switch client tries to optimize for this and can skip switching when that occurs. This also includes two additonal changes 1) a few updated logs from the original change and 2) restores each scheduling having its own handler for clarity. Bug: 213962104 Test: atest UserAwareBiometricSchedulerTest BiometricSchedulerTest BiometricSchedulerOperationTest SensorTest Face10Test Test: manual (flash, enroll fingerprint, add work profile account, reboot, verify fingerprint still works) Change-Id: Ifb73b0145aeb8afb62d1f55d2f881347b0d2ef8a --- .../biometrics/sensors/BiometricScheduler.java | 11 +++++------ .../sensors/BiometricSchedulerOperation.java | 8 +++++--- .../sensors/UserAwareBiometricScheduler.java | 6 +++--- .../biometrics/sensors/face/aidl/Sensor.java | 2 +- .../biometrics/sensors/face/hidl/Face10.java | 4 +++- .../sensors/fingerprint/aidl/Sensor.java | 2 +- .../sensors/fingerprint/hidl/Fingerprint21.java | 12 +++++++++--- .../fingerprint/hidl/Fingerprint21UdfpsMock.java | 3 +-- .../hidl/FingerprintUpdateActiveUserClient.java | 16 ++++++++++------ .../biometrics/sensors/face/aidl/SensorTest.java | 5 ++++- .../sensors/fingerprint/aidl/SensorTest.java | 5 ++++- 11 files changed, 46 insertions(+), 28 deletions(-) diff --git a/services/core/java/com/android/server/biometrics/sensors/BiometricScheduler.java b/services/core/java/com/android/server/biometrics/sensors/BiometricScheduler.java index 1f91c4d6803e8..39c5944d65c73 100644 --- a/services/core/java/com/android/server/biometrics/sensors/BiometricScheduler.java +++ b/services/core/java/com/android/server/biometrics/sensors/BiometricScheduler.java @@ -25,6 +25,7 @@ import android.hardware.biometrics.IBiometricService; import android.hardware.fingerprint.FingerprintSensorPropertiesInternal; import android.os.Handler; import android.os.IBinder; +import android.os.Looper; import android.os.RemoteException; import android.os.ServiceManager; import android.util.Slog; @@ -232,17 +233,14 @@ public class BiometricScheduler { * Creates a new scheduler. * * @param tag for the specific instance of the scheduler. Should be unique. - * @param handler handler for callbacks (all methods of this class must be called on the - * thread associated with this handler) * @param sensorType the sensorType that this scheduler is handling. * @param gestureAvailabilityDispatcher may be null if the sensor does not support gestures * (such as fingerprint swipe). */ public BiometricScheduler(@NonNull String tag, - @NonNull Handler handler, @SensorType int sensorType, @Nullable GestureAvailabilityDispatcher gestureAvailabilityDispatcher) { - this(tag, handler, sensorType, gestureAvailabilityDispatcher, + this(tag, new Handler(Looper.getMainLooper()), sensorType, gestureAvailabilityDispatcher, IBiometricService.Stub.asInterface( ServiceManager.getService(Context.BIOMETRIC_SERVICE)), LOG_NUM_RECENT_OPERATIONS, CoexCoordinator.getInstance()); @@ -376,8 +374,9 @@ public class BiometricScheduler { // send ERROR_CANCELED and skip the operation. if (clientMonitor.interruptsPrecedingClients()) { for (BiometricSchedulerOperation operation : mPendingOperations) { - Slog.d(getTag(), "New client, marking pending op as canceling: " + operation); - operation.markCanceling(); + if (operation.markCanceling()) { + Slog.d(getTag(), "New client, marking pending op as canceling: " + operation); + } } } diff --git a/services/core/java/com/android/server/biometrics/sensors/BiometricSchedulerOperation.java b/services/core/java/com/android/server/biometrics/sensors/BiometricSchedulerOperation.java index a8cce153dc706..e8b50d90b5865 100644 --- a/services/core/java/com/android/server/biometrics/sensors/BiometricSchedulerOperation.java +++ b/services/core/java/com/android/server/biometrics/sensors/BiometricSchedulerOperation.java @@ -225,12 +225,13 @@ public class BiometricSchedulerOperation { Slog.v(TAG, "Aborted: " + this); } - /** Flags this operation as canceled, but does not cancel it until started. */ - public void markCanceling() { + /** Flags this operation as canceled, if possible, but does not cancel it until started. */ + public boolean markCanceling() { if (mState == STATE_WAITING_IN_QUEUE && isInterruptable()) { mState = STATE_WAITING_IN_QUEUE_CANCELING; - Slog.v(TAG, "Marked cancelling: " + this); + return true; } + return false; } /** @@ -280,6 +281,7 @@ public class BiometricSchedulerOperation { @Override public void onClientFinished(@NonNull BaseClientMonitor clientMonitor, boolean success) { + Slog.d(TAG, "[Finished / destroy]: " + clientMonitor); mClientMonitor.destroy(); mState = STATE_FINISHED; } diff --git a/services/core/java/com/android/server/biometrics/sensors/UserAwareBiometricScheduler.java b/services/core/java/com/android/server/biometrics/sensors/UserAwareBiometricScheduler.java index 19eaa178c7c9f..603cc22968a99 100644 --- a/services/core/java/com/android/server/biometrics/sensors/UserAwareBiometricScheduler.java +++ b/services/core/java/com/android/server/biometrics/sensors/UserAwareBiometricScheduler.java @@ -23,6 +23,7 @@ import android.annotation.Nullable; import android.content.Context; import android.hardware.biometrics.IBiometricService; import android.os.Handler; +import android.os.Looper; import android.os.ServiceManager; import android.os.UserHandle; import android.util.Slog; @@ -85,7 +86,7 @@ public class UserAwareBiometricScheduler extends BiometricScheduler { } @VisibleForTesting - UserAwareBiometricScheduler(@NonNull String tag, + public UserAwareBiometricScheduler(@NonNull String tag, @NonNull Handler handler, @SensorType int sensorType, @Nullable GestureAvailabilityDispatcher gestureAvailabilityDispatcher, @@ -101,12 +102,11 @@ public class UserAwareBiometricScheduler extends BiometricScheduler { } public UserAwareBiometricScheduler(@NonNull String tag, - @NonNull Handler handler, @SensorType int sensorType, @Nullable GestureAvailabilityDispatcher gestureAvailabilityDispatcher, @NonNull CurrentUserRetriever currentUserRetriever, @NonNull UserSwitchCallback userSwitchCallback) { - this(tag, handler, sensorType, gestureAvailabilityDispatcher, + this(tag, new Handler(Looper.getMainLooper()), sensorType, gestureAvailabilityDispatcher, IBiometricService.Stub.asInterface( ServiceManager.getService(Context.BIOMETRIC_SERVICE)), currentUserRetriever, userSwitchCallback, CoexCoordinator.getInstance()); diff --git a/services/core/java/com/android/server/biometrics/sensors/face/aidl/Sensor.java b/services/core/java/com/android/server/biometrics/sensors/face/aidl/Sensor.java index 39270430c21d4..206b8f0779e8e 100644 --- a/services/core/java/com/android/server/biometrics/sensors/face/aidl/Sensor.java +++ b/services/core/java/com/android/server/biometrics/sensors/face/aidl/Sensor.java @@ -494,7 +494,7 @@ public class Sensor { mToken = new Binder(); mHandler = handler; mSensorProperties = sensorProperties; - mScheduler = new UserAwareBiometricScheduler(tag, mHandler, + mScheduler = new UserAwareBiometricScheduler(tag, BiometricScheduler.SENSOR_TYPE_FACE, null /* gestureAvailabilityDispatcher */, () -> mCurrentSession != null ? mCurrentSession.mUserId : UserHandle.USER_NULL, new UserAwareBiometricScheduler.UserSwitchCallback() { diff --git a/services/core/java/com/android/server/biometrics/sensors/face/hidl/Face10.java b/services/core/java/com/android/server/biometrics/sensors/face/hidl/Face10.java index 493c0a05e3795..e957794372aac 100644 --- a/services/core/java/com/android/server/biometrics/sensors/face/hidl/Face10.java +++ b/services/core/java/com/android/server/biometrics/sensors/face/hidl/Face10.java @@ -363,7 +363,7 @@ public class Face10 implements IHwBinder.DeathRecipient, ServiceProvider { @NonNull LockoutResetDispatcher lockoutResetDispatcher) { final Handler handler = new Handler(Looper.getMainLooper()); return new Face10(context, sensorProps, lockoutResetDispatcher, handler, - new BiometricScheduler(TAG, handler, BiometricScheduler.SENSOR_TYPE_FACE, + new BiometricScheduler(TAG, BiometricScheduler.SENSOR_TYPE_FACE, null /* gestureAvailabilityTracker */)); } @@ -896,6 +896,8 @@ public class Face10 implements IHwBinder.DeathRecipient, ServiceProvider { boolean success) { if (success) { mCurrentUserId = targetUserId; + } else { + Slog.w(TAG, "Failed to change user, still: " + mCurrentUserId); } } }); diff --git a/services/core/java/com/android/server/biometrics/sensors/fingerprint/aidl/Sensor.java b/services/core/java/com/android/server/biometrics/sensors/fingerprint/aidl/Sensor.java index 256761a61a72a..59e4b582ca84e 100644 --- a/services/core/java/com/android/server/biometrics/sensors/fingerprint/aidl/Sensor.java +++ b/services/core/java/com/android/server/biometrics/sensors/fingerprint/aidl/Sensor.java @@ -449,7 +449,7 @@ class Sensor { mHandler = handler; mSensorProperties = sensorProperties; mLockoutCache = new LockoutCache(); - mScheduler = new UserAwareBiometricScheduler(tag, handler, + mScheduler = new UserAwareBiometricScheduler(tag, BiometricScheduler.sensorTypeFromFingerprintProperties(mSensorProperties), gestureAvailabilityDispatcher, () -> mCurrentSession != null ? mCurrentSession.mUserId : UserHandle.USER_NULL, diff --git a/services/core/java/com/android/server/biometrics/sensors/fingerprint/hidl/Fingerprint21.java b/services/core/java/com/android/server/biometrics/sensors/fingerprint/hidl/Fingerprint21.java index d352cda609e3d..6feb5fa418bbc 100644 --- a/services/core/java/com/android/server/biometrics/sensors/fingerprint/hidl/Fingerprint21.java +++ b/services/core/java/com/android/server/biometrics/sensors/fingerprint/hidl/Fingerprint21.java @@ -360,7 +360,7 @@ public class Fingerprint21 implements IHwBinder.DeathRecipient, ServiceProvider @NonNull LockoutResetDispatcher lockoutResetDispatcher, @NonNull GestureAvailabilityDispatcher gestureAvailabilityDispatcher) { final BiometricScheduler scheduler = - new BiometricScheduler(TAG, handler, + new BiometricScheduler(TAG, BiometricScheduler.sensorTypeFromFingerprintProperties(sensorProps), gestureAvailabilityDispatcher); final HalResultController controller = new HalResultController(sensorProps.sensorId, @@ -490,19 +490,25 @@ public class Fingerprint21 implements IHwBinder.DeathRecipient, ServiceProvider !getEnrolledFingerprints(mSensorProperties.sensorId, targetUserId).isEmpty(); final FingerprintUpdateActiveUserClient client = new FingerprintUpdateActiveUserClient(mContext, mLazyDaemon, targetUserId, - mContext.getOpPackageName(), mSensorProperties.sensorId, mCurrentUserId, - hasEnrolled, mAuthenticatorIds, force); + mContext.getOpPackageName(), mSensorProperties.sensorId, + this::getCurrentUser, hasEnrolled, mAuthenticatorIds, force); mScheduler.scheduleClientMonitor(client, new BaseClientMonitor.Callback() { @Override public void onClientFinished(@NonNull BaseClientMonitor clientMonitor, boolean success) { if (success) { mCurrentUserId = targetUserId; + } else { + Slog.w(TAG, "Failed to change user, still: " + mCurrentUserId); } } }); } + private int getCurrentUser() { + return mCurrentUserId; + } + @Override public boolean containsSensor(int sensorId) { return mSensorProperties.sensorId == sensorId; diff --git a/services/core/java/com/android/server/biometrics/sensors/fingerprint/hidl/Fingerprint21UdfpsMock.java b/services/core/java/com/android/server/biometrics/sensors/fingerprint/hidl/Fingerprint21UdfpsMock.java index 20dab5552df98..273f8a545db55 100644 --- a/services/core/java/com/android/server/biometrics/sensors/fingerprint/hidl/Fingerprint21UdfpsMock.java +++ b/services/core/java/com/android/server/biometrics/sensors/fingerprint/hidl/Fingerprint21UdfpsMock.java @@ -138,8 +138,7 @@ public class Fingerprint21UdfpsMock extends Fingerprint21 implements TrustManage TestableBiometricScheduler(@NonNull String tag, @NonNull Handler handler, @Nullable GestureAvailabilityDispatcher gestureAvailabilityDispatcher) { - super(tag, handler, BiometricScheduler.SENSOR_TYPE_FP_OTHER, - gestureAvailabilityDispatcher); + super(tag, BiometricScheduler.SENSOR_TYPE_FP_OTHER, gestureAvailabilityDispatcher); } void init(@NonNull Fingerprint21UdfpsMock fingerprint21) { diff --git a/services/core/java/com/android/server/biometrics/sensors/fingerprint/hidl/FingerprintUpdateActiveUserClient.java b/services/core/java/com/android/server/biometrics/sensors/fingerprint/hidl/FingerprintUpdateActiveUserClient.java index fd38bdd1201eb..a2c18923c00e8 100644 --- a/services/core/java/com/android/server/biometrics/sensors/fingerprint/hidl/FingerprintUpdateActiveUserClient.java +++ b/services/core/java/com/android/server/biometrics/sensors/fingerprint/hidl/FingerprintUpdateActiveUserClient.java @@ -31,6 +31,7 @@ import com.android.server.biometrics.sensors.HalClientMonitor; import java.io.File; import java.util.Map; +import java.util.function.Supplier; /** * Sets the HAL's current active user, and updates the framework's authenticatorId cache. @@ -40,7 +41,7 @@ public class FingerprintUpdateActiveUserClient extends HalClientMonitor mCurrentUserId; private final boolean mForceUpdateAuthenticatorId; private final boolean mHasEnrolledBiometrics; private final Map mAuthenticatorIds; @@ -48,8 +49,9 @@ public class FingerprintUpdateActiveUserClient extends HalClientMonitor lazyDaemon, int userId, - @NonNull String owner, int sensorId, int currentUserId, boolean hasEnrolledBiometrics, - @NonNull Map authenticatorIds, boolean forceUpdateAuthenticatorId) { + @NonNull String owner, int sensorId, Supplier currentUserId, + boolean hasEnrolledBiometrics, @NonNull Map authenticatorIds, + boolean forceUpdateAuthenticatorId) { super(context, lazyDaemon, null /* token */, null /* listener */, userId, owner, 0 /* cookie */, sensorId, BiometricsProtoEnums.MODALITY_UNKNOWN, BiometricsProtoEnums.ACTION_UNKNOWN, BiometricsProtoEnums.CLIENT_UNKNOWN); @@ -63,7 +65,7 @@ public class FingerprintUpdateActiveUserClient extends HalClientMonitor USER_ID, - mUserSwitchCallback); + mUserSwitchCallback, + CoexCoordinator.getInstance()); mHalCallback = new Sensor.HalSessionCallback(mContext, new Handler(mLooper.getLooper()), TAG, mScheduler, SENSOR_ID, USER_ID, mLockoutCache, mLockoutResetDispatcher, mHalSessionCallback); diff --git a/services/tests/servicestests/src/com/android/server/biometrics/sensors/fingerprint/aidl/SensorTest.java b/services/tests/servicestests/src/com/android/server/biometrics/sensors/fingerprint/aidl/SensorTest.java index a012b8b06c7f5..d4609b55afba7 100644 --- a/services/tests/servicestests/src/com/android/server/biometrics/sensors/fingerprint/aidl/SensorTest.java +++ b/services/tests/servicestests/src/com/android/server/biometrics/sensors/fingerprint/aidl/SensorTest.java @@ -33,6 +33,7 @@ import android.platform.test.annotations.Presubmit; import androidx.test.filters.SmallTest; import com.android.server.biometrics.sensors.BiometricScheduler; +import com.android.server.biometrics.sensors.CoexCoordinator; import com.android.server.biometrics.sensors.LockoutCache; import com.android.server.biometrics.sensors.LockoutResetDispatcher; import com.android.server.biometrics.sensors.LockoutTracker; @@ -82,8 +83,10 @@ public class SensorTest { new Handler(mLooper.getLooper()), BiometricScheduler.SENSOR_TYPE_FP_OTHER, null /* gestureAvailabilityDispatcher */, + mBiometricService, () -> USER_ID, - mUserSwitchCallback); + mUserSwitchCallback, + CoexCoordinator.getInstance()); mHalCallback = new Sensor.HalSessionCallback(mContext, new Handler(mLooper.getLooper()), TAG, mScheduler, SENSOR_ID, USER_ID, mLockoutCache, mLockoutResetDispatcher, mHalSessionCallback);