From 95d4f0dc175f5579e765f49e3b6fa35338b807dc Mon Sep 17 00:00:00 2001 From: Yohei Yukawa Date: Fri, 10 Sep 2021 11:37:30 -0700 Subject: [PATCH] Log errors if RemoteInputConnection is used after IMS#onDestroy() This CL consolidates weak-ref handlings in RemoteInputConnection so that IME developers can find more useful messages in logcat when they are using RemoteInputConnection after InputMethodService#onDestroy(). RemoteInputConnection has been designed to not hold a strong reference to its parent InputMethodService instance. While IME developers can manually keep a strong reference to (Remote)InputConnection object that they obtained from InputMethodService#getCurrentInputConnection(), doing so does not prevent the system from garbage-collecting InputMethodService instance. With this CL, an error message will be shown in logcat if somehow RemoteInputConnection object is still touched after InputMethodService is destroyed, which hopefully can help IME developers realize they might be doing something unexpected. Other than showing error messages in logcat, there is no behavior change in this CL. Bug: 192412909 Bug: 194110780 Test: atest CtsInputMethodTestCases Change-Id: I514630d27c8953f62fdb34cd3133a662b3fbbf76 --- .../RemoteInputConnection.java | 76 ++++++++++++------- 1 file changed, 48 insertions(+), 28 deletions(-) diff --git a/core/java/android/inputmethodservice/RemoteInputConnection.java b/core/java/android/inputmethodservice/RemoteInputConnection.java index 464b421d970d5..1065d041713f0 100644 --- a/core/java/android/inputmethodservice/RemoteInputConnection.java +++ b/core/java/android/inputmethodservice/RemoteInputConnection.java @@ -22,6 +22,7 @@ import android.annotation.NonNull; import android.annotation.Nullable; import android.os.Bundle; import android.os.Handler; +import android.util.Log; import android.view.KeyEvent; import android.view.inputmethod.CompletionInfo; import android.view.inputmethod.CorrectionInfo; @@ -62,8 +63,29 @@ final class RemoteInputConnection implements InputConnection { @NonNull private final IInputContextInvoker mInvoker; + private static final class InputMethodServiceInternalHolder { + @NonNull + private final WeakReference mServiceRef; + + private InputMethodServiceInternalHolder( + @NonNull WeakReference ims) { + mServiceRef = ims; + } + + @AnyThread + @Nullable + public InputMethodServiceInternal getAndWarnIfNull() { + final InputMethodServiceInternal ims = mServiceRef.get(); + if (ims == null) { + Log.e(TAG, "InputMethodService is already destroyed. InputConnection instances" + + " cannot be used beyond InputMethodService lifetime.", new Throwable()); + } + return ims; + } + } + @NonNull - private final WeakReference mInputMethodService; + private final InputMethodServiceInternalHolder mImsInternal; @MissingMethodFlags private final int mMissingMethods; @@ -81,7 +103,7 @@ final class RemoteInputConnection implements InputConnection { @NonNull WeakReference inputMethodService, IInputContext inputContext, @MissingMethodFlags int missingMethods, @NonNull CancellationGroup cancellationGroup) { - mInputMethodService = inputMethodService; + mImsInternal = new InputMethodServiceInternalHolder(inputMethodService); mInvoker = IInputContextInvoker.create(inputContext); mMissingMethods = missingMethods; mCancellationGroup = cancellationGroup; @@ -101,11 +123,11 @@ final class RemoteInputConnection implements InputConnection { final CharSequence result = CompletableFutureUtil.getResultOrNull( value, TAG, "getTextAfterCursor()", mCancellationGroup, MAX_WAIT_TIME_MILLIS); - final InputMethodServiceInternal inputMethodService = mInputMethodService.get(); - if (inputMethodService != null && ImeTracing.getInstance().isEnabled()) { + final InputMethodServiceInternal imsInternal = mImsInternal.getAndWarnIfNull(); + if (imsInternal != null && ImeTracing.getInstance().isEnabled()) { final byte[] icProto = InputConnectionProtoDumper.buildGetTextAfterCursorProto(length, flags, result); - inputMethodService.triggerServiceDump(TAG + "#getTextAfterCursor", icProto); + imsInternal.triggerServiceDump(TAG + "#getTextAfterCursor", icProto); } return result; @@ -125,11 +147,11 @@ final class RemoteInputConnection implements InputConnection { final CharSequence result = CompletableFutureUtil.getResultOrNull( value, TAG, "getTextBeforeCursor()", mCancellationGroup, MAX_WAIT_TIME_MILLIS); - final InputMethodServiceInternal inputMethodService = mInputMethodService.get(); - if (inputMethodService != null && ImeTracing.getInstance().isEnabled()) { + final InputMethodServiceInternal imsInternal = mImsInternal.getAndWarnIfNull(); + if (imsInternal != null && ImeTracing.getInstance().isEnabled()) { final byte[] icProto = InputConnectionProtoDumper.buildGetTextBeforeCursorProto(length, flags, result); - inputMethodService.triggerServiceDump(TAG + "#getTextBeforeCursor", icProto); + imsInternal.triggerServiceDump(TAG + "#getTextBeforeCursor", icProto); } return result; @@ -149,11 +171,11 @@ final class RemoteInputConnection implements InputConnection { final CharSequence result = CompletableFutureUtil.getResultOrNull( value, TAG, "getSelectedText()", mCancellationGroup, MAX_WAIT_TIME_MILLIS); - final InputMethodServiceInternal inputMethodService = mInputMethodService.get(); - if (inputMethodService != null && ImeTracing.getInstance().isEnabled()) { + final InputMethodServiceInternal imsInternal = mImsInternal.getAndWarnIfNull(); + if (imsInternal != null && ImeTracing.getInstance().isEnabled()) { final byte[] icProto = InputConnectionProtoDumper.buildGetSelectedTextProto(flags, result); - inputMethodService.triggerServiceDump(TAG + "#getSelectedText", icProto); + imsInternal.triggerServiceDump(TAG + "#getSelectedText", icProto); } return result; @@ -187,11 +209,11 @@ final class RemoteInputConnection implements InputConnection { final SurroundingText result = CompletableFutureUtil.getResultOrNull( value, TAG, "getSurroundingText()", mCancellationGroup, MAX_WAIT_TIME_MILLIS); - final InputMethodServiceInternal inputMethodService = mInputMethodService.get(); - if (inputMethodService != null && ImeTracing.getInstance().isEnabled()) { + final InputMethodServiceInternal imsInternal = mImsInternal.getAndWarnIfNull(); + if (imsInternal != null && ImeTracing.getInstance().isEnabled()) { final byte[] icProto = InputConnectionProtoDumper.buildGetSurroundingTextProto( beforeLength, afterLength, flags, result); - inputMethodService.triggerServiceDump(TAG + "#getSurroundingText", icProto); + imsInternal.triggerServiceDump(TAG + "#getSurroundingText", icProto); } return result; @@ -207,11 +229,11 @@ final class RemoteInputConnection implements InputConnection { final int result = CompletableFutureUtil.getResultOrZero( value, TAG, "getCursorCapsMode()", mCancellationGroup, MAX_WAIT_TIME_MILLIS); - final InputMethodServiceInternal inputMethodService = mInputMethodService.get(); - if (inputMethodService != null && ImeTracing.getInstance().isEnabled()) { + final InputMethodServiceInternal imsInternal = mImsInternal.getAndWarnIfNull(); + if (imsInternal != null && ImeTracing.getInstance().isEnabled()) { final byte[] icProto = InputConnectionProtoDumper.buildGetCursorCapsModeProto( reqModes, result); - inputMethodService.triggerServiceDump(TAG + "#getCursorCapsMode", icProto); + imsInternal.triggerServiceDump(TAG + "#getCursorCapsMode", icProto); } return result; @@ -227,11 +249,11 @@ final class RemoteInputConnection implements InputConnection { final ExtractedText result = CompletableFutureUtil.getResultOrNull( value, TAG, "getExtractedText()", mCancellationGroup, MAX_WAIT_TIME_MILLIS); - final InputMethodServiceInternal inputMethodService = mInputMethodService.get(); - if (inputMethodService != null && ImeTracing.getInstance().isEnabled()) { + final InputMethodServiceInternal imsInternal = mImsInternal.getAndWarnIfNull(); + if (imsInternal != null && ImeTracing.getInstance().isEnabled()) { final byte[] icProto = InputConnectionProtoDumper.buildGetExtractedTextProto( request, flags, result); - inputMethodService.triggerServiceDump(TAG + "#getExtractedText", icProto); + imsInternal.triggerServiceDump(TAG + "#getExtractedText", icProto); } return result; @@ -248,12 +270,11 @@ final class RemoteInputConnection implements InputConnection { @AnyThread private void notifyUserActionIfNecessary() { - final InputMethodServiceInternal inputMethodService = mInputMethodService.get(); - if (inputMethodService == null) { - // This basically should not happen, because it's the the caller of this method. + final InputMethodServiceInternal imsInternal = mImsInternal.getAndWarnIfNull(); + if (imsInternal == null) { return; } - inputMethodService.notifyUserActionIfNecessary(); + imsInternal.notifyUserActionIfNecessary(); } @AnyThread @@ -400,12 +421,11 @@ final class RemoteInputConnection implements InputConnection { } if ((flags & InputConnection.INPUT_CONTENT_GRANT_READ_URI_PERMISSION) != 0) { - final InputMethodServiceInternal inputMethodService = mInputMethodService.get(); - if (inputMethodService == null) { - // This basically should not happen, because it's the caller of this method. + final InputMethodServiceInternal imsInternal = mImsInternal.getAndWarnIfNull(); + if (imsInternal == null) { return false; } - inputMethodService.exposeContent(inputContentInfo, this); + imsInternal.exposeContent(inputContentInfo, this); } final CompletableFuture value =