From 257f2ecc97d294e95b069547466d2054926d960f Mon Sep 17 00:00:00 2001 From: Brian Colonna Date: Fri, 27 Apr 2012 13:14:22 -0400 Subject: [PATCH] Fix 6395288: Added lock to avoid unbind race condition If turning the power off while FUL was closing (due to a timeout or a cancel for example), it would be possible for unbind() to get called twice due to a race condition. Turning the phone off calls unbind() from the UI thread, while the other close comes from a binder thread since it is coming from the service. PATCH SET 1 attempted to solve the problem by adding a lock, but having a lock around the bind is a bit scary. PATCH SET 2 takes a new approach by having all calls coming from a binder thread to be sent as messages to be handled on the UI thread. Having all events occur on the UI thread removes the possibility of race conditions and makes the code stronger by making everything happen in a deterministic order. This commit also cleans up the logcat logging a bit. A couple of the log messages are now printing without DEBUG being set to true. This is by no means spamming the log and they of course are only logged when FUL is being used. But it serves to give us some meaningful information from bug reports that are currently showing nothing. The statements that are now logged would have made this particular bug easy to track down. Change-Id: I25a65c0808d88cb941439e5bf1f989dba8608be4 --- .../policy/impl/BiometricSensorUnlock.java | 2 +- .../internal/policy/impl/FaceUnlock.java | 337 +++++++++++++----- 2 files changed, 243 insertions(+), 96 deletions(-) diff --git a/policy/src/com/android/internal/policy/impl/BiometricSensorUnlock.java b/policy/src/com/android/internal/policy/impl/BiometricSensorUnlock.java index ad26845366f4b..a13ccc242c96b 100644 --- a/policy/src/com/android/internal/policy/impl/BiometricSensorUnlock.java +++ b/policy/src/com/android/internal/policy/impl/BiometricSensorUnlock.java @@ -62,7 +62,7 @@ interface BiometricSensorUnlock { public boolean start(); /** - * Stops the biometric unlock procedure and unbinds from the service. + * Stops the biometric unlock procedure and unbinds from the service. Called on the UI thread. * @return whether the biometric unlock was running when called. */ public boolean stop(); diff --git a/policy/src/com/android/internal/policy/impl/FaceUnlock.java b/policy/src/com/android/internal/policy/impl/FaceUnlock.java index 16d4003354d29..6e09b7f623759 100644 --- a/policy/src/com/android/internal/policy/impl/FaceUnlock.java +++ b/policy/src/com/android/internal/policy/impl/FaceUnlock.java @@ -45,32 +45,44 @@ public class FaceUnlock implements BiometricSensorUnlock, Handler.Callback { // TODO: is mServiceRunning needed or can we just use mIsRunning or check if mService is null? private boolean mServiceRunning = false; + // TODO: now that the code has been restructure to do almost all operations from a handler, this + // lock may no longer be necessary. private final Object mServiceRunningLock = new Object(); private IFaceLockInterface mService; private boolean mBoundToService = false; private View mFaceUnlockView; private Handler mHandler; - private final int MSG_SHOW_AREA_VIEW = 0; - private final int MSG_HIDE_AREA_VIEW = 1; + private final int MSG_SHOW_FACE_UNLOCK_VIEW = 0; + private final int MSG_HIDE_FACE_UNLOCK_VIEW = 1; + private final int MSG_SERVICE_CONNECTED = 2; + private final int MSG_SERVICE_DISCONNECTED = 3; + private final int MSG_UNLOCK = 4; + private final int MSG_CANCEL = 5; + private final int MSG_REPORT_FAILED_ATTEMPT = 6; + private final int MSG_EXPOSE_FALLBACK = 7; + private final int MSG_POKE_WAKELOCK = 8; // TODO: This was added for the purpose of adhering to what the biometric interface expects // the isRunning() function to return. However, it is probably not necessary to have both // mRunning and mServiceRunning. I'd just rather wait to change that logic. - private boolean mIsRunning = false; + private volatile boolean mIsRunning = false; // Long enough to stay visible while the service starts // Short enough to not have to wait long for backup if service fails to start or crashes // The service can take a couple of seconds to start on the first try after boot - private final int VIEW_AREA_SERVICE_TIMEOUT = 3000; + private final int SERVICE_STARTUP_VIEW_TIMEOUT = 3000; // So the user has a consistent amount of time when brought to the backup method from Face // Unlock private final int BACKUP_LOCK_TIMEOUT = 5000; - KeyguardScreenCallback mKeyguardScreenCallback; + /** + * Stores some of the structures that Face Unlock will need to access and creates the handler + * will be used to execute messages on the UI thread. + */ public FaceUnlock(Context context, KeyguardUpdateMonitor updateMonitor, LockPatternUtils lockPatternUtils, KeyguardScreenCallback keyguardScreenCallback) { mContext = context; @@ -86,6 +98,7 @@ public class FaceUnlock implements BiometricSensorUnlock, Handler.Callback { * methods, we will have to add our other views (background, cancel button) here. */ public void initializeView(View biometricUnlockView) { + Log.d(TAG, "initializeView()"); mFaceUnlockView = biometricUnlockView; show(0); } @@ -102,12 +115,13 @@ public class FaceUnlock implements BiometricSensorUnlock, Handler.Callback { * timeoutMillis is 0, no hide is performed. */ public void show(long timeoutMillis) { - removeAreaDisplayMessages(); + if (DEBUG) Log.d(TAG, "show()"); + removeDisplayMessages(); if (mFaceUnlockView != null) { mFaceUnlockView.setVisibility(View.VISIBLE); } if (timeoutMillis > 0) { - mHandler.sendEmptyMessageDelayed(MSG_HIDE_AREA_VIEW, timeoutMillis); + mHandler.sendEmptyMessageDelayed(MSG_HIDE_FACE_UNLOCK_VIEW, timeoutMillis); } } @@ -115,16 +129,18 @@ public class FaceUnlock implements BiometricSensorUnlock, Handler.Callback { * Hides the Face Unlock view. */ public void hide() { + if (DEBUG) Log.d(TAG, "hide()"); // Remove messages to prevent a delayed show message from undo-ing the hide - removeAreaDisplayMessages(); - mHandler.sendEmptyMessage(MSG_HIDE_AREA_VIEW); + removeDisplayMessages(); + mHandler.sendEmptyMessage(MSG_HIDE_FACE_UNLOCK_VIEW); } /** * Binds to the Face Unlock service. Face Unlock will be started when the bind completes. The - * Face Unlock area is displayed to hide the backup while the service is starting up. + * Face Unlock view is displayed to hide the backup lock while the service is starting up. */ public boolean start() { + if (DEBUG) Log.d(TAG, "start()"); if (mIsRunning) { Log.w(TAG, "start() called when already running"); } @@ -132,14 +148,13 @@ public class FaceUnlock implements BiometricSensorUnlock, Handler.Callback { // Show Face Unlock view, but only for a little bit so lockpattern will become visible if // Face Unlock fails to start or crashes // This must show before bind to guarantee that Face Unlock has a place to display - show(VIEW_AREA_SERVICE_TIMEOUT); + show(SERVICE_STARTUP_VIEW_TIMEOUT); if (!mBoundToService) { - if (DEBUG) Log.d(TAG, "before bind to Face Unlock service"); + Log.d(TAG, "Binding to Face Unlock service"); mContext.bindService(new Intent(IFaceLockInterface.class.getName()), mConnection, Context.BIND_AUTO_CREATE, mLockPatternUtils.getCurrentUser()); - if (DEBUG) Log.d(TAG, "after bind to Face Unlock service"); mBoundToService = true; } else { Log.w(TAG, "Attempt to bind to Face Unlock when already bound"); @@ -158,11 +173,11 @@ public class FaceUnlock implements BiometricSensorUnlock, Handler.Callback { * Stops Face Unlock and unbinds from the service. */ public boolean stop() { + if (DEBUG) Log.d(TAG, "stop()"); boolean mWasRunning = mIsRunning; stopUi(); if (mBoundToService) { - if (DEBUG) Log.d(TAG, "before unbind from Face Unlock service"); if (mService != null) { try { mService.unregisterCallback(mFaceUnlockCallback); @@ -170,8 +185,8 @@ public class FaceUnlock implements BiometricSensorUnlock, Handler.Callback { // Not much we can do } } + Log.d(TAG, "Unbinding from Face Unlock service"); mContext.unbindService(mConnection); - if (DEBUG) Log.d(TAG, "after unbind from Face Unlock service"); mBoundToService = false; } else { // This is usually not an error when this happens. Sometimes we will tell it to @@ -187,6 +202,7 @@ public class FaceUnlock implements BiometricSensorUnlock, Handler.Callback { * Frees up resources used by Face Unlock and stops it if it is still running. */ public void cleanUp() { + if (DEBUG) Log.d(TAG, "cleanUp()"); if (mService != null) { try { mService.unregisterCallback(mFaceUnlockCallback); @@ -206,86 +222,224 @@ public class FaceUnlock implements BiometricSensorUnlock, Handler.Callback { } /** - * Handles showing the Face Unlock view (hiding the backup lock) and hiding the Face Unlock view - * (exposing the backup lock). In cases where 'show' needs to happen immediately, - * setVisibility() is called directly (without using this handler). This handler is used when - * 'show' needs to happen from a non-UI thread. It also handles hide() messages since they - * often require a delay. + * Handles messages such that everything happens on the UI thread in a deterministic order. + * Calls from the Face Unlock service come from binder threads. Calls from lockscreen typically + * come from the UI thread. This makes sure there are no race conditions between those calls. */ @Override public boolean handleMessage(Message msg) { switch (msg.what) { - case MSG_SHOW_AREA_VIEW: - if (mFaceUnlockView != null) { - mFaceUnlockView.setVisibility(View.VISIBLE); - } - break; - case MSG_HIDE_AREA_VIEW: - if (mFaceUnlockView != null) { - mFaceUnlockView.setVisibility(View.INVISIBLE); - } - break; - default: - Log.w(TAG, "Unhandled message"); - return false; + case MSG_SHOW_FACE_UNLOCK_VIEW: + handleShowFaceUnlockView(); + break; + case MSG_HIDE_FACE_UNLOCK_VIEW: + handleHideFaceUnlockView(); + break; + case MSG_SERVICE_CONNECTED: + handleServiceConnected(); + break; + case MSG_SERVICE_DISCONNECTED: + handleServiceDisconnected(); + break; + case MSG_UNLOCK: + handleUnlock(); + break; + case MSG_CANCEL: + handleCancel(); + break; + case MSG_REPORT_FAILED_ATTEMPT: + handleReportFailedAttempt(); + break; + case MSG_EXPOSE_FALLBACK: + handleExposeFallback(); + break; + case MSG_POKE_WAKELOCK: + handlePokeWakelock(); + break; + default: + Log.e(TAG, "Unhandled message"); + return false; } return true; } /** - * Removes show and hide messages from the message queue + * Sets the Face Unlock view to visible, thus covering the backup lock. */ - private void removeAreaDisplayMessages() { - mHandler.removeMessages(MSG_SHOW_AREA_VIEW); - mHandler.removeMessages(MSG_HIDE_AREA_VIEW); + void handleShowFaceUnlockView() { + if (DEBUG) Log.d(TAG, "handleShowFaceUnlockView()"); + if (mFaceUnlockView != null) { + mFaceUnlockView.setVisibility(View.VISIBLE); + } else { + Log.e(TAG, "mFaceUnlockView is null in handleShowFaceUnlockView()"); + } } - private ServiceConnection mConnection = new ServiceConnection() { - /** - * Completes connection, registers callback, and starts Face Unlock when service is bound - */ - @Override - public void onServiceConnected(ComponentName className, IBinder iservice) { - mService = IFaceLockInterface.Stub.asInterface(iservice); - if (DEBUG) Log.d(TAG, "Connected to Face Unlock service"); - try { - mService.registerCallback(mFaceUnlockCallback); - } catch (RemoteException e) { - Log.e(TAG, "Caught exception connecting to Face Unlock: " + e.toString()); - mService = null; - mBoundToService = false; - mIsRunning = false; - return; - } + /** + * Sets the Face Unlock view to invisible, thus exposing the backup lock. + */ + void handleHideFaceUnlockView() { + if (DEBUG) Log.d(TAG, "handleHideFaceUnlockView()"); + if (mFaceUnlockView != null) { + mFaceUnlockView.setVisibility(View.INVISIBLE); + } else { + Log.e(TAG, "mFaceUnlockView is null in handleHideFaceUnlockView()"); + } + } - if (mFaceUnlockView != null) { + /** + * Tells the service to start its UI via an AIDL interface. Called when the + * onServiceConnected() callback is received. + */ + void handleServiceConnected() { + if (DEBUG) Log.d(TAG, "handleServiceConnected()"); + try { + mService.registerCallback(mFaceUnlockCallback); + } catch (RemoteException e) { + Log.e(TAG, "Caught exception connecting to Face Unlock: " + e.toString()); + mService = null; + mBoundToService = false; + mIsRunning = false; + return; + } + + if (mFaceUnlockView != null) { + IBinder windowToken = mFaceUnlockView.getWindowToken(); + if (windowToken != null) { int[] position; position = new int[2]; mFaceUnlockView.getLocationInWindow(position); - startUi(mFaceUnlockView.getWindowToken(), position[0], position[1], - mFaceUnlockView.getWidth(), mFaceUnlockView.getHeight()); + startUi(windowToken, position[0], position[1], mFaceUnlockView.getWidth(), + mFaceUnlockView.getHeight()); + } else { + Log.e(TAG, "windowToken is null in handleServiceConnected()"); } } + } + + /** + * Called when the onServiceDisconnected() callback is received. This should not happen during + * normal operation. It indicates an error has occurred. + */ + void handleServiceDisconnected() { + Log.e(TAG, "handleServiceDisconnected()"); + // TODO: this lock may no longer be needed now that everything is being called from a + // handler + synchronized (mServiceRunningLock) { + mService = null; + mServiceRunning = false; + } + mBoundToService = false; + mIsRunning = false; + } + + /** + * Stops the Face Unlock service and tells the device to grant access to the user. Shows the + * Face Unlock view to keep the backup lock covered while the device unlocks. + */ + void handleUnlock() { + if (DEBUG) Log.d(TAG, "handleUnlock()"); + removeDisplayMessages(); + if (mFaceUnlockView != null) { + mFaceUnlockView.setVisibility(View.VISIBLE); + } else { + Log.e(TAG, "mFaceUnlockView is null in handleUnlock()"); + } + stop(); + mKeyguardScreenCallback.keyguardDone(true); + mKeyguardScreenCallback.reportSuccessfulUnlockAttempt(); + } + + /** + * Stops the Face Unlock service and exposes the backup lock. Called when the user presses the + * cancel button to skip Face Unlock or no face is detected. + */ + void handleCancel() { + if (DEBUG) Log.d(TAG, "handleCancel()"); + if (mFaceUnlockView != null) { + mFaceUnlockView.setVisibility(View.INVISIBLE); + } else { + Log.e(TAG, "mFaceUnlockView is null in handleCancel()"); + } + stop(); + mKeyguardScreenCallback.pokeWakelock(BACKUP_LOCK_TIMEOUT); + } + + /** + * Stops the Face Unlock service and exposes the backup lock, reporting a failed unlock attempt. + * Called when Face Unlock denies access to the user. + */ + void handleReportFailedAttempt() { + if (DEBUG) Log.d(TAG, "handleReportFailedAttempt()"); + mUpdateMonitor.reportFailedBiometricUnlockAttempt(); + if (mFaceUnlockView != null) { + mFaceUnlockView.setVisibility(View.INVISIBLE); + } else { + Log.e(TAG, "mFaceUnlockView is null in handleReportFailedAttempt()"); + } + stop(); + mKeyguardScreenCallback.pokeWakelock(BACKUP_LOCK_TIMEOUT); + } + + /** + * Hides the Face Unlock view to expose the backup lock. Called when the Face Unlock service UI + * is started, indicating there is no need to continue displaying the underlying view because + * the service UI is now covering the backup lock. + */ + void handleExposeFallback() { + if (DEBUG) Log.d(TAG, "handleExposeFallback()"); + if (mFaceUnlockView != null) { + mFaceUnlockView.setVisibility(View.INVISIBLE); + } else { + Log.e(TAG, "mFaceUnlockView is null in handleExposeFallback()"); + } + } + + /** + * Pokes the wakelock to keep the screen alive and active. + */ + void handlePokeWakelock() { + mKeyguardScreenCallback.pokeWakelock(); + } + + /** + * Removes show and hide messages from the message queue. Called to prevent delayed show/hide + * messages from undoing a new message. + */ + private void removeDisplayMessages() { + mHandler.removeMessages(MSG_SHOW_FACE_UNLOCK_VIEW); + mHandler.removeMessages(MSG_HIDE_FACE_UNLOCK_VIEW); + } + + /** + * Implements service connection methods. + */ + private ServiceConnection mConnection = new ServiceConnection() { + /** + * Called when the Face Unlock service connects after calling bind(). + */ + @Override + public void onServiceConnected(ComponentName className, IBinder iservice) { + Log.d(TAG, "Connected to Face Unlock service"); + mService = IFaceLockInterface.Stub.asInterface(iservice); + mHandler.sendEmptyMessage(MSG_SERVICE_CONNECTED); + } /** - * Cleans up if Face Unlock service unexpectedly disconnects + * Called if the Face Unlock service unexpectedly disconnects. This indicates an error. */ @Override public void onServiceDisconnected(ComponentName className) { - synchronized(mServiceRunningLock) { - mService = null; - mServiceRunning = false; - } - mBoundToService = false; - mIsRunning = false; - Log.w(TAG, "Unexpected disconnect from Face Unlock service"); + Log.e(TAG, "Unexpected disconnect from Face Unlock service"); + mHandler.sendEmptyMessage(MSG_SERVICE_DISCONNECTED); } }; /** - * Tells the Face Unlock service to start displaying its UI and perform recognition + * Tells the Face Unlock service to start displaying its UI and start processing. */ private void startUi(IBinder windowToken, int x, int y, int w, int h) { + if (DEBUG) Log.d(TAG, "startUi()"); synchronized (mServiceRunningLock) { if (!mServiceRunning) { if (DEBUG) Log.d(TAG, "Starting Face Unlock"); @@ -298,93 +452,86 @@ public class FaceUnlock implements BiometricSensorUnlock, Handler.Callback { } mServiceRunning = true; } else { - if (DEBUG) Log.w(TAG, "startUi() attempted while running"); + Log.w(TAG, "startUi() attempted while running"); } } } /** - * Tells the Face Unlock service to stop displaying its UI and stop recognition + * Tells the Face Unlock service to stop displaying its UI and stop processing. */ private void stopUi() { + if (DEBUG) Log.d(TAG, "stopUi()"); // Note that attempting to stop Face Unlock when it's not running is not an issue. // Face Unlock can return, which stops it and then we try to stop it when the // screen is turned off. That's why we check. synchronized (mServiceRunningLock) { if (mServiceRunning) { + if (DEBUG) Log.d(TAG, "Stopping Face Unlock"); try { - if (DEBUG) Log.d(TAG, "Stopping Face Unlock"); mService.stopUi(); } catch (RemoteException e) { Log.e(TAG, "Caught exception stopping Face Unlock: " + e.toString()); } mServiceRunning = false; + } else { + // This is usually not an error when this happens. Sometimes we will tell it to + // stop multiple times because it's called from both onWindowFocusChanged and + // onDetachedFromWindow. + if (DEBUG) Log.d(TAG, "stopUi() attempted while not running"); } } } /** - * Implements the biometric unlock service callback interface defined in AIDL + * Implements the AIDL biometric unlock service callback interface. */ private final IFaceLockCallback mFaceUnlockCallback = new IFaceLockCallback.Stub() { /** - * Stops the Face Unlock UI and indicates that the phone should be unlocked + * Called when Face Unlock wants to grant access to the user. */ @Override public void unlock() { if (DEBUG) Log.d(TAG, "unlock()"); - - // Keep fallback covered - removeAreaDisplayMessages(); - mHandler.sendEmptyMessage(MSG_SHOW_AREA_VIEW); - - stop(); - - mKeyguardScreenCallback.keyguardDone(true); - mKeyguardScreenCallback.reportSuccessfulUnlockAttempt(); + mHandler.sendEmptyMessage(MSG_UNLOCK); } /** - * Stops the Face Unlock UI and exposes the backup method without unlocking - * This means the user has cancelled out + * Called when the user presses cancel to skip Face Unlock or a face cannot be found. */ @Override public void cancel() { if (DEBUG) Log.d(TAG, "cancel()"); - hide(); // Expose fallback - stop(); - mKeyguardScreenCallback.pokeWakelock(BACKUP_LOCK_TIMEOUT); + mHandler.sendEmptyMessage(MSG_CANCEL); } /** - * Stops the Face Unlock UI and exposes the backup method without unlocking - * This means Face Unlock failed to recognize them + * Called when Face Unlock denies access to the user. */ @Override public void reportFailedAttempt() { if (DEBUG) Log.d(TAG, "reportFailedAttempt()"); - mUpdateMonitor.reportFailedBiometricUnlockAttempt(); - hide(); // Expose fallback - stop(); - mKeyguardScreenCallback.pokeWakelock(BACKUP_LOCK_TIMEOUT); + mHandler.sendEmptyMessage(MSG_REPORT_FAILED_ATTEMPT); } /** - * Removes the black area that covers the backup unlock method + * Called when the Face Unlock service starts displaying the UI, indicating that the backup + * unlock can be exposed because the Face Unlock service is now covering the backup with its + * UI. **/ @Override public void exposeFallback() { if (DEBUG) Log.d(TAG, "exposeFallback()"); - hide(); // Expose fallback + mHandler.sendEmptyMessage(MSG_EXPOSE_FALLBACK); } /** - * Allows the Face Unlock service to poke the wake lock to keep the lockscreen alive + * Called when Face Unlock wants to keep the screen alive and active. */ @Override public void pokeWakelock() { if (DEBUG) Log.d(TAG, "pokeWakelock()"); - mKeyguardScreenCallback.pokeWakelock(); + mHandler.sendEmptyMessage(MSG_POKE_WAKELOCK); } }; }