From 99852d7d708515053140ed2d512b3decbd74c8a7 Mon Sep 17 00:00:00 2001 From: Feng Cao Date: Mon, 16 Mar 2020 21:42:48 +0000 Subject: [PATCH 1/2] Revert "Revert "Do not block autofill on waiting for the IME response"" This reverts commit 949ec8ad6baf5516cbfd27b7e24d0e563c1f42a9. Reason for revert: will check in the original patch with a fix Test: manual Test: atest android.autofillservice.cts.inline Bug: 151580124 Bug: 150757314 Change-Id: I37d05eb627595c25018dd82e50db3a9b054c90a5 --- .../autofill/InlineSuggestionSession.java | 79 ++++++++----- .../com/android/server/autofill/Session.java | 105 ++++++++++++++---- 2 files changed, 133 insertions(+), 51 deletions(-) diff --git a/services/autofill/java/com/android/server/autofill/InlineSuggestionSession.java b/services/autofill/java/com/android/server/autofill/InlineSuggestionSession.java index 7fe086df07294..3376f2bd74f58 100644 --- a/services/autofill/java/com/android/server/autofill/InlineSuggestionSession.java +++ b/services/autofill/java/com/android/server/autofill/InlineSuggestionSession.java @@ -23,6 +23,7 @@ import android.annotation.NonNull; import android.annotation.Nullable; import android.content.ComponentName; import android.os.Bundle; +import android.os.Handler; import android.os.RemoteException; import android.util.Log; import android.util.Slog; @@ -38,11 +39,8 @@ import com.android.server.inputmethod.InputMethodManagerInternal; import java.util.Collections; import java.util.Optional; -import java.util.concurrent.CancellationException; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; +import java.util.function.Consumer; /** * Maintains an autofill inline suggestion session that communicates with the IME. @@ -69,7 +67,7 @@ import java.util.concurrent.TimeoutException; final class InlineSuggestionSession { private static final String TAG = "AfInlineSuggestionSession"; - private static final int INLINE_REQUEST_TIMEOUT_MS = 1000; + private static final int INLINE_REQUEST_TIMEOUT_MS = 200; @NonNull private final InputMethodManagerInternal mInputMethodManagerInternal; @@ -80,6 +78,8 @@ final class InlineSuggestionSession { private final Object mLock; @NonNull private final ImeStatusListener mImeStatusListener; + @NonNull + private final Handler mHandler; /** * To avoid the race condition, one should not access {@code mPendingImeResponse} without @@ -105,10 +105,11 @@ final class InlineSuggestionSession { private boolean mImeInputViewStarted = false; InlineSuggestionSession(InputMethodManagerInternal inputMethodManagerInternal, - int userId, ComponentName componentName) { + int userId, ComponentName componentName, Handler handler) { mInputMethodManagerInternal = inputMethodManagerInternal; mUserId = userId; mComponentName = componentName; + mHandler = handler; mLock = new Object(); mImeStatusListener = new ImeStatusListener() { @Override @@ -137,7 +138,8 @@ final class InlineSuggestionSession { }; } - public void onCreateInlineSuggestionsRequest(@NonNull AutofillId autofillId) { + public void onCreateInlineSuggestionsRequest(@NonNull AutofillId autofillId, + @NonNull Consumer requestConsumer) { if (sDebug) Log.d(TAG, "onCreateInlineSuggestionsRequest called for " + autofillId); synchronized (mLock) { @@ -154,26 +156,16 @@ final class InlineSuggestionSession { mUserId, new InlineSuggestionsRequestInfo(mComponentName, autofillId, new Bundle()), new InlineSuggestionsRequestCallbackImpl(mPendingImeResponse, - mImeStatusListener)); + mImeStatusListener, requestConsumer, mHandler, mLock)); } } - public Optional waitAndGetInlineSuggestionsRequest() { + public Optional getInlineSuggestionsRequest() { final CompletableFuture pendingImeResponse = getPendingImeResponse(); - if (pendingImeResponse == null) { + if (pendingImeResponse == null || !pendingImeResponse.isDone()) { return Optional.empty(); } - try { - return Optional.ofNullable(pendingImeResponse.get(INLINE_REQUEST_TIMEOUT_MS, - TimeUnit.MILLISECONDS)).map(ImeResponse::getRequest); - } catch (TimeoutException e) { - Log.w(TAG, "Exception getting inline suggestions request in time: " + e); - } catch (CancellationException e) { - Log.w(TAG, "Inline suggestions request cancelled"); - } catch (InterruptedException | ExecutionException e) { - throw new RuntimeException(e); - } - return Optional.empty(); + return Optional.ofNullable(pendingImeResponse.getNow(null)).map(ImeResponse::getRequest); } public boolean hideInlineSuggestionsUi(@NonNull AutofillId autofillId) { @@ -200,8 +192,7 @@ final class InlineSuggestionSession { if (sDebug) Log.d(TAG, "onInlineSuggestionsResponseLocked without IMS request"); return false; } - // There is no need to wait on the CompletableFuture since it should have been completed - // when {@link #waitAndGetInlineSuggestionsRequest()} was called. + // There is no need to wait on the CompletableFuture since it should have been completed. ImeResponse imeResponse = completedImsResponse.getNow(null); if (imeResponse == null) { if (sDebug) Log.d(TAG, "onInlineSuggestionsResponseLocked with pending IMS response"); @@ -249,20 +240,50 @@ final class InlineSuggestionSession { private static final class InlineSuggestionsRequestCallbackImpl extends IInlineSuggestionsRequestCallback.Stub { + private final Object mLock; + @GuardedBy("mLock") private final CompletableFuture mResponse; + @GuardedBy("mLock") + private final Consumer mRequestConsumer; private final ImeStatusListener mImeStatusListener; + private final Handler mHandler; + private final Runnable mTimeoutCallback; private InlineSuggestionsRequestCallbackImpl(CompletableFuture response, - ImeStatusListener imeStatusListener) { + ImeStatusListener imeStatusListener, + Consumer requestConsumer, + Handler handler, Object lock) { mResponse = response; mImeStatusListener = imeStatusListener; + mRequestConsumer = requestConsumer; + mLock = lock; + + mHandler = handler; + mTimeoutCallback = () -> { + Log.w(TAG, "Timed out waiting for IME callback InlineSuggestionsRequest."); + synchronized (mLock) { + completeIfNotLocked(null); + } + }; + mHandler.postDelayed(mTimeoutCallback, INLINE_REQUEST_TIMEOUT_MS); + } + + private void completeIfNotLocked(@Nullable ImeResponse response) { + if (mResponse.isDone()) { + return; + } + mResponse.complete(response); + mRequestConsumer.accept(response == null ? null : response.mRequest); + mHandler.removeCallbacks(mTimeoutCallback); } @BinderThread @Override public void onInlineSuggestionsUnsupported() throws RemoteException { if (sDebug) Log.d(TAG, "onInlineSuggestionsUnsupported() called."); - mResponse.complete(null); + synchronized (mLock) { + completeIfNotLocked(null); + } } @BinderThread @@ -281,9 +302,13 @@ final class InlineSuggestionSession { mImeStatusListener.onInputMethodFinishInputView(imeFieldId); } if (request != null && callback != null) { - mResponse.complete(new ImeResponse(request, callback)); + synchronized (mLock) { + completeIfNotLocked(new ImeResponse(request, callback)); + } } else { - mResponse.complete(null); + synchronized (mLock) { + completeIfNotLocked(null); + } } } diff --git a/services/autofill/java/com/android/server/autofill/Session.java b/services/autofill/java/com/android/server/autofill/Session.java index 9693535e22864..5064663871e3e 100644 --- a/services/autofill/java/com/android/server/autofill/Session.java +++ b/services/autofill/java/com/android/server/autofill/Session.java @@ -114,7 +114,9 @@ import java.util.Collections; import java.util.List; import java.util.Objects; import java.util.Optional; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Consumer; /** * A session for a given activity. @@ -307,7 +309,47 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState /** * Receiver of assist data from the app's {@link Activity}. */ - private final IAssistDataReceiver mAssistReceiver = new IAssistDataReceiver.Stub() { + private final AssistDataReceiverImpl mAssistReceiver = new AssistDataReceiverImpl(); + + private final class AssistDataReceiverImpl extends IAssistDataReceiver.Stub { + + @GuardedBy("mLock") + private InlineSuggestionsRequest mPendingInlineSuggestionsRequest; + @GuardedBy("mLock") + private FillRequest mPendingFillRequest; + @GuardedBy("mLock") + private CountDownLatch mCountDownLatch; + + @Nullable Consumer newAutofillRequestLocked( + boolean isInlineRequest) { + mCountDownLatch = new CountDownLatch(isInlineRequest ? 2 : 1); + mPendingFillRequest = null; + mPendingInlineSuggestionsRequest = null; + return isInlineRequest ? (inlineSuggestionsRequest) -> { + synchronized (mLock) { + mPendingInlineSuggestionsRequest = inlineSuggestionsRequest; + mCountDownLatch.countDown(); + maybeRequestFillLocked(); + } + } : null; + } + + void maybeRequestFillLocked() { + if (mCountDownLatch == null || mCountDownLatch.getCount() > 0 + || mPendingFillRequest == null) { + return; + } + if (mPendingInlineSuggestionsRequest != null) { + mPendingFillRequest = new FillRequest(mPendingFillRequest.getId(), + mPendingFillRequest.getFillContexts(), mPendingFillRequest.getClientState(), + mPendingFillRequest.getFlags(), mPendingInlineSuggestionsRequest); + } + mRemoteFillService.onFillRequest(mPendingFillRequest); + mPendingInlineSuggestionsRequest = null; + mPendingFillRequest = null; + mCountDownLatch = null; + } + @Override public void onHandleAssistData(Bundle resultData) throws RemoteException { if (mRemoteFillService == null) { @@ -402,17 +444,17 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState final ArrayList contexts = mergePreviousSessionLocked(/* forSave= */ false); - - final Optional inlineSuggestionsRequest = - mInlineSuggestionSession.waitAndGetInlineSuggestionsRequest(); request = new FillRequest(requestId, contexts, mClientState, flags, - inlineSuggestionsRequest.orElse(null)); + /*inlineSuggestionsRequest=*/null); + + mPendingFillRequest = request; + mCountDownLatch.countDown(); + maybeRequestFillLocked(); } if (mActivityToken != null) { mService.sendActivityAssistDataToContentCapture(mActivityToken, resultData); } - mRemoteFillService.onFillRequest(request); } @Override @@ -605,9 +647,15 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState private void maybeRequestInlineSuggestionsRequestThenFillLocked(@NonNull ViewState viewState, int newState, int flags) { if (isInlineSuggestionsEnabledLocked()) { - mInlineSuggestionSession.onCreateInlineSuggestionsRequest(mCurrentViewId); + Consumer inlineSuggestionsRequestConsumer = + mAssistReceiver.newAutofillRequestLocked(/*isInlineRequest=*/ true); + if (inlineSuggestionsRequestConsumer != null) { + mInlineSuggestionSession.onCreateInlineSuggestionsRequest(mCurrentViewId, + inlineSuggestionsRequestConsumer); + } + } else { + mAssistReceiver.newAutofillRequestLocked(/*isInlineRequest=*/ false); } - requestNewFillResponseLocked(viewState, newState, flags); } @@ -708,7 +756,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState setClientLocked(client); mInlineSuggestionSession = new InlineSuggestionSession(inputMethodManagerInternal, userId, - componentName); + componentName, handler); mMetricsLogger.write(newLogMaker(MetricsEvent.AUTOFILL_SESSION_STARTED) .addTaggedData(MetricsEvent.FIELD_AUTOFILL_FLAGS, flags)); @@ -2663,7 +2711,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState private boolean requestShowInlineSuggestionsLocked(@NonNull FillResponse response, @Nullable String filterText) { final Optional inlineSuggestionsRequest = - mInlineSuggestionSession.waitAndGetInlineSuggestionsRequest(); + mInlineSuggestionSession.getInlineSuggestionsRequest(); if (!inlineSuggestionsRequest.isPresent()) { Log.w(TAG, "InlineSuggestionsRequest unavailable"); return false; @@ -2896,6 +2944,9 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState /** * Tries to trigger Augmented Autofill when the standard service could not fulfill a request. * + *

The request may not have been sent when this method returns as it may be waiting for + * the inline suggestion request asynchronously. + * * @return callback to destroy the autofill UI, or {@code null} if not supported. */ // TODO(b/123099468): might need to call it in other places, like when the service returns a @@ -2978,6 +3029,21 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState final AutofillId focusedId = AutofillId.withoutSession(mCurrentViewId); + final Consumer requestAugmentedAutofill = + (inlineSuggestionsRequest) -> { + remoteService.onRequestAutofillLocked(id, mClient, taskId, mComponentName, + focusedId, + currentValue, inlineSuggestionsRequest, + /*inlineSuggestionsCallback=*/ + response -> mInlineSuggestionSession.onInlineSuggestionsResponse( + mCurrentViewId, response), + /*onErrorCallback=*/ () -> { + synchronized (mLock) { + cancelAugmentedAutofillLocked(); + } + }, mService.getRemoteInlineSuggestionRenderServiceLocked()); + }; + // There are 3 cases when augmented autofill should ask IME for a new request: // 1. standard autofill provider is None // 2. standard autofill provider doesn't support inline (and returns null response) @@ -2985,21 +3051,12 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState // doesn't want autofill if (mForAugmentedAutofillOnly || !isInlineSuggestionsEnabledLocked()) { if (sDebug) Slog.d(TAG, "Create inline request for augmented autofill"); - mInlineSuggestionSession.onCreateInlineSuggestionsRequest(mCurrentViewId); + mInlineSuggestionSession.onCreateInlineSuggestionsRequest(mCurrentViewId, + /*requestConsumer=*/ requestAugmentedAutofill); + } else { + requestAugmentedAutofill.accept( + mInlineSuggestionSession.getInlineSuggestionsRequest().orElse(null)); } - - Optional inlineSuggestionsRequest = - mInlineSuggestionSession.waitAndGetInlineSuggestionsRequest(); - remoteService.onRequestAutofillLocked(id, mClient, taskId, mComponentName, focusedId, - currentValue, inlineSuggestionsRequest.orElse(null), - response -> mInlineSuggestionSession.onInlineSuggestionsResponse( - mCurrentViewId, response), - () -> { - synchronized (mLock) { - cancelAugmentedAutofillLocked(); - } - }, mService.getRemoteInlineSuggestionRenderServiceLocked()); - if (mAugmentedAutofillDestroyer == null) { mAugmentedAutofillDestroyer = () -> remoteService.onDestroyAutofillWindowsRequest(); } From 323255dd28a75c7c337541a0ecf0fcf588b189cd Mon Sep 17 00:00:00 2001 From: Feng Cao Date: Mon, 16 Mar 2020 12:46:05 -0700 Subject: [PATCH 2/2] Fixed deadlock in InlineSuggestionSession. * by using the same lock from the autofill Session Test: manual Test: atest android.autofillservice.cts.inline Bug: 151580124 Change-Id: Ia34b1d0ea85c24eddf1cf1b89d07c331bb411023 --- .../autofill/InlineSuggestionSession.java | 36 ++++++++----------- .../com/android/server/autofill/Session.java | 2 +- 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/services/autofill/java/com/android/server/autofill/InlineSuggestionSession.java b/services/autofill/java/com/android/server/autofill/InlineSuggestionSession.java index 3376f2bd74f58..5de817101177a 100644 --- a/services/autofill/java/com/android/server/autofill/InlineSuggestionSession.java +++ b/services/autofill/java/com/android/server/autofill/InlineSuggestionSession.java @@ -62,7 +62,7 @@ import java.util.function.Consumer; * side flow. * *

- * This class is thread safe. + * This class should hold the same lock as {@link Session} as they call into each other. */ final class InlineSuggestionSession { @@ -105,12 +105,12 @@ final class InlineSuggestionSession { private boolean mImeInputViewStarted = false; InlineSuggestionSession(InputMethodManagerInternal inputMethodManagerInternal, - int userId, ComponentName componentName, Handler handler) { + int userId, ComponentName componentName, Handler handler, Object lock) { mInputMethodManagerInternal = inputMethodManagerInternal; mUserId = userId; mComponentName = componentName; mHandler = handler; - mLock = new Object(); + mLock = lock; mImeStatusListener = new ImeStatusListener() { @Override public void onInputMethodStartInputView(AutofillId imeFieldId) { @@ -261,29 +261,27 @@ final class InlineSuggestionSession { mHandler = handler; mTimeoutCallback = () -> { Log.w(TAG, "Timed out waiting for IME callback InlineSuggestionsRequest."); - synchronized (mLock) { - completeIfNotLocked(null); - } + completeIfNot(null); }; mHandler.postDelayed(mTimeoutCallback, INLINE_REQUEST_TIMEOUT_MS); } - private void completeIfNotLocked(@Nullable ImeResponse response) { - if (mResponse.isDone()) { - return; + private void completeIfNot(@Nullable ImeResponse response) { + synchronized (mLock) { + if (mResponse.isDone()) { + return; + } + mResponse.complete(response); + mRequestConsumer.accept(response == null ? null : response.mRequest); + mHandler.removeCallbacks(mTimeoutCallback); } - mResponse.complete(response); - mRequestConsumer.accept(response == null ? null : response.mRequest); - mHandler.removeCallbacks(mTimeoutCallback); } @BinderThread @Override public void onInlineSuggestionsUnsupported() throws RemoteException { if (sDebug) Log.d(TAG, "onInlineSuggestionsUnsupported() called."); - synchronized (mLock) { - completeIfNotLocked(null); - } + completeIfNot(null); } @BinderThread @@ -302,13 +300,9 @@ final class InlineSuggestionSession { mImeStatusListener.onInputMethodFinishInputView(imeFieldId); } if (request != null && callback != null) { - synchronized (mLock) { - completeIfNotLocked(new ImeResponse(request, callback)); - } + completeIfNot(new ImeResponse(request, callback)); } else { - synchronized (mLock) { - completeIfNotLocked(null); - } + completeIfNot(null); } } diff --git a/services/autofill/java/com/android/server/autofill/Session.java b/services/autofill/java/com/android/server/autofill/Session.java index 5064663871e3e..de3111867ab8d 100644 --- a/services/autofill/java/com/android/server/autofill/Session.java +++ b/services/autofill/java/com/android/server/autofill/Session.java @@ -756,7 +756,7 @@ final class Session implements RemoteFillService.FillServiceCallbacks, ViewState setClientLocked(client); mInlineSuggestionSession = new InlineSuggestionSession(inputMethodManagerInternal, userId, - componentName, handler); + componentName, handler, mLock); mMetricsLogger.write(newLogMaker(MetricsEvent.AUTOFILL_SESSION_STARTED) .addTaggedData(MetricsEvent.FIELD_AUTOFILL_FLAGS, flags));