Fixed Content Capture workflow when service package is updated.

The workflow already handles the case where the service dies and the framework re-establishes
the binder connection, but that didn't work when it died because the package was being updated.

This CL fixes this scenario by gracefully pausing the existing sessions before the package is
updated, then resuming them afterwards.

Test: manual verification
Test: atest CtsContentCaptureServiceTestCases # sanity check minus flakiness

Bug: 126266412
Fixes: 129072171

Change-Id: Ibc6b723c7bc08b4f920436f365d6006bc8fac7b6

9 files changed, 185 insertions, 84 deletions

diff --git a/core/java/android/service/contentcapture/ContentCaptureService.java b/core/java/android/service/contentcapture/ContentCaptureService.java
index df113979bacf..413d77d20497 100644
--- a/core/java/android/service/contentcapture/ContentCaptureService.java
+++ b/core/java/android/service/contentcapture/ContentCaptureService.java
@@ -367,7 +367,6 @@ public abstract class ContentCaptureService extends Service {
             stateFlags = initialState;
         } else {
             stateFlags |= ContentCaptureSession.STATE_DISABLED;
-
         }
         setClientState(clientReceiver, stateFlags, mClientInterface.asBinder());
     }
diff --git a/core/java/android/view/contentcapture/ContentCaptureSession.java b/core/java/android/view/contentcapture/ContentCaptureSession.java
index 6d41b289460a..ed1ca2a48850 100644
--- a/core/java/android/view/contentcapture/ContentCaptureSession.java
+++ b/core/java/android/view/contentcapture/ContentCaptureSession.java
@@ -135,11 +135,18 @@ public abstract class ContentCaptureSession implements AutoCloseable {
     public static final int STATE_SERVICE_DIED = 0x400;
 
     /**
+     * Session is disabled because the service package is being udpated.
+     *
+     * @hide
+     */
+    public static final int STATE_SERVICE_UPDATING = 0x800;
+
+    /**
      * Session is enabled, after the service died and came back to live.
      *
      * @hide
      */
-    public static final int STATE_SERVICE_RESURRECTED = 0x800;
+    public static final int STATE_SERVICE_RESURRECTED = 0x1000;
 
     private static final int INITIAL_CHILDREN_CAPACITY = 5;
 
diff --git a/core/java/android/view/contentcapture/MainContentCaptureSession.java b/core/java/android/view/contentcapture/MainContentCaptureSession.java
index 666af5968f69..790b8f9dde46 100644
--- a/core/java/android/view/contentcapture/MainContentCaptureSession.java
+++ b/core/java/android/view/contentcapture/MainContentCaptureSession.java
@@ -230,7 +230,8 @@ public final class MainContentCaptureSession extends ContentCaptureSession {
 
     /**
      * Callback from {@code system_server} after call to
-     * {@link IContentCaptureManager#startSession(IBinder, ComponentName, String, int, IBinder)}
+     * {@link IContentCaptureManager#startSession(IBinder, ComponentName, String, int,
+     * IResultReceiver)}.
      *
      * @param resultCode session state
      * @param binder handle to {@code IContentCaptureDirectManager}
diff --git a/core/java/com/android/internal/infra/AbstractRemoteService.java b/core/java/com/android/internal/infra/AbstractRemoteService.java
index 5c144d3b5752..0a83fcc6db61 100644
--- a/core/java/com/android/internal/infra/AbstractRemoteService.java
+++ b/core/java/com/android/internal/infra/AbstractRemoteService.java
@@ -481,7 +481,11 @@ public abstract class AbstractRemoteService<S extends AbstractRemoteService<S, I
 
     @Override
     public String toString() {
-        return getClass().getSimpleName() + "[" + mComponentName + "]";
+        return getClass().getSimpleName() + "[" + mComponentName
+                + " " + System.identityHashCode(this)
+                + (mService != null ? " (bound)" : " (unbound)")
+                + (mDestroyed ? " (destroyed)" : "")
+                + "]";
     }
 
     /**
diff --git a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java
index 9995d8e4d893..0d7723f1daca 100644
--- a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java
+++ b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java
@@ -167,6 +167,22 @@ public final class ContentCaptureManagerService extends
     }
 
     @Override // from AbstractMasterSystemService
+    protected void onServicePackageUpdatingLocked(int userId) {
+        final ContentCapturePerUserService service = getServiceForUserLocked(userId);
+        if (service != null) {
+            service.onPackageUpdatingLocked();
+        }
+    }
+
+    @Override // from AbstractMasterSystemService
+    protected void onServicePackageUpdatedLocked(@UserIdInt int userId) {
+        final ContentCapturePerUserService service = getServiceForUserLocked(userId);
+        if (service != null) {
+            service.onPackageUpdatedLocked();
+        }
+    }
+
+    @Override // from AbstractMasterSystemService
     protected void enforceCallingPermissionForManagement() {
         getContext().enforceCallingPermission(MANAGE_CONTENT_CAPTURE, mTag);
     }
diff --git a/services/contentcapture/java/com/android/server/contentcapture/ContentCapturePerUserService.java b/services/contentcapture/java/com/android/server/contentcapture/ContentCapturePerUserService.java
index d7d97b39959f..b164f62bc079 100644
--- a/services/contentcapture/java/com/android/server/contentcapture/ContentCapturePerUserService.java
+++ b/services/contentcapture/java/com/android/server/contentcapture/ContentCapturePerUserService.java
@@ -74,7 +74,7 @@ final class ContentCapturePerUserService
         AbstractPerUserSystemService<ContentCapturePerUserService, ContentCaptureManagerService>
         implements ContentCaptureServiceCallbacks {
 
-    private static final String TAG = ContentCaptureManagerService.class.getSimpleName();
+    private static final String TAG = ContentCapturePerUserService.class.getSimpleName();
 
     @GuardedBy("mLock")
     private final ArrayMap<String, ContentCaptureServerSession> mSessions =
@@ -87,7 +87,8 @@ final class ContentCapturePerUserService
      * master's cache (for example, because a temporary service was set).
      */
     @GuardedBy("mLock")
-    private RemoteContentCaptureService mRemoteService;
+    @Nullable
+    RemoteContentCaptureService mRemoteService;
 
     private final ContentCaptureServiceRemoteCallback mRemoteServiceCallback =
             new ContentCaptureServiceRemoteCallback();
@@ -135,6 +136,10 @@ final class ContentCapturePerUserService
         }
 
         if (!disabled) {
+            if (mMaster.debug) {
+                Slog.d(TAG, "updateRemoteService(): creating new remote service for "
+                        + serviceComponentName);
+            }
             mRemoteService = new RemoteContentCaptureService(mMaster.getContext(),
                     ContentCaptureService.SERVICE_INTERFACE, serviceComponentName,
                     mRemoteServiceCallback, mUserId, this, mMaster.isBindInstantServiceAllowed(),
@@ -182,20 +187,40 @@ final class ContentCapturePerUserService
                 }
 
                 mZombie = false;
-                final int numSessions = mSessions.size();
-                if (mMaster.debug) {
-                    Slog.d(TAG, "Ressurrecting remote service (" + mRemoteService + ") on "
-                            + numSessions + " sessions");
-                }
-
-                for (int i = 0; i < numSessions; i++) {
-                    final ContentCaptureServerSession session = mSessions.valueAt(i);
-                    session.resurrectLocked();
-                }
+                resurrectSessionsLocked();
             }
         }
     }
 
+    private void resurrectSessionsLocked() {
+        final int numSessions = mSessions.size();
+        if (mMaster.debug) {
+            Slog.d(TAG, "Ressurrecting remote service (" + mRemoteService + ") on "
+                    + numSessions + " sessions");
+        }
+
+        for (int i = 0; i < numSessions; i++) {
+            final ContentCaptureServerSession session = mSessions.valueAt(i);
+            session.resurrectLocked();
+        }
+    }
+
+    void onPackageUpdatingLocked() {
+        final int numSessions = mSessions.size();
+        if (mMaster.debug) {
+            Slog.d(TAG, "Pausing " + numSessions + " sessions while package is updating");
+        }
+        for (int i = 0; i < numSessions; i++) {
+            final ContentCaptureServerSession session = mSessions.valueAt(i);
+            session.pauseLocked();
+        }
+    }
+
+    void onPackageUpdatedLocked() {
+        updateRemoteServiceLocked(!isEnabledLocked());
+        resurrectSessionsLocked();
+    }
+
     // TODO(b/119613670): log metrics
     @GuardedBy("mLock")
     public void startSessionLocked(@NonNull IBinder activityToken,
@@ -274,9 +299,12 @@ final class ContentCapturePerUserService
             return;
         }
 
+        // Make sure service is bound, just in case the initial connection failed somehow
+        mRemoteService.ensureBoundLocked();
+
         final ContentCaptureServerSession newSession = new ContentCaptureServerSession(
-                activityToken, this, mRemoteService, componentName, clientReceiver, taskId,
-                displayId, sessionId, uid, flags);
+                activityToken, this, componentName, clientReceiver, taskId, displayId, sessionId,
+                uid, flags);
         if (mMaster.verbose) {
             Slog.v(TAG, "startSession(): new session for "
                     + ComponentName.flattenToShortString(componentName) + " and id " + sessionId);
@@ -290,21 +318,6 @@ final class ContentCapturePerUserService
         return mWhitelistHelper.isWhitelisted(componentName);
     }
 
-    /**
-     * @throws IllegalArgumentException if packages or components are empty.
-     */
-    private void setWhitelist(@Nullable List<String> packages,
-            @Nullable List<ComponentName> components) {
-        // TODO(b/122595322): add CTS test for when it's null
-        synchronized (mLock) {
-            if (mMaster.verbose) {
-                Slog.v(TAG, "whitelisting packages: " + packages + " and activities: "
-                        + components);
-            }
-            mWhitelistHelper.setWhitelist(packages, components);
-        }
-    }
-
     // TODO(b/119613670): log metrics
     @GuardedBy("mLock")
     public void finishSessionLocked(@NonNull String sessionId) {
@@ -518,12 +531,16 @@ final class ContentCapturePerUserService
         @Override
         public void setContentCaptureWhitelist(List<String> packages,
                 List<ComponentName> activities) {
+            // TODO(b/122595322): add CTS test for when it's null
             if (mMaster.verbose) {
-                Slog.v(TAG, "setContentCaptureWhitelist(packages=" + packages + ", activities="
-                        + activities + ")");
+                Slog.v(TAG, "setContentCaptureWhitelist(" + (packages == null
+                        ? "null_packages" : packages.size() + " packages")
+                        + ", " + (activities == null
+                        ? "null_activities" : activities.size() + " activities") + ")");
+            }
+            synchronized (mLock) {
+                mWhitelistHelper.setWhitelist(packages, activities);
             }
-            setWhitelist(packages, activities);
-
             // TODO(b/119613670): log metrics
         }
 
diff --git a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureServerSession.java b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureServerSession.java
index da198362f5cb..9b2c05f7fa79 100644
--- a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureServerSession.java
+++ b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureServerSession.java
@@ -15,6 +15,12 @@
  */
 package com.android.server.contentcapture;
 
+import static android.service.contentcapture.ContentCaptureService.setClientState;
+import static android.view.contentcapture.ContentCaptureSession.STATE_ACTIVE;
+import static android.view.contentcapture.ContentCaptureSession.STATE_DISABLED;
+import static android.view.contentcapture.ContentCaptureSession.STATE_SERVICE_RESURRECTED;
+import static android.view.contentcapture.ContentCaptureSession.STATE_SERVICE_UPDATING;
+
 import android.annotation.NonNull;
 import android.content.ComponentName;
 import android.os.IBinder;
@@ -23,7 +29,6 @@ import android.service.contentcapture.SnapshotData;
 import android.util.LocalLog;
 import android.util.Slog;
 import android.view.contentcapture.ContentCaptureContext;
-import android.view.contentcapture.ContentCaptureSession;
 import android.view.contentcapture.ContentCaptureSessionId;
 
 import com.android.internal.annotations.GuardedBy;
@@ -38,7 +43,6 @@ final class ContentCaptureServerSession {
 
     final IBinder mActivityToken;
     private final ContentCapturePerUserService mService;
-    private final RemoteContentCaptureService mRemoteService;
 
     // NOTE: this is the "internal" context (like package and taskId), not the explicit content
     // set by apps - those are only send to the ContentCaptureService.
@@ -61,15 +65,13 @@ final class ContentCaptureServerSession {
     private final int mUid;
 
     ContentCaptureServerSession(@NonNull IBinder activityToken,
-            @NonNull ContentCapturePerUserService service,
-            @NonNull RemoteContentCaptureService remoteService,
-            @NonNull ComponentName appComponentName, @NonNull IResultReceiver sessionStateReceiver,
+            @NonNull ContentCapturePerUserService service, @NonNull ComponentName appComponentName,
+            @NonNull IResultReceiver sessionStateReceiver,
             int taskId, int displayId, @NonNull String sessionId, int uid, int flags) {
         mActivityToken = activityToken;
         mService = service;
         mId = Preconditions.checkNotNull(sessionId);
         mUid = uid;
-        mRemoteService = remoteService;
         mContentCaptureContext = new ContentCaptureContext(/* clientContext= */ null,
                 appComponentName, taskId, displayId, flags);
         mSessionStateReceiver = sessionStateReceiver;
@@ -87,8 +89,12 @@ final class ContentCaptureServerSession {
      */
     @GuardedBy("mLock")
     public void notifySessionStartedLocked(@NonNull IResultReceiver clientReceiver) {
-        mRemoteService.onSessionStarted(mContentCaptureContext, mId, mUid, clientReceiver,
-                ContentCaptureSession.STATE_ACTIVE);
+        if (mService.mRemoteService == null) {
+            Slog.w(TAG, "notifySessionStartedLocked(): no remote service");
+            return;
+        }
+        mService.mRemoteService.onSessionStarted(mContentCaptureContext, mId, mUid, clientReceiver,
+                STATE_ACTIVE);
     }
 
     /**
@@ -101,7 +107,11 @@ final class ContentCaptureServerSession {
             logHistory.log("snapshot: id=" + mId);
         }
 
-        mRemoteService.onActivitySnapshotRequest(mId, snapshotData);
+        if (mService.mRemoteService == null) {
+            Slog.w(TAG, "sendActivitySnapshotLocked(): no remote service");
+            return;
+        }
+        mService.mRemoteService.onActivitySnapshotRequest(mId, snapshotData);
     }
 
     /**
@@ -134,7 +144,11 @@ final class ContentCaptureServerSession {
         }
         // TODO(b/111276913): must call client to set session as FINISHED_BY_SERVER
         if (notifyRemoteService) {
-            mRemoteService.onSessionFinished(mId);
+            if (mService.mRemoteService == null) {
+                Slog.w(TAG, "destroyLocked(): no remote service");
+                return;
+            }
+            mService.mRemoteService.onSessionFinished(mId);
         }
     }
 
@@ -143,10 +157,27 @@ final class ContentCaptureServerSession {
      */
     @GuardedBy("mLock")
     public void resurrectLocked() {
-        mRemoteService.onSessionStarted(new ContentCaptureContext(mContentCaptureContext,
+        final RemoteContentCaptureService remoteService = mService.mRemoteService;
+        if (remoteService == null) {
+            Slog.w(TAG, "destroyLocked(: no remote service");
+            return;
+        }
+        if (mService.isVerbose()) {
+            Slog.v(TAG, "resurrecting " + mActivityToken + " on " + remoteService);
+        }
+        remoteService.onSessionStarted(new ContentCaptureContext(mContentCaptureContext,
                 ContentCaptureContext.FLAG_RECONNECTED), mId, mUid, mSessionStateReceiver,
-                ContentCaptureSession.STATE_ACTIVE
-                        | ContentCaptureSession.STATE_SERVICE_RESURRECTED);
+                STATE_ACTIVE | STATE_SERVICE_RESURRECTED);
+    }
+
+    /**
+     * Called to pause the session while the service is being updated.
+     */
+    @GuardedBy("mLock")
+    public void pauseLocked() {
+        if (mService.isVerbose()) Slog.v(TAG, "pausing " + mActivityToken);
+        setClientState(mSessionStateReceiver, STATE_DISABLED | STATE_SERVICE_UPDATING,
+                /* binder= */ null);
     }
 
     @GuardedBy("mLock")
diff --git a/services/contentcapture/java/com/android/server/contentcapture/RemoteContentCaptureService.java b/services/contentcapture/java/com/android/server/contentcapture/RemoteContentCaptureService.java
index 2ce5059244a1..df9ccbc499ba 100644
--- a/services/contentcapture/java/com/android/server/contentcapture/RemoteContentCaptureService.java
+++ b/services/contentcapture/java/com/android/server/contentcapture/RemoteContentCaptureService.java
@@ -54,7 +54,7 @@ final class RemoteContentCaptureService
         mIdleUnbindTimeoutMs = idleUnbindTimeoutMs;
 
         // Bind right away, which will trigger a onConnected() on service's
-        scheduleBind();
+        ensureBoundLocked();
     }
 
     @Override // from AbstractRemoteService
@@ -89,6 +89,10 @@ final class RemoteContentCaptureService
         }
     }
 
+    public void ensureBoundLocked() {
+        scheduleBind();
+    }
+
     /**
      * Called by {@link ContentCaptureServerSession} to generate a call to the
      * {@link RemoteContentCaptureService} to indicate the session was created.
diff --git a/services/core/java/com/android/server/infra/AbstractMasterSystemService.java b/services/core/java/com/android/server/infra/AbstractMasterSystemService.java
index 430203de8268..ed894ee0bee8 100644
--- a/services/core/java/com/android/server/infra/AbstractMasterSystemService.java
+++ b/services/core/java/com/android/server/infra/AbstractMasterSystemService.java
@@ -131,14 +131,10 @@ public abstract class AbstractMasterSystemService<M extends AbstractMasterSystem
     private final boolean mRefreshServiceOnPackageUpdate;
 
     /**
-     * Name of the service's package that was active but then was removed because its package
-     * update.
-     *
-     * <p>It's a temporary state set / used by the {@link PackageMonitor} implementation, but
-     * defined here so it can be dumped.
+     * Name of the service packages whose APK are being updated, keyed by user id.
      */
     @GuardedBy("mLock")
-    private String mLastActivePackageName;
+    private SparseArray<String> mUpdatingPackageNames;
 
     /**
      * Default constructor.
@@ -565,6 +561,20 @@ public abstract class AbstractMasterSystemService<M extends AbstractMasterSystem
     }
 
     /**
+     * Called before the package that provides the service for the given user is being updated.
+     */
+    protected void onServicePackageUpdatingLocked(@UserIdInt int userId) {
+        if (verbose) Slog.v(mTag, "onServicePackageUpdatingLocked(" + userId + ")");
+    }
+
+    /**
+     * Called after the package that provides the service for the given user is being updated.
+     */
+    protected void onServicePackageUpdatedLocked(@UserIdInt int userId) {
+        if (verbose) Slog.v(mTag, "onServicePackageUpdated(" + userId + ")");
+    }
+
+    /**
      * Called after the service is removed from the cache.
      */
     @SuppressWarnings("unused")
@@ -602,8 +612,10 @@ public abstract class AbstractMasterSystemService<M extends AbstractMasterSystem
             final int size = mServicesCache.size();
             pw.print(prefix); pw.print("Debug: "); pw.print(realDebug);
             pw.print(" Verbose: "); pw.println(realVerbose);
-            pw.print(" Refresh on package update: "); pw.println(mRefreshServiceOnPackageUpdate);
-            pw.print(" Last active service on update: "); pw.println(mLastActivePackageName);
+            pw.print("Refresh on package update: "); pw.println(mRefreshServiceOnPackageUpdate);
+            if (mUpdatingPackageNames != null) {
+                pw.print("Packages being updated: "); pw.println(mUpdatingPackageNames);
+            }
             if (mServiceNameResolver != null) {
                 pw.print(prefix); pw.print("Name resolver: ");
                 mServiceNameResolver.dumpShort(pw); pw.println();
@@ -644,39 +656,49 @@ public abstract class AbstractMasterSystemService<M extends AbstractMasterSystem
         final PackageMonitor monitor = new PackageMonitor() {
 
             @Override
-            public void onPackageUpdateStarted(String packageName, int uid) {
+            public void onPackageUpdateStarted(@NonNull String packageName, int uid) {
+                if (verbose) Slog.v(mTag, "onPackageUpdateStarted(): " + packageName);
+                final String activePackageName = getActiveServicePackageNameLocked();
+                if (!packageName.equals(activePackageName)) return;
+
+                final int userId = getChangingUserId();
                 synchronized (mLock) {
-                    final String activePackageName = getActiveServicePackageNameLocked();
-                    if (packageName.equals(activePackageName)) {
-                        final int userId = getChangingUserId();
-                        if (mRefreshServiceOnPackageUpdate) {
-                            if (debug) {
-                                Slog.d(mTag, "Removing service for user " + userId
-                                        + " because package " + activePackageName
-                                        + " is being updated");
-                            }
-                            mLastActivePackageName = activePackageName;
-                            removeCachedServiceLocked(userId);
-                        } else {
-                            if (debug) {
-                                Slog.d(mTag, "Holding service for user " + userId
-                                        + " while package " + activePackageName
-                                        + " is being updated");
-                            }
+                    if (mUpdatingPackageNames == null) {
+                        mUpdatingPackageNames = new SparseArray<String>(mServicesCache.size());
+                    }
+                    mUpdatingPackageNames.put(userId, packageName);
+                    onServicePackageUpdatingLocked(userId);
+                    if (mRefreshServiceOnPackageUpdate) {
+                        if (debug) {
+                            Slog.d(mTag, "Removing service for user " + userId + " because package "
+                                    + activePackageName + " is being updated");
+                        }
+                        removeCachedServiceLocked(userId);
+                    } else {
+                        if (debug) {
+                            Slog.d(mTag, "Holding service for user " + userId + " while package "
+                                    + activePackageName + " is being updated");
                         }
                     }
                 }
             }
 
             @Override
-            public void onPackageUpdateFinished(String packageName, int uid) {
+            public void onPackageUpdateFinished(@NonNull String packageName, int uid) {
+                if (verbose) Slog.v(mTag, "onPackageUpdateFinished(): " + packageName);
+                final int userId = getChangingUserId();
                 synchronized (mLock) {
-                    String activePackageName = getActiveServicePackageNameLocked();
-                    if (activePackageName == null) {
-                        activePackageName = mLastActivePackageName;
-                        mLastActivePackageName = null;
-                    }
-                    if (!packageName.equals(activePackageName)) {
+                    final String activePackageName = mUpdatingPackageNames == null ? null
+                            : mUpdatingPackageNames.get(userId);
+                    if (packageName.equals(activePackageName)) {
+                        if (mUpdatingPackageNames != null) {
+                            mUpdatingPackageNames.remove(userId);
+                            if (mUpdatingPackageNames.size() == 0) {
+                                mUpdatingPackageNames = null;
+                            }
+                        }
+                        onServicePackageUpdatedLocked(userId);
+                    } else {
                         handlePackageUpdateLocked(packageName);
                     }
                 }