From d9dc954e8f0cde52d4f332c22c2a1e278f6f8508 Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Wed, 19 Sep 2018 11:54:28 -0700 Subject: [PATCH] Fixed FillCallback.onFailure() and SaveCallback.onFailure() behavior. FillCallback.onFailure() was not working as intented - it finished the session on AutofillManagerService, but didn't update the client state on AutofillManager. And both of these methods were displaying Toasts to the user, which is something the autofill services could take care of. Hence, for services target with SDK Q, the message is ignored. Also added a new Autofill Metric: FIELD_AUTOFILL_MESSAGE_LEN Test: atest CtsAutoFillServiceTestCases:android.autofillservice.cts.LoginActivityTest#testAutofillAgainAfterOnFailure Test: atest CtsAutoFillServiceTestCases # to make sure it didn't break anything Test: time mmm -j frameworks/base/:doc-comment-check-docs Test: m -j update-api Bug: 112192360 Fixes: 116103297 Change-Id: I499909200980943dedf1fc8524dd1f14b49e2158 --- .../service/autofill/AutofillService.java | 7 +--- .../service/autofill/FillCallback.java | 17 +++++--- .../service/autofill/SaveCallback.java | 22 +++++----- .../view/autofill/AutofillManager.java | 18 ++++++-- .../metrics_constants/metrics_constants.proto | 7 ++++ .../autofill/AutofillManagerServiceImpl.java | 8 ++++ .../server/autofill/RemoteFillService.java | 1 + .../com/android/server/autofill/Session.java | 41 +++++++++++++++++-- 8 files changed, 94 insertions(+), 27 deletions(-) diff --git a/core/java/android/service/autofill/AutofillService.java b/core/java/android/service/autofill/AutofillService.java index 6c18b4560d294..573d57779df7d 100644 --- a/core/java/android/service/autofill/AutofillService.java +++ b/core/java/android/service/autofill/AutofillService.java @@ -651,14 +651,11 @@ public abstract class AutofillService extends Service { /** * Called when the user requests the service to save the contents of a screen. * - *

Service must call one of the {@link SaveCallback} methods (like - * {@link SaveCallback#onSuccess()} or {@link SaveCallback#onFailure(CharSequence)}) - * to notify the Android System of the result of the request. - * *

If the service could not handle the request right away—for example, because it must * launch an activity asking the user to authenticate first or because the network is * down—the service could keep the {@link SaveRequest request} and reuse it later, - * but the service must call {@link SaveCallback#onSuccess()} right away. + * but the service must always call {@link SaveCallback#onSuccess()} or + * {@link SaveCallback#onSuccess(android.content.IntentSender)} right away. * *

Note: To retrieve the actual value of fields input by the user, the service * should call diff --git a/core/java/android/service/autofill/FillCallback.java b/core/java/android/service/autofill/FillCallback.java index 3893f2a6033d2..06e2896a7f8ad 100644 --- a/core/java/android/service/autofill/FillCallback.java +++ b/core/java/android/service/autofill/FillCallback.java @@ -83,19 +83,24 @@ public final class FillCallback { * fulfill the request; in this case, the service should call {@link #onSuccess(FillResponse) * onSuccess(null)} instead. * - *

Note: on Android versions up to {@link android.os.Build.VERSION_CODES#P}, this - * method is not working as intended, and the service should call + *

Note: prior to {@link android.os.Build.VERSION_CODES#Q}, this + * method was not working as intended and the service should always call * {@link #onSuccess(FillResponse) onSuccess(null)} instead. * - * @param message error message to be displayed to the user. Note: this message is - * displayed on {@code logcat} logs and should not contain PII (Personally Identifiable - * Information, such as username or email address). + *

Note: for apps targeting {@link android.os.Build.VERSION_CODES#Q} or higher, this + * method just logs the message on {@code logcat}; for apps targetting older SDKs, it also + * displays the message to user using a {@link android.widget.Toast}. Generally speaking, you + * should not display an error to the user if the request failed, unless the request had the + * {@link FillRequest#FLAG_MANUAL_REQUEST} flag. + * + * @param message error message. Note: this message should not contain PII + * (Personally Identifiable Information, such as username or email address). * * @throws IllegalStateException if this method or {@link #onSuccess(FillResponse)} was already * called. */ public void onFailure(@Nullable CharSequence message) { - Log.w(TAG, "onFailure(): " + (message == null ? null : message.length() + "_chars")); + Log.w(TAG, "onFailure(): " + message); assertNotCalled(); mCalled = true; try { diff --git a/core/java/android/service/autofill/SaveCallback.java b/core/java/android/service/autofill/SaveCallback.java index 0625095cfe3f7..1753ecfa4e234 100644 --- a/core/java/android/service/autofill/SaveCallback.java +++ b/core/java/android/service/autofill/SaveCallback.java @@ -83,28 +83,30 @@ public final class SaveCallback { } } + + + /** * Notifies the Android System that an * {@link AutofillService#onSaveRequest(SaveRequest, SaveCallback)} could not be handled * by the service. * - *

This method should only be called when the service could not handle the request right away - * and could not recover or retry it. If the service could retry or recover, it could keep - * the {@link SaveRequest} and call {@link #onSuccess()} instead. + *

This method is just used for logging purposes, the Android System won't call the service + * again in case of failures—if you need to recover from the failure, just save the + * {@link SaveRequest} and try again later. * - *

Note: The Android System displays an UI with the supplied error message; if - * you prefer to show your own message, call {@link #onSuccess()} or - * {@link #onSuccess(IntentSender)} instead. + *

Note: for apps targeting {@link android.os.Build.VERSION_CODES#Q} or higher, this + * method just logs the message on {@code logcat}; for apps targetting older SDKs, it also + * displays the message to user using a {@link android.widget.Toast}. * - * @param message error message to be displayed to the user. Note: this message is - * displayed on {@code logcat} logs and should not contain PII (Personally Identifiable - * Information, such as username or email address). + * @param message error message. Note: this message should not contain PII + * (Personally Identifiable Information, such as username or email address). * * @throws IllegalStateException if this method, {@link #onSuccess()}, * or {@link #onSuccess(IntentSender)} was already called. */ public void onFailure(CharSequence message) { - Log.w(TAG, "onFailure(): " + (message == null ? null : message.length() + "_chars")); + Log.w(TAG, "onFailure(): " + message); assertNotCalled(); mCalled = true; try { diff --git a/core/java/android/view/autofill/AutofillManager.java b/core/java/android/view/autofill/AutofillManager.java index e87048e1a5cb2..7abe19e795f4e 100644 --- a/core/java/android/view/autofill/AutofillManager.java +++ b/core/java/android/view/autofill/AutofillManager.java @@ -301,6 +301,14 @@ public final class AutofillManager { */ public static final int STATE_UNKNOWN_COMPAT_MODE = 5; + /** + * Same as {@link #STATE_UNKNOWN}, but used on + * {@link AutofillManagerClient#setSessionFinished(int)} when the session was finished because + * the service failed to fullfil a request. + * + * @hide + */ + public static final int STATE_UNKNOWN_FAILED = 6; /** * Timeout in ms for calls to the field classification service. @@ -2023,8 +2031,10 @@ public final class AutofillManager { * @param newState {@link #STATE_FINISHED} (because the autofill service returned a {@code null} * FillResponse), {@link #STATE_UNKNOWN} (because the session was removed), * {@link #STATE_UNKNOWN_COMPAT_MODE} (beucase the session was finished when the URL bar - * changed on compat mode), or {@link #STATE_DISABLED_BY_SERVICE} (because the autofill service - * disabled further autofill requests for the activity). + * changed on compat mode), {@link #STATE_UNKNOWN_FAILED} (because the session was finished + * when the service failed to fullfil the request, or {@link #STATE_DISABLED_BY_SERVICE} + * (because the autofill service or {@link #STATE_DISABLED_BY_SERVICE} (because the autofill + * service disabled further autofill requests for the activity). */ private void setSessionFinished(int newState) { synchronized (mLock) { @@ -2032,7 +2042,7 @@ public final class AutofillManager { Log.v(TAG, "setSessionFinished(): from " + getStateAsStringLocked() + " to " + getStateAsString(newState)); } - if (newState == STATE_UNKNOWN_COMPAT_MODE) { + if (newState == STATE_UNKNOWN_COMPAT_MODE || newState == STATE_UNKNOWN_FAILED) { resetSessionLocked(/* resetEnteredIds= */ true); mState = STATE_UNKNOWN; } else { @@ -2229,6 +2239,8 @@ public final class AutofillManager { return "DISABLED_BY_SERVICE"; case STATE_UNKNOWN_COMPAT_MODE: return "UNKNOWN_COMPAT_MODE"; + case STATE_UNKNOWN_FAILED: + return "UNKNOWN_FAILED"; default: return "INVALID:" + state; } diff --git a/proto/src/metrics_constants/metrics_constants.proto b/proto/src/metrics_constants/metrics_constants.proto index 41b3febef31ff..de85dcb16adbf 100644 --- a/proto/src/metrics_constants/metrics_constants.proto +++ b/proto/src/metrics_constants/metrics_constants.proto @@ -4026,6 +4026,8 @@ message MetricsEvent { // - AUTOFILL_DATASET_AUTHENTICATED // - AUTOFILL_INVALID_AUTHENTICATION // - AUTOFILL_INVALID_DATASET_AUTHENTICATION + // NOTE: starting on OS Q, it also added the following fields: + // Tag FIELD_AUTOFILL_TEXT_LEN: length of the error message provided by the service AUTOFILL_REQUEST = 907; // Tag of a field for a package of an autofill service @@ -4102,6 +4104,8 @@ message MetricsEvent { // Tag FIELD_CLASS_NAME: Class name of the activity that is autofilled. // Tag FIELD_AUTOFILL_SESSION_ID: id of the autofill session associated with this metric. // Tag FIELD_AUTOFILL_COMPAT_MODE: package is being autofilled on compatibility mode. + // NOTE: starting on OS Q, it also added the following fields: + // Tag FIELD_AUTOFILL_TEXT_LEN: length of the error message provided by the service AUTOFILL_DATA_SAVE_REQUEST = 918; // An auto-fill session was finished @@ -6517,6 +6521,9 @@ message MetricsEvent { // OS: Q MOBILE_NETWORK = 1571; + // Tag of a field for the length of a text + FIELD_AUTOFILL_TEXT_LEN = 1572; + // ---- End Q Constants, all Q constants go above this line ---- // Add new aosp constants above this line. diff --git a/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java b/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java index 8c8352f81df5b..78facf84b03c8 100644 --- a/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java +++ b/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java @@ -235,6 +235,13 @@ final class AutofillManagerServiceImpl { } } + int getTargedSdkLocked() { + if (mInfo == null) { + return 0; + } + return mInfo.getServiceInfo().applicationInfo.targetSdkVersion; + } + private boolean isSetupCompletedLocked() { final String setupComplete = Settings.Secure.getStringForUser( mContext.getContentResolver(), Settings.Secure.USER_SETUP_COMPLETE, mUserId); @@ -953,6 +960,7 @@ final class AutofillManagerServiceImpl { pw.println(); mInfo.dump(prefix2, pw); pw.print(prefix); pw.print("Service Label: "); pw.println(getServiceLabel()); + pw.print(prefix); pw.print("Target SDK: "); pw.println(getTargedSdkLocked()); } pw.print(prefix); pw.print("Component from settings: "); pw.println(getComponentNameFromSettings()); diff --git a/services/autofill/java/com/android/server/autofill/RemoteFillService.java b/services/autofill/java/com/android/server/autofill/RemoteFillService.java index 2671327350be9..d1b09ca1aca8e 100644 --- a/services/autofill/java/com/android/server/autofill/RemoteFillService.java +++ b/services/autofill/java/com/android/server/autofill/RemoteFillService.java @@ -29,6 +29,7 @@ import android.content.Context; import android.content.Intent; import android.content.IntentSender; import android.content.ServiceConnection; +import android.os.Build; import android.os.Handler; import android.os.IBinder; import android.os.IBinder.DeathRecipient; diff --git a/services/autofill/java/com/android/server/autofill/Session.java b/services/autofill/java/com/android/server/autofill/Session.java index c9eb2d2a386f2..35f16b626afd3 100644 --- a/services/autofill/java/com/android/server/autofill/Session.java +++ b/services/autofill/java/com/android/server/autofill/Session.java @@ -50,6 +50,7 @@ import android.graphics.Bitmap; import android.graphics.Rect; import android.metrics.LogMaker; import android.os.Binder; +import android.os.Build; import android.os.Bundle; import android.os.Handler; import android.os.IBinder; @@ -63,6 +64,7 @@ import android.service.autofill.AutofillService; import android.service.autofill.Dataset; import android.service.autofill.FieldClassification; import android.service.autofill.FieldClassification.Match; +import android.text.TextUtils; import android.service.autofill.FillContext; import android.service.autofill.FillRequest; import android.service.autofill.FillResponse; @@ -744,12 +746,17 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState private void onFillRequestFailureOrTimeout(int requestId, boolean timedOut, @Nullable CharSequence message) { + boolean showMessage = !TextUtils.isEmpty(message); synchronized (mLock) { if (mDestroyed) { Slog.w(TAG, "Call to Session#onFillRequestFailureOrTimeout(req=" + requestId + ") rejected - session: " + id + " destroyed"); return; } + if (sDebug) { + Slog.d(TAG, "finishing session due to service " + + (timedOut ? "timeout" : "failure")); + } mService.resetLastResponse(); final LogMaker requestLog = mRequestLogs.get(requestId); if (requestLog == null) { @@ -757,8 +764,21 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState } else { requestLog.setType(timedOut ? MetricsEvent.TYPE_CLOSE : MetricsEvent.TYPE_FAILURE); } + if (showMessage) { + final int targetSdk = mService.getTargedSdkLocked(); + if (targetSdk >= Build.VERSION_CODES.Q) { + showMessage = false; + Slog.w(TAG, "onFillRequestFailureOrTimeout(): not showing '" + message + + "' because service's targetting API " + targetSdk); + } + if (message != null) { + requestLog.addTaggedData(MetricsEvent.FIELD_AUTOFILL_TEXT_LEN, + message.length()); + } + } } - if (message != null) { + notifyUnavailableToClient(AutofillManager.STATE_UNKNOWN_FAILED); + if (showMessage) { getUiForShowing().showError(message, this); } removeSelf(); @@ -793,6 +813,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState @Override public void onSaveRequestFailure(@Nullable CharSequence message, @NonNull String servicePackageName) { + boolean showMessage = !TextUtils.isEmpty(message); synchronized (mLock) { mIsSaving = false; @@ -801,12 +822,26 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState + id + " destroyed"); return; } + if (showMessage) { + final int targetSdk = mService.getTargedSdkLocked(); + if (targetSdk >= Build.VERSION_CODES.Q) { + showMessage = false; + Slog.w(TAG, "onSaveRequestFailure(): not showing '" + message + + "' because service's targetting API " + targetSdk); + } + } } - LogMaker log = newLogMaker(MetricsEvent.AUTOFILL_DATA_SAVE_REQUEST, servicePackageName) + final LogMaker log = + newLogMaker(MetricsEvent.AUTOFILL_DATA_SAVE_REQUEST, servicePackageName) .setType(MetricsEvent.TYPE_FAILURE); + if (message != null) { + log.addTaggedData(MetricsEvent.FIELD_AUTOFILL_TEXT_LEN, message.length()); + } mMetricsLogger.write(log); - getUiForShowing().showError(message, this); + if (showMessage) { + getUiForShowing().showError(message, this); + } removeSelf(); }