From f73795f92689d1ab0e341bd5b8af8c5a44f8a2c4 Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Mon, 18 Sep 2017 14:10:10 -0700 Subject: [PATCH] Handle autofill auth scenarios where the FillContext cannot be retrieved: Test: manually modified code to force a null FillContext and verified the session is properly cleared (even in the client side) Test: cts-tradefed run commandAndExit cts-dev -m CtsAutoFillServiceTestCases Fixes: 65374274 Change-Id: Ica482a48b0be34b89e36f9a34078fdcd48134537 --- .../autofill/AutofillManagerServiceImpl.java | 2 +- .../server/autofill/RemoteFillService.java | 5 +-- .../com/android/server/autofill/Session.java | 39 ++++++++++++++++--- .../server/autofill/ui/AutoFillUI.java | 18 ++++----- 4 files changed, 45 insertions(+), 19 deletions(-) diff --git a/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java b/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java index 2b7a671fc99db..1bec9eea82b0f 100644 --- a/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java +++ b/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java @@ -631,7 +631,7 @@ final class AutofillManagerServiceImpl { void destroySessionsLocked() { if (mSessions.size() == 0) { - mUi.destroyAll(null, null); + mUi.destroyAll(null, null, false); return; } while (mSessions.size() > 0) { diff --git a/services/autofill/java/com/android/server/autofill/RemoteFillService.java b/services/autofill/java/com/android/server/autofill/RemoteFillService.java index f315148547e96..af55807ff1f04 100644 --- a/services/autofill/java/com/android/server/autofill/RemoteFillService.java +++ b/services/autofill/java/com/android/server/autofill/RemoteFillService.java @@ -578,9 +578,8 @@ final class RemoteFillService implements DeathRecipient { public void run() { synchronized (mLock) { if (isCancelledLocked()) { - // TODO(b/653742740): we should probably return here, but for now we're justing - // logging to confirm this is the problem if it happens again. - Slog.e(LOG_TAG, "run() called after canceled: " + mRequest); + if (sDebug) Slog.d(LOG_TAG, "run() called after canceled: " + mRequest); + return; } } final RemoteFillService remoteService = getService(); diff --git a/services/autofill/java/com/android/server/autofill/Session.java b/services/autofill/java/com/android/server/autofill/Session.java index dd0c874530ddd..1386e8e60012d 100644 --- a/services/autofill/java/com/android/server/autofill/Session.java +++ b/services/autofill/java/com/android/server/autofill/Session.java @@ -577,6 +577,10 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState // FillServiceCallbacks @Override public void authenticate(int requestId, int datasetIndex, IntentSender intent, Bundle extras) { + if (sDebug) { + Slog.d(TAG, "authenticate(): requestId=" + requestId + "; datasetIdx=" + datasetIndex + + "; intentSender=" + intent); + } final Intent fillInIntent; synchronized (mLock) { if (mDestroyed) { @@ -585,6 +589,10 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState return; } fillInIntent = createAuthFillInIntentLocked(requestId, extras); + if (fillInIntent == null) { + forceRemoveSelfLocked(); + return; + } } mService.setAuthenticationSelected(id); @@ -1537,6 +1545,10 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState } void autoFill(int requestId, int datasetIndex, Dataset dataset, boolean generateEvent) { + if (sDebug) { + Slog.d(TAG, "autoFill(): requestId=" + requestId + "; datasetIdx=" + datasetIndex + + "; dataset=" + dataset); + } synchronized (mLock) { if (mDestroyed) { Slog.w(TAG, "Call to Session#autoFill() rejected - session: " @@ -1557,10 +1569,14 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState mService.logDatasetAuthenticationSelected(dataset.getId(), id); setViewStatesLocked(null, dataset, ViewState.STATE_WAITING_DATASET_AUTH, false); final Intent fillInIntent = createAuthFillInIntentLocked(requestId, mClientState); - + if (fillInIntent == null) { + forceRemoveSelfLocked(); + return; + } final int authenticationId = AutofillManager.makeAuthenticationId(requestId, datasetIndex); startAuthentication(authenticationId, dataset.getAuthentication(), fillInIntent); + } } @@ -1570,14 +1586,16 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState } } + // TODO: this should never be null, but we got at least one occurrence, probably due to a race. + @Nullable private Intent createAuthFillInIntentLocked(int requestId, Bundle extras) { final Intent fillInIntent = new Intent(); final FillContext context = getFillContextByRequestIdLocked(requestId); if (context == null) { - // TODO(b/653742740): this will crash system_server. We need to handle it, but we're - // keeping it crashing for now so we can diagnose when it happens again - Slog.wtf(TAG, "no FillContext for requestId" + requestId + "; mContexts= " + mContexts); + Slog.wtf(TAG, "createAuthFillInIntentLocked(): no FillContext. requestId=" + requestId + + "; mContexts= " + mContexts); + return null; } fillInIntent.putExtra(AutofillManager.EXTRA_ASSIST_STRUCTURE, context.getStructure()); fillInIntent.putExtra(AutofillManager.EXTRA_CLIENT_STATE, extras); @@ -1717,7 +1735,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState if (mDestroyed) { return null; } - mUi.destroyAll(mPendingSaveUi, this); + mUi.destroyAll(mPendingSaveUi, this, true); mUi.clearCallback(this); mDestroyed = true; mMetricsLogger.action(MetricsEvent.AUTOFILL_SESSION_FINISHED, mPackageName); @@ -1733,7 +1751,16 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState mPendingSaveUi = null; removeSelfLocked(); - mUi.destroyAll(mPendingSaveUi, this); + + mHandlerCaller.getHandler().post(() -> { + try { + mClient.setState(mService.isEnabled(), true, false); + } catch (RemoteException e) { + Slog.w(TAG, "error updating client state: " + e); + } + }); + + mUi.destroyAll(mPendingSaveUi, this, false); } /** diff --git a/services/autofill/java/com/android/server/autofill/ui/AutoFillUI.java b/services/autofill/java/com/android/server/autofill/ui/AutoFillUI.java index cac2bff579ed7..688894b77f8cb 100644 --- a/services/autofill/java/com/android/server/autofill/ui/AutoFillUI.java +++ b/services/autofill/java/com/android/server/autofill/ui/AutoFillUI.java @@ -271,7 +271,7 @@ public final class AutoFillUI { if (mCallback != null) { mCallback.save(); } - destroySaveUiUiThread(pendingSaveUi); + destroySaveUiUiThread(pendingSaveUi, true); } @Override @@ -289,7 +289,7 @@ public final class AutoFillUI { if (mCallback != null) { mCallback.cancelSave(); } - destroySaveUiUiThread(pendingSaveUi); + destroySaveUiUiThread(pendingSaveUi, true); } @Override @@ -331,8 +331,8 @@ public final class AutoFillUI { * Destroy all UI affordances. */ public void destroyAll(@Nullable PendingUi pendingSaveUi, - @Nullable AutoFillUiCallback callback) { - mHandler.post(() -> destroyAllUiThread(pendingSaveUi, callback)); + @Nullable AutoFillUiCallback callback, boolean notifyClient) { + mHandler.post(() -> destroyAllUiThread(pendingSaveUi, callback, notifyClient)); } public void dump(PrintWriter pw) { @@ -375,7 +375,7 @@ public final class AutoFillUI { } @android.annotation.UiThread - private void destroySaveUiUiThread(@Nullable PendingUi pendingSaveUi) { + private void destroySaveUiUiThread(@Nullable PendingUi pendingSaveUi, boolean notifyClient) { if (mSaveUi == null) { // Calling destroySaveUiUiThread() twice is normal - it usually happens when the // first call is made after the SaveUI is hidden and the second when the session is @@ -387,7 +387,7 @@ public final class AutoFillUI { if (sDebug) Slog.d(TAG, "destroySaveUiUiThread(): " + pendingSaveUi); mSaveUi.destroy(); mSaveUi = null; - if (pendingSaveUi != null) { + if (pendingSaveUi != null && notifyClient) { try { if (sDebug) Slog.d(TAG, "destroySaveUiUiThread(): notifying client"); pendingSaveUi.client.setSaveUiState(pendingSaveUi.id, false); @@ -399,9 +399,9 @@ public final class AutoFillUI { @android.annotation.UiThread private void destroyAllUiThread(@Nullable PendingUi pendingSaveUi, - @Nullable AutoFillUiCallback callback) { + @Nullable AutoFillUiCallback callback, boolean notifyClient) { hideFillUiUiThread(callback); - destroySaveUiUiThread(pendingSaveUi); + destroySaveUiUiThread(pendingSaveUi, notifyClient); } @android.annotation.UiThread @@ -413,7 +413,7 @@ public final class AutoFillUI { Slog.d(TAG, "hideAllUiThread(): " + "destroying Save UI because pending restoration is finished"); } - destroySaveUiUiThread(pendingSaveUi); + destroySaveUiUiThread(pendingSaveUi, true); } } }