Ensure View#onInputConnectionClosedInternal() timing
This is a follow up CL to our previous CLs [1][2], which introduced
an @hide callback
View#onInputConnectionClosedInternal()
to notify View when an is closed.
What this CL aims to do is to fix a potential problem in a code path
that has not been yet used. Thus there should be, in theory, no
observable app compat impact.
The problem is that
RemoteInputConnectionImpl#deactivate()
can dispatch
A: View#onInputConnectionClosedInternal()
before
B: InputConnection#closeConnection()
is completed when A and B need to be dispatched to two different
threads. This can, in theory, happen when
A: View#onInputConnectionClosedInternal()
C: InputConnection#getHandler()
are both explicitly overridden. That said, A is still @hide and only
by TextView, which basically does not support InputConnection with a
custom InputConnection#getHandler().
Anyway, with this CL A is guaranteed to happen after B under any
circumstances.
[1]: Iaafb0a03126c9292c24415f866dbdd72cadfa239
7b384751ea2a19beb7a635c52545742b2171ae8d
[2]: I9280604e7ec7e8d08c1179e6bbf0068647a41040
7b384751ea2a19beb7a635c52545742b2171ae8d
Bug: 163400105
Test: atest FrameworksCoreTests:ViewInputConnectionTest
Change-Id: I8a0e321ecf6e0b3be4b6ab1a35e6ac7259826c2e
diff --git a/core/java/com/android/internal/inputmethod/RemoteInputConnectionImpl.java b/core/java/com/android/internal/inputmethod/RemoteInputConnectionImpl.java
index 2d44054..0c27012 100644
--- a/core/java/com/android/internal/inputmethod/RemoteInputConnectionImpl.java
+++ b/core/java/com/android/internal/inputmethod/RemoteInputConnectionImpl.java
@@ -121,25 +121,54 @@
// reportFinish() will take effect.
return;
}
- closeConnection();
-
- // Notify the app that the InputConnection was closed.
- final View servedView = mServedView.get();
- if (servedView != null) {
- final Handler handler = servedView.getHandler();
- // The handler is null if the view is already detached. When that's the case, for
- // now, we simply don't dispatch this callback.
- if (handler != null) {
- if (DEBUG) {
- Log.v(TAG, "Calling View.onInputConnectionClosed: view=" + servedView);
+ dispatch(() -> {
+ // Note that we do not need to worry about race condition here, because 1) mFinished is
+ // updated only inside this block, and 2) the code here is running on a Handler hence we
+ // assume multiple closeConnection() tasks will not be handled at the same time.
+ if (isFinished()) {
+ return;
+ }
+ Trace.traceBegin(Trace.TRACE_TAG_INPUT, "InputConnection#closeConnection");
+ try {
+ InputConnection ic = getInputConnection();
+ // Note we do NOT check isActive() here, because this is safe
+ // for an IME to call at any time, and we need to allow it
+ // through to clean up our state after the IME has switched to
+ // another client.
+ if (ic == null) {
+ return;
}
- if (handler.getLooper().isCurrentThread()) {
- servedView.onInputConnectionClosedInternal();
- } else {
- handler.post(servedView::onInputConnectionClosedInternal);
+ @MissingMethodFlags
+ final int missingMethods = InputConnectionInspector.getMissingMethodFlags(ic);
+ if ((missingMethods & MissingMethodFlags.CLOSE_CONNECTION) == 0) {
+ ic.closeConnection();
+ }
+ } finally {
+ synchronized (mLock) {
+ mInputConnection = null;
+ mFinished = true;
+ }
+ Trace.traceEnd(Trace.TRACE_TAG_INPUT);
+ }
+
+ // Notify the app that the InputConnection was closed.
+ final View servedView = mServedView.get();
+ if (servedView != null) {
+ final Handler handler = servedView.getHandler();
+ // The handler is null if the view is already detached. When that's the case, for
+ // now, we simply don't dispatch this callback.
+ if (handler != null) {
+ if (DEBUG) {
+ Log.v(TAG, "Calling View.onInputConnectionClosed: view=" + servedView);
+ }
+ if (handler.getLooper().isCurrentThread()) {
+ servedView.onInputConnectionClosedInternal();
+ } else {
+ handler.post(servedView::onInputConnectionClosedInternal);
+ }
}
}
- }
+ });
}
@Override
@@ -705,39 +734,6 @@
});
}
- private void closeConnection() {
- dispatch(() -> {
- // Note that we do not need to worry about race condition here, because 1) mFinished is
- // updated only inside this block, and 2) the code here is running on a Handler hence we
- // assume multiple closeConnection() tasks will not be handled at the same time.
- if (isFinished()) {
- return;
- }
- Trace.traceBegin(Trace.TRACE_TAG_INPUT, "InputConnection#closeConnection");
- try {
- InputConnection ic = getInputConnection();
- // Note we do NOT check isActive() here, because this is safe
- // for an IME to call at any time, and we need to allow it
- // through to clean up our state after the IME has switched to
- // another client.
- if (ic == null) {
- return;
- }
- @MissingMethodFlags
- final int missingMethods = InputConnectionInspector.getMissingMethodFlags(ic);
- if ((missingMethods & MissingMethodFlags.CLOSE_CONNECTION) == 0) {
- ic.closeConnection();
- }
- } finally {
- synchronized (mLock) {
- mInputConnection = null;
- mFinished = true;
- }
- Trace.traceEnd(Trace.TRACE_TAG_INPUT);
- }
- });
- }
-
@Override
public void commitContent(InputContentInfo inputContentInfo, int flags, Bundle opts,
IBooleanResultCallback callback) {
diff --git a/core/tests/coretests/src/android/view/ViewInputConnectionTest.java b/core/tests/coretests/src/android/view/ViewInputConnectionTest.java
index d667af3..d60c0c6 100644
--- a/core/tests/coretests/src/android/view/ViewInputConnectionTest.java
+++ b/core/tests/coretests/src/android/view/ViewInputConnectionTest.java
@@ -19,13 +19,25 @@
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import android.annotation.DurationMillisLong;
import android.app.Instrumentation;
import android.content.Context;
+import android.graphics.Color;
+import android.os.Bundle;
import android.os.Handler;
+import android.os.HandlerThread;
+import android.os.Process;
+import android.text.InputType;
import android.text.format.DateUtils;
+import android.view.inputmethod.CompletionInfo;
+import android.view.inputmethod.CorrectionInfo;
import android.view.inputmethod.EditorInfo;
+import android.view.inputmethod.ExtractedText;
+import android.view.inputmethod.ExtractedTextRequest;
import android.view.inputmethod.InputConnection;
+import android.view.inputmethod.InputContentInfo;
import android.view.inputmethod.InputMethodManager;
import android.widget.Button;
import android.widget.EditText;
@@ -45,12 +57,23 @@
import org.junit.Test;
import org.junit.runner.RunWith;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+
/**
* Tests for internal APIs/behaviors of {@link View} and {@link InputConnection}.
*/
@MediumTest
@RunWith(AndroidJUnit4.class)
public class ViewInputConnectionTest {
+ @DurationMillisLong
+ private static final long TIMEOUT = 5000;
+ @DurationMillisLong
+ private static final long EXPECTED_TIMEOUT = 500;
+
@Rule
public ActivityTestRule<ViewInputConnectionTestActivity> mActivityRule =
new ActivityTestRule<>(ViewInputConnectionTestActivity.class);
@@ -289,4 +312,282 @@
super.onInputConnectionClosedInternal();
}
}
+
+
+ @Test
+ public void testInputConnectionCallbacks_nonUiThread() throws Throwable {
+ try (InputConnectionHandlingThread thread = new InputConnectionHandlingThread()) {
+ final ViewGroup viewGroup = getOnMainSync(() -> mActivity.findViewById(R.id.root));
+ final TestOffThreadEditor editor = getOnMainSync(() -> {
+ final TestOffThreadEditor myEditor =
+ new TestOffThreadEditor(viewGroup.getContext(), thread.getHandler());
+ viewGroup.addView(myEditor);
+ myEditor.requestFocus();
+ return myEditor;
+ });
+
+ mInstrumentation.waitForIdleSync();
+
+ assertThat(editor.mOnCreateInputConnectionCalled.await(TIMEOUT, TimeUnit.MILLISECONDS))
+ .isTrue();
+
+ // Invalidate the currently used InputConnection by moving the focus to a new EditText.
+ mActivityRule.runOnUiThread(() -> {
+ final EditText editText = new EditText(viewGroup.getContext());
+ viewGroup.addView(editText);
+ editText.requestFocus();
+ });
+
+ // Make sure that InputConnection#closeConnection() gets called on the handler thread.
+ assertThat(editor.mInputConnectionClosedCalled.await(TIMEOUT, TimeUnit.MILLISECONDS))
+ .isTrue();
+ assertThat(editor.mInputConnectionClosedCallingThreadId.get())
+ .isEqualTo(thread.getThreadId());
+
+ // Make sure that View#onInputConnectionClosed() is not yet dispatched, because
+ // InputConnection#closeConnection() is still blocked.
+ assertThat(editor.mOnInputConnectionClosedCalled.await(
+ EXPECTED_TIMEOUT, TimeUnit.MILLISECONDS)).isFalse();
+
+ // Unblock InputConnection#closeConnection()
+ editor.mInputConnectionClosedBlocker.countDown();
+
+ // Make sure that View#onInputConnectionClosed() is dispatched on the main thread.
+ assertThat(editor.mOnInputConnectionClosedCalled.await(TIMEOUT, TimeUnit.MILLISECONDS))
+ .isTrue();
+ assertThat(editor.mInputConnectionClosedBlockerTimedOut.get()).isFalse();
+ assertThat(editor.mOnInputConnectionClosedCallingThreadId.get())
+ .isEqualTo(getOnMainSync(Process::myTid));
+ }
+ }
+
+ private <T> T getOnMainSync(@NonNull Supplier<T> supplier) throws Throwable {
+ final AtomicReference<T> result = new AtomicReference<>();
+ mActivityRule.runOnUiThread(() -> result.set(supplier.get()));
+ return result.get();
+ }
+
+ private static class TestOffThreadEditor extends View {
+ private static final int TEST_VIEW_HEIGHT = 10;
+
+ public CountDownLatch mOnCreateInputConnectionCalled = new CountDownLatch(1);
+ public CountDownLatch mInputConnectionClosedCalled = new CountDownLatch(1);
+ public CountDownLatch mInputConnectionClosedBlocker = new CountDownLatch(1);
+ public AtomicBoolean mInputConnectionClosedBlockerTimedOut = new AtomicBoolean();
+ public AtomicReference<Integer> mInputConnectionClosedCallingThreadId =
+ new AtomicReference<>();
+
+ public CountDownLatch mOnInputConnectionClosedCalled = new CountDownLatch(1);
+ public AtomicReference<Integer> mOnInputConnectionClosedCallingThreadId =
+ new AtomicReference<>();
+
+ private final Handler mInputConnectionHandler;
+
+ TestOffThreadEditor(Context context, @NonNull Handler inputConnectionHandler) {
+ super(context);
+ setBackgroundColor(Color.YELLOW);
+ setLayoutParams(new ViewGroup.LayoutParams(
+ ViewGroup.LayoutParams.MATCH_PARENT, TEST_VIEW_HEIGHT));
+ setFocusableInTouchMode(true);
+ setFocusable(true);
+ mInputConnectionHandler = inputConnectionHandler;
+ }
+
+ @Override
+ public InputConnection onCreateInputConnection(EditorInfo outAttrs) {
+ mOnCreateInputConnectionCalled.countDown();
+ outAttrs.inputType = InputType.TYPE_CLASS_TEXT;
+ return new NoOpInputConnection() {
+ @Override
+ public Handler getHandler() {
+ return mInputConnectionHandler;
+ }
+
+ @Override
+ public void closeConnection() {
+ mInputConnectionClosedCallingThreadId.compareAndSet(null, Process.myTid());
+ mInputConnectionClosedCalled.countDown();
+ try {
+ if (mInputConnectionClosedBlocker.await(TIMEOUT, TimeUnit.MILLISECONDS)) {
+ return;
+ }
+ } catch (InterruptedException e) {
+ }
+ mInputConnectionClosedBlockerTimedOut.set(true);
+ }
+ };
+ }
+
+ @Override
+ public boolean onCheckIsTextEditor() {
+ return true;
+ }
+
+ @Override
+ public void onInputConnectionClosedInternal() {
+ mOnInputConnectionClosedCallingThreadId.compareAndSet(null, Process.myTid());
+ mOnInputConnectionClosedCalled.countDown();
+ super.onInputConnectionClosedInternal();
+ }
+ }
+
+ static class NoOpInputConnection implements InputConnection {
+
+ @Override
+ public CharSequence getTextBeforeCursor(int n, int flags) {
+ return null;
+ }
+
+ @Override
+ public CharSequence getTextAfterCursor(int n, int flags) {
+ return null;
+ }
+
+ @Override
+ public CharSequence getSelectedText(int flags) {
+ return null;
+ }
+
+ @Override
+ public int getCursorCapsMode(int reqModes) {
+ return 0;
+ }
+
+ @Override
+ public ExtractedText getExtractedText(ExtractedTextRequest request, int flags) {
+ return null;
+ }
+
+ @Override
+ public boolean deleteSurroundingText(int beforeLength, int afterLength) {
+ return false;
+ }
+
+ @Override
+ public boolean deleteSurroundingTextInCodePoints(int beforeLength, int afterLength) {
+ return false;
+ }
+
+ @Override
+ public boolean setComposingText(CharSequence text, int newCursorPosition) {
+ return false;
+ }
+
+ @Override
+ public boolean setComposingRegion(int start, int end) {
+ return false;
+ }
+
+ @Override
+ public boolean finishComposingText() {
+ return false;
+ }
+
+ @Override
+ public boolean commitText(CharSequence text, int newCursorPosition) {
+ return false;
+ }
+
+ @Override
+ public boolean commitCompletion(CompletionInfo text) {
+ return false;
+ }
+
+ @Override
+ public boolean commitCorrection(CorrectionInfo correctionInfo) {
+ return false;
+ }
+
+ @Override
+ public boolean setSelection(int start, int end) {
+ return false;
+ }
+
+ @Override
+ public boolean performEditorAction(int editorAction) {
+ return false;
+ }
+
+ @Override
+ public boolean performContextMenuAction(int id) {
+ return false;
+ }
+
+ @Override
+ public boolean beginBatchEdit() {
+ return false;
+ }
+
+ @Override
+ public boolean endBatchEdit() {
+ return false;
+ }
+
+ @Override
+ public boolean sendKeyEvent(KeyEvent event) {
+ return false;
+ }
+
+ @Override
+ public boolean clearMetaKeyStates(int states) {
+ return false;
+ }
+
+ @Override
+ public boolean reportFullscreenMode(boolean enabled) {
+ return false;
+ }
+
+ @Override
+ public boolean performPrivateCommand(String action, Bundle data) {
+ return false;
+ }
+
+ @Override
+ public boolean requestCursorUpdates(int cursorUpdateMode) {
+ return false;
+ }
+
+ @Override
+ public Handler getHandler() {
+ return null;
+ }
+
+ @Override
+ public void closeConnection() {
+
+ }
+
+ @Override
+ public boolean commitContent(InputContentInfo inputContentInfo, int flags, Bundle opts) {
+ return false;
+ }
+ }
+
+ private static final class InputConnectionHandlingThread extends HandlerThread
+ implements AutoCloseable {
+
+ private final Handler mHandler;
+
+ InputConnectionHandlingThread() {
+ super("IC-callback");
+ start();
+ mHandler = Handler.createAsync(getLooper());
+ }
+
+ @NonNull
+ Handler getHandler() {
+ return mHandler;
+ }
+
+ @Override
+ public void close() {
+ quitSafely();
+ try {
+ join(TIMEOUT);
+ } catch (InterruptedException e) {
+ fail("Failed to stop the thread: " + e);
+ }
+ }
+ }
}
