Fixed cancel() and commit():

- Call removeSelfLocked() on cancelSessionLocked.
- Call removeSelf() after dispatching the PendingSaveRequest.
- Added a finalizer callback to PendingSaveRequest that calls removeSelf().
- Fixed SaveUi SnackBar so its actions are only triggered once.
- Reused removeSelfLocked() when needed.
- Removed unnecessary {} on some lambdas.
- Removed unnecessary mLock on PendingSaveRequest.
- Removed unnecessary mLock usage on PendingFillRequest.

Test: CtsAutoFillServiceTestCases (including new tests) pass
Test: manual verification

Fixes: 35721501
Fixes: 35844249

Change-Id: I9789218777b62a9558a602b8eaed0714d8b77fa0

4 files changed, 135 insertions, 60 deletions

diff --git a/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java b/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java
index cde39468244c..df76009db008 100644
--- a/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java
+++ b/services/autofill/java/com/android/server/autofill/AutofillManagerServiceImpl.java
@@ -235,9 +235,8 @@ final class AutofillManagerServiceImpl {
                 if (!hasService()) {
                     final int sessionCount = mSessions.size();
                     for (int i = sessionCount - 1; i >= 0; i--) {
-                        Session session = mSessions.valueAt(i);
-                        session.destroyLocked();
-                        mSessions.removeAt(i);
+                        final Session session = mSessions.valueAt(i);
+                        session.removeSelfLocked();
                     }
                 }
                 sendStateToClients();
@@ -328,7 +327,13 @@ final class AutofillManagerServiceImpl {
             return;
         }
 
-        session.showSaveLocked();
+        final boolean finished = session.showSaveLocked();
+        if (DEBUG) {
+            Log.d(TAG, "finishSessionLocked(): session finished on save? " + finished);
+        }
+        if (finished) {
+            session.removeSelf();
+        }
     }
 
     void cancelSessionLocked(IBinder activityToken) {
@@ -341,8 +346,7 @@ final class AutofillManagerServiceImpl {
             Slog.w(TAG, "cancelSessionLocked(): no session for " + activityToken);
             return;
         }
-
-        session.destroyLocked();
+        session.removeSelfLocked();
     }
 
     private Session createSessionByTokenLocked(@NonNull IBinder activityToken,
@@ -754,9 +758,7 @@ final class AutofillManagerServiceImpl {
             synchronized (mLock) {
                 fillInIntent = createAuthFillInIntent(mStructure);
             }
-            mHandlerCaller.getHandler().post(() -> {
-                startAuthentication(intent, fillInIntent);
-            });
+            mHandlerCaller.getHandler().post(() -> startAuthentication(intent, fillInIntent));
         }
 
         // FillServiceCallbacks
@@ -776,8 +778,7 @@ final class AutofillManagerServiceImpl {
                 Binder.restoreCallingIdentity(identity);
             }
             synchronized (mLock) {
-                destroyLocked();
-                mSessions.remove(this);
+                removeSelfLocked();
             }
         }
 
@@ -790,9 +791,7 @@ final class AutofillManagerServiceImpl {
         // AutoFillUiCallback
         @Override
         public void fill(Dataset dataset) {
-            mHandlerCaller.getHandler().post(() -> {
-                autoFill(dataset);
-            });
+            mHandlerCaller.getHandler().post(() -> autoFill(dataset));
         }
 
         // AutoFillUiCallback
@@ -805,17 +804,13 @@ final class AutofillManagerServiceImpl {
         // AutoFillUiCallback
         @Override
         public void cancelSave() {
-            mHandlerCaller.getHandler().post(() -> {
-                removeSelf();
-            });
+            mHandlerCaller.getHandler().post(() -> removeSelf());
         }
 
         // AutoFillUiCallback
         @Override
         public void onEvent(AutofillId id, int event) {
-            mHandlerCaller.getHandler().post(() -> {
-                notifyChangeToClient(id, event);
-            });
+            mHandlerCaller.getHandler().post(() -> notifyChangeToClient(id, event));
         }
 
         public void setAuthenticationResultLocked(Bundle data) {
@@ -845,12 +840,14 @@ final class AutofillManagerServiceImpl {
         }
 
         /**
-         * Show the save UI, when session can be saved.
+         * Shows the save UI, when session can be saved.
+         *
+         * @return {@code true} if session is done, or {@code false} if it's pending user action.
          */
-        public void showSaveLocked() {
+        public boolean showSaveLocked() {
             if (mStructure == null) {
                 Slog.wtf(TAG, "showSaveLocked(): no mStructure");
-                return;
+                return true;
             }
             if (mCurrentResponse == null) {
                 // Happens when the activity / session was finished before the service replied, or
@@ -858,7 +855,7 @@ final class AutofillManagerServiceImpl {
                 if (DEBUG) {
                     Slog.d(TAG, "showSaveLocked(): no mCurrentResponse");
                 }
-                return;
+                return true;
             }
             final SaveInfo saveInfo = mCurrentResponse.getSaveInfo();
             if (DEBUG) {
@@ -874,13 +871,13 @@ final class AutofillManagerServiceImpl {
              */
 
             if (saveInfo == null) {
-                return;
+                return true;
             }
 
             final AutofillId[] requiredIds = saveInfo.getRequiredIds();
             if (requiredIds == null || requiredIds.length == 0) {
                 Slog.w(TAG, "showSaveLocked(): no required ids on saveInfo");
-                return;
+                return true;
             }
 
             boolean allRequiredAreNotEmpty = true;
@@ -950,7 +947,7 @@ final class AutofillManagerServiceImpl {
                     getUiForShowing().showSaveUi(
                             mInfo.getServiceInfo().loadLabel(mContext.getPackageManager()),
                             saveInfo, mPackageName);
-                    return;
+                    return false;
                 }
             }
             // Nothing changed...
@@ -959,7 +956,7 @@ final class AutofillManagerServiceImpl {
                         + "allRequiredAreNotNull=" + allRequiredAreNotEmpty
                         + ", atLeastOneChanged=" + atLeastOneChanged);
             }
-            removeSelf();
+            return true;
         }
 
         /**
@@ -1001,7 +998,7 @@ final class AutofillManagerServiceImpl {
                 mStructure.dump();
             }
 
-            mRemoteFillService.onSaveRequest(mStructure, extras);
+            mRemoteFillService.onSaveRequest(mStructure, extras, () -> removeSelf());
         }
 
         void updateLocked(AutofillId id, Rect bounds, AutofillValue value, int flags) {
@@ -1255,15 +1252,18 @@ final class AutofillManagerServiceImpl {
                     mPackageName);
         }
 
-        private void removeSelf() {
-            if (VERBOSE) {
-                Slog.v(TAG, "removeSelf()");
+        void removeSelf() {
+            synchronized (mLock) {
+                removeSelfLocked();
             }
+        }
 
-            synchronized (mLock) {
-                destroyLocked();
-                mSessions.remove(mActivityToken);
+        private void removeSelfLocked() {
+            if (VERBOSE) {
+                Slog.v(TAG, "removeSelfLocked()");
             }
+            destroyLocked();
+            mSessions.remove(mActivityToken);
         }
     }
 }
diff --git a/services/autofill/java/com/android/server/autofill/RemoteFillService.java b/services/autofill/java/com/android/server/autofill/RemoteFillService.java
index c41ac05ef6aa..d1c8b4f84a91 100644
--- a/services/autofill/java/com/android/server/autofill/RemoteFillService.java
+++ b/services/autofill/java/com/android/server/autofill/RemoteFillService.java
@@ -16,6 +16,8 @@
 
 package com.android.server.autofill;
 
+import static com.android.server.autofill.Helper.DEBUG;
+
 import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.app.assist.AssistStructure;
@@ -38,8 +40,10 @@ import android.service.autofill.IFillCallback;
 import android.service.autofill.ISaveCallback;
 import android.text.format.DateUtils;
 import android.util.Slog;
+
 import com.android.internal.os.HandlerCaller;
 import com.android.server.FgThread;
+import com.android.server.autofill.AutofillManagerServiceImpl.Session;
 
 import java.io.PrintWriter;
 import java.lang.ref.WeakReference;
@@ -55,8 +59,6 @@ import java.lang.ref.WeakReference;
 final class RemoteFillService implements DeathRecipient {
     private static final String LOG_TAG = "RemoteFillService";
 
-    private static final boolean DEBUG = Helper.DEBUG;
-
     // How long after the last interaction with the service we would unbind
     private static final long TIMEOUT_IDLE_BIND_MILLIS = 5 * DateUtils.SECOND_IN_MILLIS;
 
@@ -139,9 +141,10 @@ final class RemoteFillService implements DeathRecipient {
         mHandler.obtainMessageO(MyHandler.MSG_ON_PENDING_REQUEST, request).sendToTarget();
     }
 
-    public void onSaveRequest(@NonNull AssistStructure structure, @Nullable Bundle extras) {
+    public void onSaveRequest(@NonNull AssistStructure structure, @Nullable Bundle extras,
+            @Nullable Runnable finalizer) {
         cancelScheduledUnbind();
-        final PendingSaveRequest request = new PendingSaveRequest(structure, extras, this);
+        final PendingSaveRequest request = new PendingSaveRequest(structure, extras, this, finalizer);
         mHandler.obtainMessageO(MyHandler.MSG_ON_PENDING_REQUEST, request).sendToTarget();
     }
 
@@ -414,8 +417,8 @@ final class RemoteFillService implements DeathRecipient {
     private static final class PendingFillRequest extends PendingRequest {
         private final Object mLock = new Object();
         private final WeakReference<RemoteFillService> mWeakService;
-        private AssistStructure mStructure;
-        private Bundle mExtras;
+        private final AssistStructure mStructure;
+        private final Bundle mExtras;
         private final IFillCallback mCallback;
         private ICancellationSignal mCancellation;
         private boolean mCancelled;
@@ -473,10 +476,6 @@ final class RemoteFillService implements DeathRecipient {
                 try {
                     remoteService.mAutoFillService.onFillRequest(mStructure,
                             mExtras, mCallback, mFlags);
-                    synchronized (mLock) {
-                        mStructure = null;
-                        mExtras = null;
-                    }
                 } catch (RemoteException e) {
                     Slog.e(LOG_TAG, "Error calling on fill request", e);
                     cancel();
@@ -506,17 +505,18 @@ final class RemoteFillService implements DeathRecipient {
     }
 
     private static final class PendingSaveRequest extends PendingRequest {
-        private final Object mLock = new Object();
         private final WeakReference<RemoteFillService> mWeakService;
-        private AssistStructure mStructure;
-        private Bundle mExtras;
+        private final AssistStructure mStructure;
+        private final Bundle mExtras;
         private final ISaveCallback mCallback;
+        private final Runnable mFinalizer;
 
-        public PendingSaveRequest(@NonNull AssistStructure structure,
-                @Nullable Bundle extras, @NonNull RemoteFillService service) {
+        public PendingSaveRequest(@NonNull AssistStructure structure, @Nullable Bundle extras,
+                @NonNull RemoteFillService service, @Nullable Runnable finalizer) {
             mStructure = structure;
             mExtras = extras;
             mWeakService = new WeakReference<>(service);
+            mFinalizer = finalizer;
             mCallback = new ISaveCallback.Stub() {
                 @Override
                 public void onSuccess() {
@@ -540,19 +540,17 @@ final class RemoteFillService implements DeathRecipient {
 
         @Override
         public void run() {
-            RemoteFillService service = mWeakService.get();
+            final RemoteFillService service = mWeakService.get();
             if (service != null) {
                 try {
-                    service.mAutoFillService.onSaveRequest(mStructure,
-                            mExtras, mCallback);
-                    synchronized (mLock) {
-                        mStructure = null;
-                        mExtras = null;
-                    }
+                    service.mAutoFillService.onSaveRequest(mStructure, mExtras, mCallback);
                 } catch (RemoteException e) {
                     Slog.e(LOG_TAG, "Error calling on save request", e);
                 }
             }
+            if (mFinalizer != null) {
+              mFinalizer.run();
+            }
         }
 
         @Override
diff --git a/services/autofill/java/com/android/server/autofill/ui/Helper.java b/services/autofill/java/com/android/server/autofill/ui/Helper.java
new file mode 100644
index 000000000000..996e4213ac44
--- /dev/null
+++ b/services/autofill/java/com/android/server/autofill/ui/Helper.java
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.server.autofill.ui;
+
+final class Helper {
+
+    static final boolean DEBUG = true; // TODO(b/33197203): set to false when stable
+    static final boolean VERBOSE = false;
+    private Helper() {
+        throw new UnsupportedOperationException("contains static members only");
+    }
+}
diff --git a/services/autofill/java/com/android/server/autofill/ui/SaveUi.java b/services/autofill/java/com/android/server/autofill/ui/SaveUi.java
index 644abe60e314..509351bf0858 100644
--- a/services/autofill/java/com/android/server/autofill/ui/SaveUi.java
+++ b/services/autofill/java/com/android/server/autofill/ui/SaveUi.java
@@ -16,6 +16,8 @@
 
 package com.android.server.autofill.ui;
 
+import static com.android.server.autofill.ui.Helper.DEBUG;
+
 import android.annotation.NonNull;
 import android.app.Dialog;
 import android.content.Context;
@@ -23,6 +25,7 @@ import android.content.IntentSender;
 import android.os.Handler;
 import android.service.autofill.SaveInfo;
 import android.text.format.DateUtils;
+import android.util.Slog;
 import android.view.Gravity;
 import android.view.Window;
 import android.view.WindowManager;
@@ -37,23 +40,66 @@ import com.android.server.UiThread;
  * Autofill Save Prompt
  */
 final class SaveUi {
+
+    private static final String TAG = "SaveUi";
+
     public interface OnSaveListener {
         void onSave();
         void onCancel(IntentSender listener);
         void onDestroy();
     }
 
+    private class OneTimeListener implements OnSaveListener {
+
+        private final OnSaveListener mRealListener;
+        private boolean mDone;
+
+        OneTimeListener(OnSaveListener realListener) {
+            mRealListener = realListener;
+        }
+
+        @Override
+        public void onSave() {
+            if (DEBUG) Slog.d(TAG, "onSave(): " + mDone);
+            if (mDone) {
+                return;
+            }
+            mDone = true;
+            mRealListener.onSave();
+        }
+
+        @Override
+        public void onCancel(IntentSender listener) {
+            if (DEBUG) Slog.d(TAG, "onCancel(): " + mDone);
+            if (mDone) {
+                return;
+            }
+            mDone = true;
+            mRealListener.onCancel(listener);
+        }
+
+        @Override
+        public void onDestroy() {
+            if (DEBUG) Slog.d(TAG, "onDestroy(): " + mDone);
+            if (mDone) {
+                return;
+            }
+            mDone = true;
+            mRealListener.onDestroy();
+        }
+    }
+
     private final Handler mHandler = UiThread.getHandler();
 
     private final @NonNull Dialog mDialog;
 
-    private final @NonNull OnSaveListener mListener;
+    private final @NonNull OneTimeListener mListener;
 
     private boolean mDestroyed;
 
     SaveUi(@NonNull Context context, @NonNull CharSequence providerLabel, @NonNull SaveInfo info,
             @NonNull OnSaveListener listener, int lifeTimeMs) {
-        mListener = listener;
+        mListener = new OneTimeListener(listener);
 
         final LayoutInflater inflater = LayoutInflater.from(context);
         final View view = inflater.inflate(R.layout.autofill_save, null);
@@ -118,7 +164,12 @@ final class SaveUi {
 
         mDialog.show();
 
-        mHandler.postDelayed(() -> mListener.onCancel(null), lifeTimeMs);
+        mHandler.postDelayed(() -> {
+            if (!mListener.mDone) {
+                mListener.onCancel(null);
+                Slog.d(TAG, "Save snackbar timed out after " + lifeTimeMs + "ms");
+            }
+        }, lifeTimeMs);
     }
 
     void destroy() {