From 2f28843249aa4e2b645ebd170936ca49be050ac2 Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Thu, 21 Mar 2019 13:57:45 -0700 Subject: 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 --- .../contentcapture/ContentCaptureService.java | 1 - .../view/contentcapture/ContentCaptureSession.java | 9 ++- .../contentcapture/MainContentCaptureSession.java | 3 +- .../internal/infra/AbstractRemoteService.java | 6 +- .../ContentCaptureManagerService.java | 16 ++++ .../ContentCapturePerUserService.java | 83 ++++++++++++-------- .../ContentCaptureServerSession.java | 57 ++++++++++---- .../RemoteContentCaptureService.java | 6 +- .../server/infra/AbstractMasterSystemService.java | 88 ++++++++++++++-------- 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 @@ -134,12 +134,19 @@ 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 implements ContentCaptureServiceCallbacks { - private static final String TAG = ContentCaptureManagerService.class.getSimpleName(); + private static final String TAG = ContentCapturePerUserService.class.getSimpleName(); @GuardedBy("mLock") private final ArrayMap 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 packages, - @Nullable List 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 packages, List 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 AbstractMasterSystemServiceIt'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 mUpdatingPackageNames; /** * Default constructor. @@ -564,6 +560,20 @@ public abstract class AbstractMasterSystemService(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); } } -- cgit v1.2.3-59-g8ed1b