From d55ddf27c6cc7771d711d4b90f8bf85e5cba8615 Mon Sep 17 00:00:00 2001 From: Yohei Yukawa Date: Thu, 5 Aug 2021 17:02:02 -0700 Subject: [PATCH] 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 --- .../RemoteInputConnectionImpl.java | 94 +++--- .../android/view/ViewInputConnectionTest.java | 301 ++++++++++++++++++ 2 files changed, 346 insertions(+), 49 deletions(-) diff --git a/core/java/com/android/internal/inputmethod/RemoteInputConnectionImpl.java b/core/java/com/android/internal/inputmethod/RemoteInputConnectionImpl.java index 2d44054e03723..0c2701219ef45 100644 --- a/core/java/com/android/internal/inputmethod/RemoteInputConnectionImpl.java +++ b/core/java/com/android/internal/inputmethod/RemoteInputConnectionImpl.java @@ -121,25 +121,54 @@ public final class RemoteInputConnectionImpl extends IInputContext.Stub { // 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 @@ public final class RemoteInputConnectionImpl extends IInputContext.Stub { }); } - 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 d667af39f5857..d60c0c688e1a4 100644 --- a/core/tests/coretests/src/android/view/ViewInputConnectionTest.java +++ b/core/tests/coretests/src/android/view/ViewInputConnectionTest.java @@ -19,13 +19,25 @@ package android.view; 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.Rule; 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 mActivityRule = new ActivityTestRule<>(ViewInputConnectionTestActivity.class); @@ -289,4 +312,282 @@ public class ViewInputConnectionTest { 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 getOnMainSync(@NonNull Supplier supplier) throws Throwable { + final AtomicReference 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 mInputConnectionClosedCallingThreadId = + new AtomicReference<>(); + + public CountDownLatch mOnInputConnectionClosedCalled = new CountDownLatch(1); + public AtomicReference 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); + } + } + } }