From 53e9bee4ea135996f0d8e870c745ced3c35caf69 Mon Sep 17 00:00:00 2001 From: Simranjit Kohli Date: Mon, 1 May 2023 22:20:02 -0700 Subject: [PATCH] [PCC]: Fixes for PCC Create session only once. Previously, multiple sessions could be created, and can result in multiple requsets to PCC. Add local logs for debugging. Fix to show datasets from PCC. Seems like it stopped working from a previous change. Upcoming PccFieldClassificationTest should prevent such regressions. Test: atest CtsAutoFillServiceTestCases Also atest android.autofillservice.cts.servicebehavior.PccFieldClassificationTest --iterations=5 Bug: 279608578 Change-Id: Ia026adc6e45ea3d7c15c560fac59093f202ccc4e --- .../view/autofill/AutofillManager.java | 22 ++- .../RemoteFieldClassificationService.java | 4 + .../com/android/server/autofill/Session.java | 141 +++++++++++++----- 3 files changed, 121 insertions(+), 46 deletions(-) diff --git a/core/java/android/view/autofill/AutofillManager.java b/core/java/android/view/autofill/AutofillManager.java index f7b7d33879386..d6bc035c17668 100644 --- a/core/java/android/view/autofill/AutofillManager.java +++ b/core/java/android/view/autofill/AutofillManager.java @@ -1497,14 +1497,22 @@ public final class AutofillManager { // to PCC classification service. if (AutofillFeatureFlags.isAutofillPccClassificationEnabled()) { synchronized (mLock) { - final boolean clientAdded = tryAddServiceClientIfNeededLocked(); - if (clientAdded){ - startSessionLocked(/* id= */ AutofillId.NO_AUTOFILL_ID, - /* bounds= */ null, /* value= */ null, /* flags= */ FLAG_PCC_DETECTION); - } else { - if (sVerbose) { - Log.v(TAG, "not starting session: no service client"); + // If session has already been created, that'd mean we already have issued the + // detection request previously. It is possible in cases like autofocus that this + // method isn't invoked, so the server should still handle such cases where fill + // request comes in but PCC Detection hasn't been triggered. There is no benefit to + // trigger PCC Detection separately in those cases. + if (!isActiveLocked()) { + final boolean clientAdded = tryAddServiceClientIfNeededLocked(); + if (clientAdded) { + startSessionLocked(/* id= */ AutofillId.NO_AUTOFILL_ID, /* bounds= */ null, + /* value= */ null, /* flags= */ FLAG_PCC_DETECTION); + } else { + if (sVerbose) { + Log.v(TAG, "not starting session: no service client"); + } } + } } } diff --git a/services/autofill/java/com/android/server/autofill/RemoteFieldClassificationService.java b/services/autofill/java/com/android/server/autofill/RemoteFieldClassificationService.java index feae56e89784a..b8bac61b346b6 100644 --- a/services/autofill/java/com/android/server/autofill/RemoteFieldClassificationService.java +++ b/services/autofill/java/com/android/server/autofill/RemoteFieldClassificationService.java @@ -157,6 +157,8 @@ final class RemoteFieldClassificationService if (sDebug) { Log.d(TAG, "onSuccess Response: " + response); } + fieldClassificationServiceCallbacks + .onClassificationRequestSuccess(response); } @Override @@ -165,6 +167,8 @@ final class RemoteFieldClassificationService if (sDebug) { Log.d(TAG, "onFailure"); } + fieldClassificationServiceCallbacks + .onClassificationRequestFailure(0, null); } @Override diff --git a/services/autofill/java/com/android/server/autofill/Session.java b/services/autofill/java/com/android/server/autofill/Session.java index b2e8ffcd8fcad..01339e7db78c8 100644 --- a/services/autofill/java/com/android/server/autofill/Session.java +++ b/services/autofill/java/com/android/server/autofill/Session.java @@ -79,11 +79,6 @@ import static com.android.server.autofill.SaveEventLogger.SAVE_UI_SHOWN_REASON_O import static com.android.server.autofill.SaveEventLogger.SAVE_UI_SHOWN_REASON_REQUIRED_ID_CHANGE; import static com.android.server.autofill.SaveEventLogger.SAVE_UI_SHOWN_REASON_TRIGGER_ID_SET; import static com.android.server.autofill.SaveEventLogger.SAVE_UI_SHOWN_REASON_UNKNOWN; -import static com.android.server.autofill.SessionCommittedEventLogger.CommitReason; -import static com.android.server.autofill.SessionCommittedEventLogger.COMMIT_REASON_ACTIVITY_FINISHED; -import static com.android.server.autofill.SessionCommittedEventLogger.COMMIT_REASON_VIEW_CHANGED; -import static com.android.server.autofill.SessionCommittedEventLogger.COMMIT_REASON_VIEW_CLICKED; -import static com.android.server.autofill.SessionCommittedEventLogger.COMMIT_REASON_VIEW_COMMITTED; import static com.android.server.autofill.SessionCommittedEventLogger.COMMIT_REASON_SESSION_DESTROYED; import static com.android.server.wm.ActivityTaskManagerInternal.ASSIST_KEY_RECEIVER_EXTRAS; import static com.android.server.wm.ActivityTaskManagerInternal.ASSIST_KEY_STRUCTURE; @@ -208,6 +203,10 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState RemoteFieldClassificationService.FieldClassificationServiceCallbacks { private static final String TAG = "AutofillSession"; + // This should never be true in production. This is only for local debugging. + // Otherwise it will spam logcat. + private static final boolean DBG = false; + private static final String ACTION_DELAYED_FILL = "android.service.autofill.action.DELAYED_FILL"; private static final String EXTRA_REQUEST_ID = "android.service.autofill.extra.REQUEST_ID"; @@ -1284,6 +1283,8 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState @GuardedBy("mLock") private void requestAssistStructureForPccLocked(int flags) { + if (!mClassificationState.shouldTriggerRequest()) return; + mClassificationState.updatePendingRequest(); // Get request id int requestId; // TODO(b/158623971): Update this to prevent possible overflow @@ -1619,12 +1620,18 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState // TODO(b/266379948): Ideally wait for PCC request to finish for a while more // (say 100ms) before proceeding further on. + if (DBG) { + Slog.d(TAG, "DBG: Initial response: " + response); + } synchronized (mLock) { response = getEffectiveFillResponse(response); if (isEmptyResponse(response)) { // Treat it as a null response. processNullResponseLocked(requestId, requestFlags); } + if (DBG) { + Slog.d(TAG, "DBG: Processed response: " + response); + } processResponseLocked(response, null, requestFlags); } } @@ -1651,12 +1658,25 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState DatasetComputationContainer autofillProviderContainer = new DatasetComputationContainer(); computeDatasetsForProviderAndUpdateContainer(response, autofillProviderContainer); + if (DBG) { + Slog.d(TAG, "DBG: computeDatasetsForProviderAndUpdateContainer: " + + autofillProviderContainer); + } if (!mService.getMaster().isPccClassificationEnabled()) { + if (sVerbose) { + Slog.v(TAG, "PCC classification is disabled"); + } return createShallowCopy(response, autofillProviderContainer); } synchronized (mLock) { if (mClassificationState.mState != ClassificationState.STATE_RESPONSE || mClassificationState.mLastFieldClassificationResponse == null) { + if (sVerbose) { + Slog.v(TAG, "PCC classification no last response:" + + (mClassificationState.mLastFieldClassificationResponse == null) + + " ,ineligible state=" + + (mClassificationState.mState != ClassificationState.STATE_RESPONSE)); + } return createShallowCopy(response, autofillProviderContainer); } if (!mClassificationState.processResponse()) return response; @@ -1664,11 +1684,22 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState boolean preferAutofillProvider = mService.getMaster().preferProviderOverPcc(); boolean shouldUseFallback = mService.getMaster().shouldUsePccFallback(); if (preferAutofillProvider && !shouldUseFallback) { + if (sVerbose) { + Slog.v(TAG, "preferAutofillProvider but no fallback"); + } return createShallowCopy(response, autofillProviderContainer); } + if (DBG) { + synchronized (mLock) { + Slog.d(TAG, "DBG: ClassificationState: " + mClassificationState); + } + } DatasetComputationContainer detectionPccContainer = new DatasetComputationContainer(); computeDatasetsForPccAndUpdateContainer(response, detectionPccContainer); + if (DBG) { + Slog.d(TAG, "DBG: computeDatasetsForPccAndUpdateContainer: " + detectionPccContainer); + } DatasetComputationContainer resultContainer; if (preferAutofillProvider) { @@ -1742,6 +1773,20 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState // FillResponse. Set mDatasets = new LinkedHashSet<>(); ArrayMap> mAutofillIdToDatasetMap = new ArrayMap<>(); + + public String toString() { + final StringBuilder builder = new StringBuilder("DatasetComputationContainer["); + if (mAutofillIds != null) { + builder.append(", autofillIds=").append(mAutofillIds); + } + if (mDatasets != null) { + builder.append(", mDatasets=").append(mDatasets); + } + if (mAutofillIdToDatasetMap != null) { + builder.append(", mAutofillIdToDatasetMap=").append(mAutofillIdToDatasetMap); + } + return builder.append(']').toString(); + } } // Adds fallback datasets to the first container. @@ -1893,7 +1938,6 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState Dataset dataset = datasets.get(i); if (dataset.getAutofillDatatypes() == null || dataset.getAutofillDatatypes().isEmpty()) continue; - if (dataset.getFieldIds() != null && dataset.getFieldIds().size() > 0) continue; ArrayList fieldIds = new ArrayList<>(); ArrayList fieldValues = new ArrayList<>(); @@ -1902,9 +1946,10 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState ArrayList fieldInlinePresentations = new ArrayList<>(); ArrayList fieldInlineTooltipPresentations = new ArrayList<>(); ArrayList fieldFilters = new ArrayList<>(); + Set datasetAutofillIds = new ArraySet<>(); for (int j = 0; j < dataset.getAutofillDatatypes().size(); j++) { - if (dataset.getAutofillDatatypes().get(0) == null) continue; + if (dataset.getAutofillDatatypes().get(j) == null) continue; String hint = dataset.getAutofillDatatypes().get(j); if (hintsToAutofillIdMap.containsKey(hint)) { @@ -1913,6 +1958,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState for (AutofillId autofillId : tempIds) { eligibleAutofillIds.add(autofillId); + datasetAutofillIds.add(autofillId); // For each of the field, copy over values. fieldIds.add(autofillId); fieldValues.add(dataset.getFieldValues().get(j)); @@ -1926,37 +1972,6 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState dataset.getFieldInlineTooltipPresentation(j)); fieldFilters.add(dataset.getFilter(j)); } - - Dataset newDataset = - new Dataset( - fieldIds, - fieldValues, - fieldPresentations, - fieldDialogPresentations, - fieldInlinePresentations, - fieldInlineTooltipPresentations, - fieldFilters, - new ArrayList<>(), - dataset.getFieldContent(), - null, - null, - null, - null, - dataset.getId(), - dataset.getAuthentication()); - eligibleDatasets.add(newDataset); - - // Associate this dataset with all the ids that are represented with it. - Set newDatasets; - for (AutofillId autofillId : tempIds) { - if (map.containsKey(autofillId)) { - newDatasets = map.get(autofillId); - } else { - newDatasets = new ArraySet<>(); - } - newDatasets.add(newDataset); - map.put(autofillId, newDatasets); - } } // TODO(b/266379948): handle the case: // groupHintsToAutofillIdMap.containsKey(hint)) @@ -1964,6 +1979,34 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState // TODO(b/266379948): also handle the case where there could be more types in // the dataset, provided by the provider, however, they aren't applicable. } + Dataset newDataset = + new Dataset( + fieldIds, + fieldValues, + fieldPresentations, + fieldDialogPresentations, + fieldInlinePresentations, + fieldInlineTooltipPresentations, + fieldFilters, + new ArrayList<>(), + dataset.getFieldContent(), + null, + null, + null, + null, + dataset.getId(), + dataset.getAuthentication()); + eligibleDatasets.add(newDataset); + Set newDatasets; + for (AutofillId autofillId : datasetAutofillIds) { + if (map.containsKey(autofillId)) { + newDatasets = map.get(autofillId); + } else { + newDatasets = new ArraySet<>(); + } + newDatasets.add(newDataset); + map.put(autofillId, newDatasets); + } } container.mAutofillIds = eligibleAutofillIds; container.mDatasets = eligibleDatasets; @@ -5443,6 +5486,26 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState mState = STATE_PENDING_REQUEST; mPendingFieldClassificationRequest = null; } + + @GuardedBy("mLock") + private boolean shouldTriggerRequest() { + return mState == STATE_INITIAL || mState == STATE_INVALIDATED; + } + + @GuardedBy("mLock") + @Override + public String toString() { + return "ClassificationState: [" + + "state=" + stateToString() + + ", mPendingFieldClassificationRequest=" + mPendingFieldClassificationRequest + + ", mLastFieldClassificationResponse=" + mLastFieldClassificationResponse + + ", mClassificationHintsMap=" + mClassificationHintsMap + + ", mClassificationGroupHintsMap=" + mClassificationGroupHintsMap + + ", mHintsToAutofillIdMap=" + mHintsToAutofillIdMap + + ", mGroupHintsToAutofillIdMap=" + mGroupHintsToAutofillIdMap + + "]"; + } + } @Override @@ -5966,7 +6029,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState return serviceInfo == null ? Process.INVALID_UID : serviceInfo.applicationInfo.uid; } - // DetectionServiceCallbacks + // FieldClassificationServiceCallbacks public void onClassificationRequestSuccess(@Nullable FieldClassificationResponse response) { mClassificationState.updateResponseReceived(response); }