Prevent race condition

Use proper synchronization to prevent server crash.

There were more cases where lokcing wasn't done as expected.
Those are addressed in this change.

Additional refactoring is also done to make code follow existing pattern.

Test: atest CtsAutoFillServiceTestCases
Bug: 339729009
Change-Id: I4c43bfcd8be93d1e3dd159fa243693cb0fc607bb

4 files changed, 74 insertions, 64 deletions

diff --git a/services/autofill/java/com/android/server/autofill/AutofillManagerService.java b/services/autofill/java/com/android/server/autofill/AutofillManagerService.java
index 6fc05b72da9b..eae516e10d6e 100644
--- a/services/autofill/java/com/android/server/autofill/AutofillManagerService.java
+++ b/services/autofill/java/com/android/server/autofill/AutofillManagerService.java
@@ -2003,7 +2003,7 @@ public final class AutofillManagerService
                 final AutofillManagerServiceImpl service =
                         peekServiceForUserWithLocalBinderIdentityLocked(userId);
                 if (service != null) {
-                    service.setViewAutofilled(sessionId, getCallingUid(), id);
+                    service.setViewAutofilledLocked(sessionId, getCallingUid(), id);
                 } else if (sVerbose) {
                     Slog.v(TAG, "setAutofillFailure(): no service for " + userId);
                 }
diff --git a/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java b/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java
index 588266fba47a..9ad15b2d431f 100644
--- a/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java
+++ b/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java
@@ -478,7 +478,7 @@ final class AutofillManagerServiceImpl
     }
 
     @GuardedBy("mLock")
-    void setViewAutofilled(int sessionId, int uid, @NonNull AutofillId id) {
+    void setViewAutofilledLocked(int sessionId, int uid, @NonNull AutofillId id) {
         if (!isEnabledLocked()) {
             Slog.wtf(TAG, "Service not enabled");
             return;
@@ -488,7 +488,7 @@ final class AutofillManagerServiceImpl
             Slog.v(TAG, "setViewAutofilled(): no session for " + sessionId + "(" + uid + ")");
             return;
         }
-        session.setViewAutofilled(id);
+        session.setViewAutofilledLocked(id);
     }
 
     @GuardedBy("mLock")
@@ -792,10 +792,9 @@ final class AutofillManagerServiceImpl
      * Initializes the last fill selection after an autofill service returned a new
      * {@link FillResponse}.
      */
-    void setLastResponse(int sessionId, @NonNull FillResponse response) {
-        synchronized (mLock) {
+    @GuardedBy("mLock")
+    void setLastResponseLocked(int sessionId, @NonNull FillResponse response) {
             mEventHistory = new FillEventHistory(sessionId, response.getClientState());
-        }
     }
 
     void setLastAugmentedAutofillResponse(int sessionId) {
diff --git a/services/autofill/java/com/android/server/autofill/Session.java b/services/autofill/java/com/android/server/autofill/Session.java
index aa67ffed0b2d..53a82648c640 100644
--- a/services/autofill/java/com/android/server/autofill/Session.java
+++ b/services/autofill/java/com/android/server/autofill/Session.java
@@ -1677,22 +1677,23 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState
 
         final LogMaker requestLog;
 
-        // Start a new FillResponse logger for the success case.
-        mFillResponseEventLogger.startLogForNewResponse();
-        mFillResponseEventLogger.maybeSetRequestId(requestId);
-        mFillResponseEventLogger.maybeSetAppPackageUid(uid);
-        mFillResponseEventLogger.maybeSetResponseStatus(RESPONSE_STATUS_SUCCESS);
-        mFillResponseEventLogger.startResponseProcessingTime();
-        // Time passed since session was created
-        final long fillRequestReceivedRelativeTimestamp =
-            SystemClock.elapsedRealtime() - mLatencyBaseTime;
-        mPresentationStatsEventLogger.maybeSetFillResponseReceivedTimestampMs(
-            (int) (fillRequestReceivedRelativeTimestamp));
-        mFillResponseEventLogger.maybeSetLatencyFillResponseReceivedMillis(
-            (int) (fillRequestReceivedRelativeTimestamp));
-        mFillResponseEventLogger.maybeSetDetectionPreference(getDetectionPreferenceForLogging());
-
         synchronized (mLock) {
+            // Start a new FillResponse logger for the success case.
+            mFillResponseEventLogger.startLogForNewResponse();
+            mFillResponseEventLogger.maybeSetRequestId(requestId);
+            mFillResponseEventLogger.maybeSetAppPackageUid(uid);
+            mFillResponseEventLogger.maybeSetResponseStatus(RESPONSE_STATUS_SUCCESS);
+            mFillResponseEventLogger.startResponseProcessingTime();
+            // Time passed since session was created
+            final long fillRequestReceivedRelativeTimestamp =
+                    SystemClock.elapsedRealtime() - mLatencyBaseTime;
+            mPresentationStatsEventLogger.maybeSetFillResponseReceivedTimestampMs(
+                    (int) (fillRequestReceivedRelativeTimestamp));
+            mFillResponseEventLogger.maybeSetLatencyFillResponseReceivedMillis(
+                    (int) (fillRequestReceivedRelativeTimestamp));
+            mFillResponseEventLogger.maybeSetDetectionPreference(
+                    getDetectionPreferenceForLogging());
+
             if (mDestroyed) {
                 Slog.w(TAG, "Call to Session#onFillRequestSuccess() rejected - session: "
                         + id + " destroyed");
@@ -1744,11 +1745,9 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState
                 Slog.v(TAG, "Service requested to wait for delayed fill response.");
                 registerDelayedFillBroadcastLocked();
             }
-        }
 
-        mService.setLastResponse(id, response);
+            mService.setLastResponseLocked(id, response);
 
-        synchronized (mLock) {
             if (mLogViewEntered) {
                 mLogViewEntered = false;
                 mService.logViewEntered(id, null);
@@ -1821,16 +1820,18 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState
         }
 
         int datasetCount = (datasetList == null) ? 0 : datasetList.size();
-        mFillResponseEventLogger.maybeSetTotalDatasetsProvided(datasetCount);
-        // It's possible that this maybe overwritten later on after PCC filtering.
-        mFillResponseEventLogger.maybeSetAvailableCount(datasetCount);
+        synchronized (mLock) {
+            mFillResponseEventLogger.maybeSetTotalDatasetsProvided(datasetCount);
+            // It's possible that this maybe overwritten later on after PCC filtering.
+            mFillResponseEventLogger.maybeSetAvailableCount(datasetCount);
 
-        // TODO(b/266379948): Ideally wait for PCC request to finish for a while more
-        // (say 100ms) before proceeding further on.
+            // TODO(b/266379948): Ideally wait for PCC request to finish for a while more
+            // (say 100ms) before proceeding further on.
 
-        processResponseLockedForPcc(response, response.getClientState(), requestFlags);
-        mFillResponseEventLogger.maybeSetLatencyResponseProcessingMillis();
-        mFillResponseEventLogger.logAndEndEvent();
+            processResponseLockedForPcc(response, response.getClientState(), requestFlags);
+            mFillResponseEventLogger.maybeSetLatencyResponseProcessingMillis();
+            mFillResponseEventLogger.logAndEndEvent();
+        }
     }
 
 
@@ -2381,21 +2382,22 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState
             @Nullable CharSequence message) {
         boolean showMessage = !TextUtils.isEmpty(message);
 
-        // Start a new FillResponse logger for the failure or timeout case.
-        mFillResponseEventLogger.startLogForNewResponse();
-        mFillResponseEventLogger.maybeSetRequestId(requestId);
-        mFillResponseEventLogger.maybeSetAppPackageUid(uid);
-        mFillResponseEventLogger.maybeSetAvailableCount(
-                AVAILABLE_COUNT_WHEN_FILL_REQUEST_FAILED_OR_TIMEOUT);
-        mFillResponseEventLogger.maybeSetTotalDatasetsProvided(
-                AVAILABLE_COUNT_WHEN_FILL_REQUEST_FAILED_OR_TIMEOUT);
-        mFillResponseEventLogger.maybeSetDetectionPreference(getDetectionPreferenceForLogging());
-        final long fillRequestReceivedRelativeTimestamp =
-            SystemClock.elapsedRealtime() - mLatencyBaseTime;
-        mFillResponseEventLogger.maybeSetLatencyFillResponseReceivedMillis(
-            (int)(fillRequestReceivedRelativeTimestamp));
-
         synchronized (mLock) {
+            // Start a new FillResponse logger for the failure or timeout case.
+            mFillResponseEventLogger.startLogForNewResponse();
+            mFillResponseEventLogger.maybeSetRequestId(requestId);
+            mFillResponseEventLogger.maybeSetAppPackageUid(uid);
+            mFillResponseEventLogger.maybeSetAvailableCount(
+                    AVAILABLE_COUNT_WHEN_FILL_REQUEST_FAILED_OR_TIMEOUT);
+            mFillResponseEventLogger.maybeSetTotalDatasetsProvided(
+                    AVAILABLE_COUNT_WHEN_FILL_REQUEST_FAILED_OR_TIMEOUT);
+            mFillResponseEventLogger.maybeSetDetectionPreference(
+                    getDetectionPreferenceForLogging());
+            final long fillRequestReceivedRelativeTimestamp =
+                    SystemClock.elapsedRealtime() - mLatencyBaseTime;
+            mFillResponseEventLogger.maybeSetLatencyFillResponseReceivedMillis(
+                    (int) (fillRequestReceivedRelativeTimestamp));
+
             unregisterDelayedFillBroadcastLocked();
             if (mDestroyed) {
                 Slog.w(TAG, "Call to Session#onFillRequestFailureOrTimeout(req=" + requestId
@@ -2635,8 +2637,8 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState
                         + id + " destroyed");
                 return;
             }
+            mSaveEventLogger.maybeSetLatencySaveRequestMillis();
         }
-        mSaveEventLogger.maybeSetLatencySaveRequestMillis();
         mHandler.sendMessage(obtainMessage(
                 AutofillManagerServiceImpl::handleSessionSave,
                 mService, this));
@@ -3227,7 +3229,9 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState
         mHandler.sendMessage(obtainMessage(Session::handleLogContextCommitted, this,
                 Event.NO_SAVE_UI_REASON_NONE,
                 COMMIT_REASON_UNKNOWN));
-        logAllEvents(COMMIT_REASON_UNKNOWN);
+        synchronized (mLock) {
+            logAllEventsLocked(COMMIT_REASON_UNKNOWN);
+        }
     }
 
     /**
@@ -4571,7 +4575,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState
 
                     FillResponse response = viewState.getSecondaryResponse();
                     if (response != null) {
-                        logPresentationStatsOnViewEntered(response, isCredmanRequested);
+                        logPresentationStatsOnViewEnteredLocked(response, isCredmanRequested);
                     }
 
                     // If the ViewState is ready to be displayed, onReady() will be called.
@@ -4662,7 +4666,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState
 
                 FillResponse response = viewState.getResponse();
                 if (response != null) {
-                    logPresentationStatsOnViewEntered(response, isCredmanRequested);
+                    logPresentationStatsOnViewEnteredLocked(response, isCredmanRequested);
                 }
 
                 if (isSameViewEntered) {
@@ -4703,7 +4707,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState
     }
 
     @GuardedBy("mLock")
-    private void logPresentationStatsOnViewEntered(FillResponse response,
+    private void logPresentationStatsOnViewEnteredLocked(FillResponse response,
             boolean isCredmanRequested) {
         mPresentationStatsEventLogger.maybeSetRequestId(response.getRequestId());
         mPresentationStatsEventLogger.maybeSetIsCredentialRequest(isCredmanRequested);
@@ -5061,7 +5065,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState
 
         getUiForShowing().showFillDialog(filledId, response, filterText,
                 mService.getServicePackageName(), mComponentName, serviceIcon, this,
-                id, mCompatMode, mPresentationStatsEventLogger);
+                id, mCompatMode, mPresentationStatsEventLogger, mLock);
         return true;
     }
 
@@ -5420,7 +5424,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState
      * Sets the state of views that failed to autofill.
      */
     @GuardedBy("mLock")
-    void setViewAutofilled(@NonNull AutofillId id) {
+    void setViewAutofilledLocked(@NonNull AutofillId id) {
         if (sVerbose) {
             Slog.v(TAG, "View autofilled: " + id);
         }
@@ -6663,7 +6667,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState
     }
 
     @GuardedBy("mLock")
-    private void logAllEvents(@AutofillCommitReason int val) {
+    private void logAllEventsLocked(@AutofillCommitReason int val) {
         if (sVerbose) {
             Slog.v(TAG, "logAllEvents(" + id + "): commitReason: " + val);
         }
@@ -6695,7 +6699,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState
         if (sVerbose) {
             Slog.v(TAG, "destroyLocked for session: " + id);
         }
-        logAllEvents(COMMIT_REASON_SESSION_DESTROYED);
+        logAllEventsLocked(COMMIT_REASON_SESSION_DESTROYED);
 
         if (mDestroyed) {
             return null;
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 3b9c54f79e61..8cc666b538ec 100644
--- a/services/autofill/java/com/android/server/autofill/ui/AutoFillUI.java
+++ b/services/autofill/java/com/android/server/autofill/ui/AutoFillUI.java
@@ -425,7 +425,8 @@ public final class AutoFillUI {
             @Nullable String filterText, @Nullable String servicePackageName,
             @NonNull ComponentName componentName, @Nullable Drawable serviceIcon,
             @NonNull AutoFillUiCallback callback, int sessionId, boolean compatMode,
-            @Nullable PresentationStatsEventLogger mPresentationStatsEventLogger) {
+            @Nullable PresentationStatsEventLogger presentationStatsEventLogger,
+            @NonNull Object sessionLock) {
         if (sVerbose) {
             Slog.v(TAG, "showFillDialog for "
                     + componentName.toShortString() + ": " + response);
@@ -467,9 +468,11 @@ public final class AutoFillUI {
                         @Override
                         public void onDatasetPicked(Dataset dataset) {
                             log(MetricsEvent.TYPE_ACTION);
-                            if (mPresentationStatsEventLogger != null) {
-                                mPresentationStatsEventLogger.maybeSetPositiveCtaButtonClicked(
-                                    true);
+                            synchronized (sessionLock) {
+                                if (presentationStatsEventLogger != null) {
+                                    presentationStatsEventLogger.maybeSetPositiveCtaButtonClicked(
+                                            true);
+                                }
                             }
                             hideFillDialogUiThread(callback);
                             if (mCallback != null) {
@@ -482,8 +485,10 @@ public final class AutoFillUI {
                         @Override
                         public void onDismissed() {
                             log(MetricsEvent.TYPE_DISMISS);
-                            if (mPresentationStatsEventLogger != null) {
-                                mPresentationStatsEventLogger.maybeSetDialogDismissed(true);
+                            synchronized (sessionLock) {
+                                if (presentationStatsEventLogger != null) {
+                                    presentationStatsEventLogger.maybeSetDialogDismissed(true);
+                                }
                             }
                             hideFillDialogUiThread(callback);
                             callback.requestShowSoftInput(focusedId);
@@ -493,9 +498,11 @@ public final class AutoFillUI {
                         @Override
                         public void onCanceled() {
                             log(MetricsEvent.TYPE_CLOSE);
-                            if (mPresentationStatsEventLogger != null) {
-                                mPresentationStatsEventLogger.maybeSetNegativeCtaButtonClicked(
-                                    true);
+                            synchronized (sessionLock) {
+                                if (presentationStatsEventLogger != null) {
+                                    presentationStatsEventLogger.maybeSetNegativeCtaButtonClicked(
+                                            true);
+                                }
                             }
                             hideFillDialogUiThread(callback);
                             callback.requestShowSoftInput(focusedId);