From 39233ff029c522d790008aa25cef8b295a141b4a Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Wed, 28 Nov 2018 16:54:23 -0800 Subject: [PATCH] Refactored AbstractRemoteService in 2 classes. AbstractRemoteService was spun off from RemoteFillService, which only supports 1 pending request while not bound to the service - if a new request comes, it cancels the previous one. This behavior is fine for Autofill, but for Content Capture all requests must be queued. In fact, the upcoming CTS tests for Content Capture were failing because the 1st PendingOnContentCaptureEventsRequest would cancel the pending PendingSessionLifecycleRequest. So, this CL fix this problem by creating 2 subclasses: - AbstractSinglePendingRequestRemoteService - AbstractMultiplePendingRequestsRemoteService Test: atest CtsContentCaptureServiceTestCases CtsAutoFillServiceTestCases Bug: 111276913 Bug: 117779333 Change-Id: I43bb98be16f5def037f85a415e1f77799adf156e --- .../intelligence/SmartSuggestionsService.java | 13 +- .../intelligence/ContentCaptureManager.java | 2 +- .../server/autofill/RemoteFillService.java | 27 ++-- .../com/android/server/autofill/Session.java | 2 +- ...tMultiplePendingRequestsRemoteService.java | 96 ++++++++++++++ .../android/server/AbstractRemoteService.java | 117 ++++++++++-------- ...ractSinglePendingRequestRemoteService.java | 82 ++++++++++++ .../IntelligencePerUserService.java | 2 +- .../RemoteIntelligenceService.java | 7 +- 9 files changed, 278 insertions(+), 70 deletions(-) create mode 100644 services/core/java/com/android/server/AbstractMultiplePendingRequestsRemoteService.java create mode 100644 services/core/java/com/android/server/AbstractSinglePendingRequestRemoteService.java diff --git a/core/java/android/service/intelligence/SmartSuggestionsService.java b/core/java/android/service/intelligence/SmartSuggestionsService.java index 90f04ca66db79..0e29e70ecc3b6 100644 --- a/core/java/android/service/intelligence/SmartSuggestionsService.java +++ b/core/java/android/service/intelligence/SmartSuggestionsService.java @@ -60,6 +60,7 @@ public abstract class SmartSuggestionsService extends Service { // TODO(b/111330312): STOPSHIP use dynamic value, or change to false static final boolean DEBUG = true; + static final boolean VERBOSE = false; /** * The {@link Intent} that must be declared as handled by the service. @@ -209,7 +210,11 @@ public abstract class SmartSuggestionsService extends Service { * @param sessionId the session's Id */ public void onCreateInteractionSession(@NonNull InteractionContext context, - @NonNull InteractionSessionId sessionId) {} + @NonNull InteractionSessionId sessionId) { + if (VERBOSE) { + Log.v(TAG, "onCreateInteractionSession(id=" + sessionId + ", ctx=" + context + ")"); + } + } /** * Notifies the service of {@link ContentCaptureEvent events} associated with a content capture @@ -319,7 +324,11 @@ public abstract class SmartSuggestionsService extends Service { * * @param sessionId the id of the session to destroy */ - public void onDestroyInteractionSession(@NonNull InteractionSessionId sessionId) {} + public void onDestroyInteractionSession(@NonNull InteractionSessionId sessionId) { + if (VERBOSE) { + Log.v(TAG, "onDestroyInteractionSession(id=" + sessionId + ")"); + } + } /** @hide */ static final class AutofillProxy { diff --git a/core/java/android/view/intelligence/ContentCaptureManager.java b/core/java/android/view/intelligence/ContentCaptureManager.java index 9023cd08159a7..45518d5e5943c 100644 --- a/core/java/android/view/intelligence/ContentCaptureManager.java +++ b/core/java/android/view/intelligence/ContentCaptureManager.java @@ -189,7 +189,7 @@ public final class ContentCaptureManager { } } - private void handleSessionStarted(int resultCode) { + private void handleSessionStarted(int resultCode) { mState = resultCode; mDisabled.set(mState == STATE_DISABLED); if (VERBOSE) { diff --git a/services/autofill/java/com/android/server/autofill/RemoteFillService.java b/services/autofill/java/com/android/server/autofill/RemoteFillService.java index 9aa9d7c528180..af65759548420 100644 --- a/services/autofill/java/com/android/server/autofill/RemoteFillService.java +++ b/services/autofill/java/com/android/server/autofill/RemoteFillService.java @@ -40,9 +40,9 @@ import android.service.autofill.SaveRequest; import android.text.format.DateUtils; import android.util.Slog; -import com.android.server.AbstractRemoteService; +import com.android.server.AbstractSinglePendingRequestRemoteService; -final class RemoteFillService extends AbstractRemoteService { +final class RemoteFillService extends AbstractSinglePendingRequestRemoteService { private static final long TIMEOUT_IDLE_BIND_MILLIS = 5 * DateUtils.SECOND_IN_MILLIS; private static final long TIMEOUT_REMOTE_REQUEST_MILLIS = 5 * DateUtils.SECOND_IN_MILLIS; @@ -69,8 +69,8 @@ final class RemoteFillService extends AbstractRemoteService { mCallbacks = callbacks; } - @Override - protected void onConnectedStateChanged(boolean state) { + @Override // from AbstractRemoteService + protected void handleOnConnectedStateChanged(boolean state) { if (mAutoFillService == null) { Slog.w(mTag, "onConnectedStateChanged(): null service"); return; @@ -82,18 +82,18 @@ final class RemoteFillService extends AbstractRemoteService { } } - @Override + @Override // from AbstractRemoteService protected IInterface getServiceInterface(IBinder service) { mAutoFillService = IAutoFillService.Stub.asInterface(service); return mAutoFillService; } - @Override + @Override // from AbstractRemoteService protected long getTimeoutIdleBindMillis() { return TIMEOUT_IDLE_BIND_MILLIS; } - @Override + @Override // from AbstractRemoteService protected long getRemoteRequestMillis() { return TIMEOUT_REMOTE_REQUEST_MILLIS; } @@ -136,6 +136,19 @@ final class RemoteFillService extends AbstractRemoteService { scheduleRequest(new PendingSaveRequest(request, this)); } + private boolean handleResponseCallbackCommon( + @NonNull PendingRequest pendingRequest) { + if (isDestroyed()) return false; + + if (mPendingRequest == pendingRequest) { + mPendingRequest = null; + } + if (mPendingRequest == null) { + scheduleUnbind(); + } + return true; + } + private void dispatchOnFillRequestSuccess(@NonNull PendingFillRequest pendingRequest, @Nullable FillResponse response, int requestFlags) { mHandler.post(() -> { diff --git a/services/autofill/java/com/android/server/autofill/Session.java b/services/autofill/java/com/android/server/autofill/Session.java index 8676f7f5bea0e..4c645076eb952 100644 --- a/services/autofill/java/com/android/server/autofill/Session.java +++ b/services/autofill/java/com/android/server/autofill/Session.java @@ -902,7 +902,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState // VultureCallback @Override - public void onServiceDied(AbstractRemoteService service) { + public void onServiceDied(AbstractRemoteService> service) { Slog.w(TAG, "removing session because service died"); forceRemoveSelfLocked(); } diff --git a/services/core/java/com/android/server/AbstractMultiplePendingRequestsRemoteService.java b/services/core/java/com/android/server/AbstractMultiplePendingRequestsRemoteService.java new file mode 100644 index 0000000000000..f532b22cb8d42 --- /dev/null +++ b/services/core/java/com/android/server/AbstractMultiplePendingRequestsRemoteService.java @@ -0,0 +1,96 @@ +/* + * Copyright (C) 2018 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; + +import android.annotation.NonNull; +import android.content.ComponentName; +import android.content.Context; +import android.util.Slog; + +import java.io.PrintWriter; +import java.util.ArrayList; + +/** + * Base class representing a remote service that can queue multiple pending requests while not + * bound. + * + * @param the concrete remote service class + * + * @hide + */ +public abstract class AbstractMultiplePendingRequestsRemoteService< + S extends AbstractMultiplePendingRequestsRemoteService> + extends AbstractRemoteService { + + private final int mInitialCapacity; + + protected ArrayList> mPendingRequests; + + public AbstractMultiplePendingRequestsRemoteService(@NonNull Context context, + @NonNull String serviceInterface, @NonNull ComponentName componentName, int userId, + @NonNull VultureCallback callback, boolean bindInstantServiceAllowed, boolean verbose, + int initialCapacity) { + super(context, serviceInterface, componentName, userId, callback, bindInstantServiceAllowed, + verbose); + mInitialCapacity = initialCapacity; + } + + @Override // from AbstractRemoteService + void handlePendingRequests() { + if (mPendingRequests != null) { + final int size = mPendingRequests.size(); + if (mVerbose) Slog.v(mTag, "Sending " + size + " pending requests"); + for (int i = 0; i < size; i++) { + mPendingRequests.get(i).run(); + } + mPendingRequests = null; + } + } + + @Override // from AbstractRemoteService + protected void handleOnDestroy() { + if (mPendingRequests != null) { + final int size = mPendingRequests.size(); + if (mVerbose) Slog.v(mTag, "Canceling " + size + " pending requests"); + for (int i = 0; i < size; i++) { + mPendingRequests.get(i).cancel(); + } + mPendingRequests = null; + } + } + + @Override // from AbstractRemoteService + public void dump(@NonNull String prefix, @NonNull PrintWriter pw) { + super.dump(prefix, pw); + + pw.append(prefix).append("initialCapacity=").append(String.valueOf(mInitialCapacity)) + .println(); + final int size = mPendingRequests == null ? 0 : mPendingRequests.size(); + pw.append(prefix).append("pendingRequests=").append(String.valueOf(size)).println(); + } + + @Override // from AbstractRemoteService + void handlePendingRequestWhileUnBound(@NonNull PendingRequest pendingRequest) { + if (mPendingRequests == null) { + mPendingRequests = new ArrayList<>(mInitialCapacity); + } + mPendingRequests.add(pendingRequest); + if (mVerbose) { + Slog.v(mTag, "queued " + mPendingRequests.size() + " requests; last=" + pendingRequest); + } + } +} diff --git a/services/core/java/com/android/server/AbstractRemoteService.java b/services/core/java/com/android/server/AbstractRemoteService.java index 0d4cf6b01ba84..f636487c666b0 100644 --- a/services/core/java/com/android/server/AbstractRemoteService.java +++ b/services/core/java/com/android/server/AbstractRemoteService.java @@ -45,13 +45,20 @@ import java.lang.ref.WeakReference; * *

All state of this class is modified on a handler thread. * + *

NOTE: this class should not be extended directly, you should extend either + * {@link AbstractSinglePendingRequestRemoteService} or + * {@link AbstractMultiplePendingRequestsRemoteService}. + * *

See {@code com.android.server.autofill.RemoteFillService} for a concrete * (no pun intended) example of how to use it. * + * @param the concrete remote service class + * * @hide */ //TODO(b/117779333): improve javadoc above instead of using Autofill as an example -public abstract class AbstractRemoteService implements DeathRecipient { +public abstract class AbstractRemoteService> + implements DeathRecipient { private static final int MSG_UNBIND = 1; @@ -64,8 +71,6 @@ public abstract class AbstractRemoteService implements DeathRecipient { protected final Handler mHandler; protected final ComponentName mComponentName; - protected PendingRequest mPendingRequest; - private final Context mContext; private final Intent mIntent; private final VultureCallback mVultureCallback; @@ -88,10 +93,11 @@ public abstract class AbstractRemoteService implements DeathRecipient { * * @param service service that died! */ - void onServiceDied(AbstractRemoteService service); + void onServiceDied(AbstractRemoteService> service); } - public AbstractRemoteService(@NonNull Context context, @NonNull String serviceInterface, + // NOTE: must be package-protected so this class is not extend outside + AbstractRemoteService(@NonNull Context context, @NonNull String serviceInterface, @NonNull ComponentName componentName, int userId, @NonNull VultureCallback callback, boolean bindInstantServiceAllowed, boolean verbose) { mContext = context; @@ -118,12 +124,25 @@ public abstract class AbstractRemoteService implements DeathRecipient { return mDestroyed; } + private void handleOnConnectedStateChangedInternal(boolean connected) { + if (connected) { + handlePendingRequests(); + } + handleOnConnectedStateChanged(connected); + } + /** - * Callback called when the system connected / disconnected to the service. + * Handles the pending requests when the connection it bounds to the remote service. + */ + abstract void handlePendingRequests(); + + /** + * Callback called when the system connected / disconnected to the service and the pending + * requests have been handled. * * @param state {@code true} when connected, {@code false} when disconnected. */ - protected void onConnectedStateChanged(boolean state) { + protected void handleOnConnectedStateChanged(boolean state) { } /** @@ -144,14 +163,18 @@ public abstract class AbstractRemoteService implements DeathRecipient { private void handleDestroy() { if (checkIfDestroyed()) return; - if (mPendingRequest != null) { - mPendingRequest.cancel(); - mPendingRequest = null; - } - ensureUnbound(); + handleOnDestroy(); + handleEnsureUnbound(); mDestroyed = true; } + /** + * Clears the state when this object is destroyed. + * + *

Typically used to cancel the pending requests. + */ + protected abstract void handleOnDestroy(); + @Override // from DeathRecipient public void binderDied() { mHandler.sendMessage(obtainMessage(AbstractRemoteService::handleBinderDied, this)); @@ -183,9 +206,7 @@ public abstract class AbstractRemoteService implements DeathRecipient { pw.append(prefix).append(tab).append("destroyed=") .append(String.valueOf(mDestroyed)).println(); pw.append(prefix).append(tab).append("bound=") - .append(String.valueOf(isBound())).println(); - pw.append(prefix).append(tab).append("hasPendingRequest=") - .append(String.valueOf(mPendingRequest != null)).println(); + .append(String.valueOf(handleIsBound())).println(); pw.append(prefix).append("mBindInstantServiceAllowed=").println(mBindInstantServiceAllowed); pw.append(prefix).append("idleTimeout=") .append(Long.toString(getTimeoutIdleBindMillis() / 1000)).append("s").println(); @@ -194,7 +215,7 @@ public abstract class AbstractRemoteService implements DeathRecipient { pw.println(); } - protected void scheduleRequest(PendingRequest pendingRequest) { + protected void scheduleRequest(@NonNull PendingRequest pendingRequest) { mHandler.sendMessage(obtainMessage( AbstractRemoteService::handlePendingRequest, this, pendingRequest)); } @@ -215,19 +236,20 @@ public abstract class AbstractRemoteService implements DeathRecipient { private void handleUnbind() { if (checkIfDestroyed()) return; - ensureUnbound(); + handleEnsureUnbound(); } - private void handlePendingRequest( - PendingRequest pendingRequest) { + /** + * Handles a request, either processing it right now when bound, or saving it to be handled when + * bound. + */ + protected final void handlePendingRequest(@NonNull PendingRequest pendingRequest) { if (checkIfDestroyed() || mCompleted) return; - if (!isBound()) { - if (mPendingRequest != null) { - mPendingRequest.cancel(); - } - mPendingRequest = pendingRequest; - ensureBound(); + if (!handleIsBound()) { + if (mVerbose) Slog.v(mTag, "handlePendingRequest(): queuing" + pendingRequest); + handlePendingRequestWhileUnBound(pendingRequest); + handleEnsureBound(); } else { if (mVerbose) Slog.v(mTag, "handlePendingRequest(): " + pendingRequest); pendingRequest.run(); @@ -237,12 +259,17 @@ public abstract class AbstractRemoteService implements DeathRecipient { } } - private boolean isBound() { + /** + * Defines what to do with a request that arrives while not bound to the service. + */ + abstract void handlePendingRequestWhileUnBound(@NonNull PendingRequest pendingRequest); + + private boolean handleIsBound() { return mServiceInterface != null; } - private void ensureBound() { - if (isBound() || mBinding) return; + private void handleEnsureBound() { + if (handleIsBound() || mBinding) return; if (mVerbose) Slog.v(mTag, "ensureBound()"); mBinding = true; @@ -265,13 +292,13 @@ public abstract class AbstractRemoteService implements DeathRecipient { } } - private void ensureUnbound() { - if (!isBound() && !mBinding) return; + private void handleEnsureUnbound() { + if (!handleIsBound() && !mBinding) return; if (mVerbose) Slog.v(mTag, "ensureUnbound()"); mBinding = false; - if (isBound()) { - onConnectedStateChanged(false); + if (handleIsBound()) { + handleOnConnectedStateChangedInternal(false); if (mServiceInterface != null) { mServiceInterface.asBinder().unlinkToDeath(this, 0); mServiceInterface = null; @@ -283,6 +310,7 @@ public abstract class AbstractRemoteService implements DeathRecipient { private class RemoteServiceConnection implements ServiceConnection { @Override public void onServiceConnected(ComponentName name, IBinder service) { + if (mVerbose) Slog.v(mTag, "onServiceConnected()"); if (mDestroyed || !mBinding) { // This is abnormal. Unbinding the connection has been requested already. Slog.wtf(mTag, "onServiceConnected() was dispatched after unbindService."); @@ -296,15 +324,7 @@ public abstract class AbstractRemoteService implements DeathRecipient { handleBinderDied(); return; } - onConnectedStateChanged(true); - - if (mPendingRequest != null) { - final PendingRequest pendingRequest = - mPendingRequest; - mPendingRequest = null; - handlePendingRequest(pendingRequest); - } - + handleOnConnectedStateChangedInternal(true); mServiceDied = false; } @@ -325,25 +345,12 @@ public abstract class AbstractRemoteService implements DeathRecipient { return mDestroyed; } - protected boolean handleResponseCallbackCommon( - PendingRequest pendingRequest) { - if (isDestroyed()) return false; - - if (mPendingRequest == pendingRequest) { - mPendingRequest = null; - } - if (mPendingRequest == null) { - scheduleUnbind(); - } - return true; - } - /** * Base class for the requests serviced by the remote service. * * @param the remote service class */ - public abstract static class PendingRequest + public abstract static class PendingRequest> implements Runnable { protected final String mTag = getClass().getSimpleName(); protected final Object mLock = new Object(); diff --git a/services/core/java/com/android/server/AbstractSinglePendingRequestRemoteService.java b/services/core/java/com/android/server/AbstractSinglePendingRequestRemoteService.java new file mode 100644 index 0000000000000..8e1f540a4d5ef --- /dev/null +++ b/services/core/java/com/android/server/AbstractSinglePendingRequestRemoteService.java @@ -0,0 +1,82 @@ +/* + * Copyright (C) 2018 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; + +import android.annotation.NonNull; +import android.content.ComponentName; +import android.content.Context; +import android.util.Slog; + +import java.io.PrintWriter; + +/** + * Base class representing a remote service that can have only one pending requests while not bound. + * + *

If another request is received while not bound, the previous one will be canceled. + * + * @param the concrete remote service class + * + * @hide + */ +public abstract class AbstractSinglePendingRequestRemoteService< + S extends AbstractSinglePendingRequestRemoteService> extends AbstractRemoteService { + + protected PendingRequest mPendingRequest; + + public AbstractSinglePendingRequestRemoteService(@NonNull Context context, + @NonNull String serviceInterface, @NonNull ComponentName componentName, int userId, + @NonNull VultureCallback callback, boolean bindInstantServiceAllowed, + boolean verbose) { + super(context, serviceInterface, componentName, userId, callback, bindInstantServiceAllowed, + verbose); + } + + @Override // from AbstractRemoteService + void handlePendingRequests() { + if (mPendingRequest != null) { + final PendingRequest pendingRequest = mPendingRequest; + mPendingRequest = null; + handlePendingRequest(pendingRequest); + } + } + + @Override // from AbstractRemoteService + protected void handleOnDestroy() { + if (mPendingRequest != null) { + mPendingRequest.cancel(); + mPendingRequest = null; + } + } + + @Override // from AbstractRemoteService + public void dump(@NonNull String prefix, @NonNull PrintWriter pw) { + super.dump(prefix, pw); + pw.append(prefix).append("hasPendingRequest=") + .append(String.valueOf(mPendingRequest != null)).println(); + } + + @Override // from AbstractRemoteService + void handlePendingRequestWhileUnBound(@NonNull PendingRequest pendingRequest) { + if (mPendingRequest != null) { + if (mVerbose) { + Slog.v(mTag, "handlePendingRequestWhileUnBound(): cancelling " + mPendingRequest); + } + mPendingRequest.cancel(); + } + mPendingRequest = pendingRequest; + } +} diff --git a/services/intelligence/java/com/android/server/intelligence/IntelligencePerUserService.java b/services/intelligence/java/com/android/server/intelligence/IntelligencePerUserService.java index 84e06b04768c9..e3b09c6304994 100644 --- a/services/intelligence/java/com/android/server/intelligence/IntelligencePerUserService.java +++ b/services/intelligence/java/com/android/server/intelligence/IntelligencePerUserService.java @@ -200,7 +200,7 @@ final class IntelligencePerUserService return; } if (mMaster.verbose) { - Slog.v(TAG, "sendEvents(): id=" + sessionId + "; events =" + events.size()); + Slog.v(TAG, "sendEvents(): id=" + sessionId + ", events=" + events.size()); } session.sendEventsLocked(events); } diff --git a/services/intelligence/java/com/android/server/intelligence/RemoteIntelligenceService.java b/services/intelligence/java/com/android/server/intelligence/RemoteIntelligenceService.java index 5ebb99eeddcf4..d9f4f20dc971a 100644 --- a/services/intelligence/java/com/android/server/intelligence/RemoteIntelligenceService.java +++ b/services/intelligence/java/com/android/server/intelligence/RemoteIntelligenceService.java @@ -36,12 +36,13 @@ import android.view.autofill.IAutoFillManagerClient; import android.view.intelligence.ContentCaptureEvent; import com.android.internal.os.IResultReceiver; -import com.android.server.AbstractRemoteService; +import com.android.server.AbstractMultiplePendingRequestsRemoteService; import java.util.List; //TODO(b/111276913): rename once the final name is defined -final class RemoteIntelligenceService extends AbstractRemoteService { +final class RemoteIntelligenceService + extends AbstractMultiplePendingRequestsRemoteService { private static final String TAG = "RemoteIntelligenceService"; @@ -56,7 +57,7 @@ final class RemoteIntelligenceService extends AbstractRemoteService { RemoteIntelligenceServiceCallbacks callbacks, boolean bindInstantServiceAllowed, boolean verbose) { super(context, serviceInterface, componentName, userId, callbacks, - bindInstantServiceAllowed, verbose); + bindInstantServiceAllowed, verbose, /* initialCapacity= */ 2); mCallbacks = callbacks; }