From 1e5f59fe9b9b398dfc62c410cf98dadbf599219b Mon Sep 17 00:00:00 2001 From: Adam He Date: Tue, 9 Apr 2019 11:46:01 -0700 Subject: [PATCH] Refactored field classification workflow to not hold locks when calling ExtServices. Fixes: 127665636 Test: atest android.autofillservice.cts.FieldsClassificationTest Change-Id: Ie3b82e6e8d56db8d953d36d478afb4c327929958 --- .../java/com/android/server/autofill/Session.java | 109 +++++++++++---------- 1 file changed, 57 insertions(+), 52 deletions(-) diff --git a/services/autofill/java/com/android/server/autofill/Session.java b/services/autofill/java/com/android/server/autofill/Session.java index 0402b8fb9285..a1bba54ef306 100644 --- a/services/autofill/java/com/android/server/autofill/Session.java +++ b/services/autofill/java/com/android/server/autofill/Session.java @@ -1243,18 +1243,55 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState * when necessary. */ public void logContextCommitted() { - mHandler.sendMessage(obtainMessage( - Session::doLogContextCommitted, this)); + mHandler.sendMessage(obtainMessage(Session::handleLogContextCommitted, this)); + } + + private void handleLogContextCommitted() { + final FillResponse lastResponse; + synchronized (mLock) { + lastResponse = getLastResponseLocked("logContextCommited()"); + } + + if (lastResponse == null) { + Slog.w(TAG, "handleLogContextCommitted(): last response is null"); + return; + } + + // Merge UserData if necessary. + // Fields in packageUserData will override corresponding fields in genericUserData. + final UserData genericUserData = mService.getUserData(); + final UserData packageUserData = lastResponse.getUserData(); + final FieldClassificationUserData userData; + if (packageUserData == null && genericUserData == null) { + userData = null; + } else if (packageUserData != null && genericUserData != null) { + userData = new CompositeUserData(genericUserData, packageUserData); + } else if (packageUserData != null) { + userData = packageUserData; + } else { + userData = mService.getUserData(); + } + + final FieldClassificationStrategy fcStrategy = mService.getFieldClassificationStrategy(); + + // Sets field classification scores + if (userData != null && fcStrategy != null) { + logFieldClassificationScore(fcStrategy, userData); + } else { + logContextCommitted(null, null); + } } - private void doLogContextCommitted() { + private void logContextCommitted(@Nullable ArrayList detectedFieldIds, + @Nullable ArrayList detectedFieldClassifications) { synchronized (mLock) { - logContextCommittedLocked(); + logContextCommittedLocked(detectedFieldIds, detectedFieldClassifications); } } @GuardedBy("mLock") - private void logContextCommittedLocked() { + private void logContextCommittedLocked(@Nullable ArrayList detectedFieldIds, + @Nullable ArrayList detectedFieldClassifications) { final FillResponse lastResponse = getLastResponseLocked("logContextCommited()"); if (lastResponse == null) return; @@ -1308,21 +1345,6 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState return; } - // Merge UserData if necessary. - // Fields in packageUserData will override corresponding fields in genericUserData. - final UserData genericUserData = mService.getUserData(); - final UserData packageUserData = lastResponse.getUserData(); - final FieldClassificationUserData userData; - if (packageUserData == null && genericUserData == null) { - userData = null; - } else if (packageUserData != null && genericUserData != null) { - userData = new CompositeUserData(genericUserData, packageUserData); - } else if (packageUserData != null) { - userData = packageUserData; - } else { - userData = mService.getUserData(); - } - for (int i = 0; i < mViewStates.size(); i++) { final ViewState viewState = mViewStates.valueAt(i); final int state = viewState.getState(); @@ -1447,33 +1469,18 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState } } - // Sets field classification scores - final FieldClassificationStrategy fcStrategy = mService.getFieldClassificationStrategy(); - if (userData != null && fcStrategy != null) { - logFieldClassificationScoreLocked(fcStrategy, ignoredDatasets, changedFieldIds, - changedDatasetIds, manuallyFilledFieldIds, manuallyFilledDatasetIds, - userData, mViewStates.values()); - } else { - mService.logContextCommittedLocked(id, mClientState, mSelectedDatasetIds, - ignoredDatasets, changedFieldIds, changedDatasetIds, - manuallyFilledFieldIds, manuallyFilledDatasetIds, - mComponentName, mCompatMode); - } + mService.logContextCommittedLocked(id, mClientState, mSelectedDatasetIds, + ignoredDatasets, changedFieldIds, changedDatasetIds, + manuallyFilledFieldIds, manuallyFilledDatasetIds, detectedFieldIds, + detectedFieldClassifications, mComponentName, mCompatMode); } /** * Adds the matches to {@code detectedFieldsIds} and {@code detectedFieldClassifications} for * {@code fieldId} based on its {@code currentValue} and {@code userData}. */ - private void logFieldClassificationScoreLocked( - @NonNull FieldClassificationStrategy fcStrategy, - @NonNull ArraySet ignoredDatasets, - @NonNull ArrayList changedFieldIds, - @NonNull ArrayList changedDatasetIds, - @NonNull ArrayList manuallyFilledFieldIds, - @NonNull ArrayList> manuallyFilledDatasetIds, - @NonNull FieldClassificationUserData userData, - @NonNull Collection viewStates) { + private void logFieldClassificationScore(@NonNull FieldClassificationStrategy fcStrategy, + @NonNull FieldClassificationUserData userData) { final String[] userValues = userData.getValues(); final String[] categoryIds = userData.getCategoryIds(); @@ -1499,6 +1506,11 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState final ArrayList detectedFieldClassifications = new ArrayList<>( maxFieldsSize); + final Collection viewStates; + synchronized (mLock) { + viewStates = mViewStates.values(); + } + final int viewsSize = viewStates.size(); // First, we get all scores. @@ -1514,10 +1526,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState final RemoteCallback callback = new RemoteCallback((result) -> { if (result == null) { if (sDebug) Slog.d(TAG, "setFieldClassificationScore(): no results"); - mService.logContextCommittedLocked(id, mClientState, mSelectedDatasetIds, - ignoredDatasets, changedFieldIds, changedDatasetIds, - manuallyFilledFieldIds, manuallyFilledDatasetIds, - mComponentName, mCompatMode); + logContextCommitted(null, null); return; } final Scores scores = result.getParcelable(EXTRA_SCORES); @@ -1544,7 +1553,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState final Float currentScore = scoresByField.get(categoryId); if (currentScore != null && currentScore > score) { if (sVerbose) { - Slog.v(TAG, "skipping score " + score + Slog.v(TAG, "skipping score " + score + " because it's less than " + currentScore); } continue; @@ -1554,8 +1563,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState + autofillId); } scoresByField.put(categoryId, score); - } - else if (sVerbose) { + } else if (sVerbose) { Slog.v(TAG, "skipping score 0 at index " + j + " and id " + autofillId); } } @@ -1579,10 +1587,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState return; } - mService.logContextCommittedLocked(id, mClientState, mSelectedDatasetIds, - ignoredDatasets, changedFieldIds, changedDatasetIds, manuallyFilledFieldIds, - manuallyFilledDatasetIds, detectedFieldIds, detectedFieldClassifications, - mComponentName, mCompatMode); + logContextCommitted(detectedFieldIds, detectedFieldClassifications); }); fcStrategy.calculateScores(callback, currentValues, userValues, categoryIds, -- 2.11.0