Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit aaa38c9f authored by Yohei Yukawa's avatar Yohei Yukawa
Browse files

Ensure IC#finishComposingText() is called on the correct Handler.

This attempts to reland previously reverted CLs [1][2] due to an
unexpected regression (Bug 27824691).

The Bug 27868748 we want to address by this CL is that currently
InputConnection#finishComposingText() can be called on the root view's
Handler no matter what Handler is associated with
ControlledInputConnectionWrapper.  Actually the root cause of
Bug 6789252 is the same, but there we worked around it by not calling
InputConnection#finishComposingText() in certain situations [3].
With this CL we should be able to logically revert that workaround.

This CL also removes redundant IMM#mServedInputConnection.  This is safe
because the following two fields have the same lifetime.
 - InputMethodManager#mServedInputConnection
 - InputMethodManager#mServedInputConnectionWrapper
We do not need to maintain both of them.  This also allows us to use a
strong refecente in IInputConnectionWrapper#mInputConnection instead of
a WeakReference.  To understand why this is safe, we need to understand
how things previously worked, which is as follows:

  1. InputMethodManager#mServedInputConnection becomes non-null.
    -> IInputConnectionWrapper#mInputConnection.get() is guaranteed to
       be alive.
  2. InputMethodManager#mServedInputConnection becomes null or another
     object.
    -> IInputConnectionWrapper#mInputConnection.get() may not be alive.

Since we know exactly when InputMethodManager#mServedInputConnection is
updated, in theory we do not need to use WeakReference here, and
with this CL we do not use WeakReference anymore.  Actually the initial
commit [1] accidentally removed the last strong reference to the active
InputConnection and WeakReference could be null at any time, which was
what we observed in Bug 27824691.

  [1]: I1181e067aa5bedbdf0c7ec1bcec479257aea511c
       afb6558c
  [2]: Ibe94f115e607a198d12ecd3d4e4f91a7d9469c98
       16e2c7b5
  [3]: I66f51da1299532793ef8fa700f35b0811670f235
       4e5184f9

Bug: 27868748
Change-Id: If2a03bc84d318775fd4a197fa43acde086eda442
parent 40e1135e
Loading
Loading
Loading
Loading
+0 −14
Original line number Diff line number Diff line
@@ -3300,7 +3300,6 @@ public final class ViewRootImpl implements ViewParent,
    private final static int MSG_DISPATCH_APP_VISIBILITY = 8;
    private final static int MSG_DISPATCH_GET_NEW_SURFACE = 9;
    private final static int MSG_DISPATCH_KEY_FROM_IME = 11;
    private final static int MSG_FINISH_INPUT_CONNECTION = 12;
    private final static int MSG_CHECK_FOCUS = 13;
    private final static int MSG_CLOSE_SYSTEM_DIALOGS = 14;
    private final static int MSG_DISPATCH_DRAG_EVENT = 15;
@@ -3340,8 +3339,6 @@ public final class ViewRootImpl implements ViewParent,
                    return "MSG_DISPATCH_GET_NEW_SURFACE";
                case MSG_DISPATCH_KEY_FROM_IME:
                    return "MSG_DISPATCH_KEY_FROM_IME";
                case MSG_FINISH_INPUT_CONNECTION:
                    return "MSG_FINISH_INPUT_CONNECTION";
                case MSG_CHECK_FOCUS:
                    return "MSG_CHECK_FOCUS";
                case MSG_CLOSE_SYSTEM_DIALOGS:
@@ -3562,12 +3559,6 @@ public final class ViewRootImpl implements ViewParent,
                }
                enqueueInputEvent(event, null, QueuedInputEvent.FLAG_DELIVER_POST_IME, true);
            } break;
            case MSG_FINISH_INPUT_CONNECTION: {
                InputMethodManager imm = InputMethodManager.peekInstance();
                if (imm != null) {
                    imm.reportFinishInputConnection((InputConnection)msg.obj);
                }
            } break;
            case MSG_CHECK_FOCUS: {
                InputMethodManager imm = InputMethodManager.peekInstance();
                if (imm != null) {
@@ -5878,11 +5869,6 @@ public final class ViewRootImpl implements ViewParent,
        }
    }

    public void dispatchFinishInputConnection(InputConnection connection) {
        Message msg = mHandler.obtainMessage(MSG_FINISH_INPUT_CONNECTION, connection);
        mHandler.sendMessage(msg);
    }

    public void dispatchResized(Rect frame, Rect overscanInsets, Rect contentInsets,
            Rect visibleInsets, Rect stableInsets, Rect outsets, boolean reportDraw,
            Configuration newConfig, Rect backDropFrame, boolean forceLayout,
+2 −2
Original line number Diff line number Diff line
@@ -158,8 +158,8 @@ public class BaseInputConnection implements InputConnection {
     *
     * @hide
     */
    protected void reportFinish() {
        // Intentionaly empty
    public void reportFinish() {
        // Intentionally empty
    }

    /**
+20 −50
Original line number Diff line number Diff line
@@ -317,7 +317,6 @@ public final class InputMethodManager {
    /**
     * The InputConnection that was last retrieved from the served view.
     */
    InputConnection mServedInputConnection;
    ControlledInputConnectionWrapper mServedInputConnectionWrapper;
    /**
     * The completions that were last provided by the served view.
@@ -498,7 +497,7 @@ public final class InputMethodManager {
                            // from a thread that created mServedView. That could happen
                            // the current activity is running in the system process.
                            // In that case, we really should not call
                            // mServedInputConnection.finishComposingText.
                            // mServedInputConnectionWrapper.finishComposingText().
                            if (checkFocusNoStartInput(mHasBeenInactive, false)) {
                                final int reason = active ?
                                        InputMethodClient.START_INPUT_REASON_ACTIVATED_BY_IMMS :
@@ -532,22 +531,25 @@ public final class InputMethodManager {

    private static class ControlledInputConnectionWrapper extends IInputConnectionWrapper {
        private final InputMethodManager mParentInputMethodManager;
        private boolean mActive;

        public ControlledInputConnectionWrapper(final Looper mainLooper, final InputConnection conn,
                final InputMethodManager inputMethodManager) {
            super(mainLooper, conn);
            mParentInputMethodManager = inputMethodManager;
            mActive = true;
        }

        @Override
        public boolean isActive() {
            return mParentInputMethodManager.mActive && mActive;
            return mParentInputMethodManager.mActive && !isFinished();
        }

        void deactivate() {
            mActive = false;
            if (isFinished()) {
                // This is a small performance optimization.  Still only the 1st call of
                // reportFinish() will take effect.
                return;
            }
            reportFinish();
        }

        @Override
@@ -562,7 +564,9 @@ public final class InputMethodManager {

        @Override
        public String toString() {
            return "ControlledInputConnectionWrapper{mActive=" + mActive
            return "ControlledInputConnectionWrapper{"
                    + "connection=" + getInputConnection()
                    + " finished=" + isFinished()
                    + " mParentInputMethodManager.mActive=" + mParentInputMethodManager.mActive
                    + "}";
        }
@@ -780,7 +784,8 @@ public final class InputMethodManager {
     */
    public boolean isAcceptingText() {
        checkFocus();
        return mServedInputConnection != null;
        return mServedInputConnectionWrapper != null &&
                mServedInputConnectionWrapper.getInputConnection() != null;
    }

    /**
@@ -815,7 +820,6 @@ public final class InputMethodManager {
     */
    void clearConnectionLocked() {
        mCurrentTextBoxAttribute = null;
        mServedInputConnection = null;
        if (mServedInputConnectionWrapper != null) {
            mServedInputConnectionWrapper.deactivate();
            mServedInputConnectionWrapper = null;
@@ -836,7 +840,6 @@ public final class InputMethodManager {
                    throw e.rethrowFromSystemServer();
                }
            }
            notifyInputConnectionFinished();
            mServedView = null;
            mCompletions = null;
            mServedConnecting = false;
@@ -844,37 +847,6 @@ public final class InputMethodManager {
        }
    }

    /**
     * Notifies the served view that the current InputConnection will no longer be used.
     */
    private void notifyInputConnectionFinished() {
        if (mServedView != null && mServedInputConnection != null) {
            // We need to tell the previously served view that it is no
            // longer the input target, so it can reset its state.  Schedule
            // this call on its window's Handler so it will be on the correct
            // thread and outside of our lock.
            ViewRootImpl viewRootImpl = mServedView.getViewRootImpl();
            if (viewRootImpl != null) {
                // This will result in a call to reportFinishInputConnection() below.
                viewRootImpl.dispatchFinishInputConnection(mServedInputConnection);
            }
        }
    }

    /**
     * Called from the FINISH_INPUT_CONNECTION message above.
     * @hide
     */
    public void reportFinishInputConnection(InputConnection ic) {
        if (mServedInputConnection != ic) {
            ic.finishComposingText();
            // To avoid modifying the public InputConnection interface
            if (ic instanceof BaseInputConnection) {
                ((BaseInputConnection) ic).reportFinish();
            }
        }
    }

    public void displayCompletions(View view, CompletionInfo[] completions) {
        checkFocus();
        synchronized (mH) {
@@ -1240,9 +1212,10 @@ public final class InputMethodManager {
            // Hook 'em up and let 'er rip.
            mCurrentTextBoxAttribute = tba;
            mServedConnecting = false;
            // Notify the served view that its previous input connection is finished
            notifyInputConnectionFinished();
            mServedInputConnection = ic;
            if (mServedInputConnectionWrapper != null) {
                mServedInputConnectionWrapper.deactivate();
                mServedInputConnectionWrapper = null;
            }
            ControlledInputConnectionWrapper servedContext;
            final int missingMethodFlags;
            if (ic != null) {
@@ -1267,9 +1240,6 @@ public final class InputMethodManager {
                servedContext = null;
                missingMethodFlags = 0;
            }
            if (mServedInputConnectionWrapper != null) {
                mServedInputConnectionWrapper.deactivate();
            }
            mServedInputConnectionWrapper = servedContext;

            try {
@@ -1413,7 +1383,7 @@ public final class InputMethodManager {
            return false;
        }

        InputConnection ic = null;
        final ControlledInputConnectionWrapper ic;
        synchronized (mH) {
            if (mServedView == mNextServedView && !forceNewFocus) {
                return false;
@@ -1433,7 +1403,7 @@ public final class InputMethodManager {
                return false;
            }

            ic = mServedInputConnection;
            ic = mServedInputConnectionWrapper;

            mServedView = mNextServedView;
            mCurrentTextBoxAttribute = null;
@@ -2282,7 +2252,7 @@ public final class InputMethodManager {
        } else {
            p.println("  mCurrentTextBoxAttribute: null");
        }
        p.println("  mServedInputConnection=" + mServedInputConnection);
        p.println("  mServedInputConnectionWrapper=" + mServedInputConnectionWrapper);
        p.println("  mCompletions=" + Arrays.toString(mCompletions));
        p.println("  mCursorRect=" + mCursorRect);
        p.println("  mCursorSelStart=" + mCursorSelStart
+86 −27
Original line number Diff line number Diff line
@@ -16,6 +16,10 @@

package com.android.internal.view;

import com.android.internal.annotations.GuardedBy;

import android.annotation.NonNull;
import android.annotation.Nullable;
import android.os.Bundle;
import android.os.Handler;
import android.os.Looper;
@@ -23,6 +27,7 @@ import android.os.Message;
import android.os.RemoteException;
import android.util.Log;
import android.view.KeyEvent;
import android.view.inputmethod.BaseInputConnection;
import android.view.inputmethod.CompletionInfo;
import android.view.inputmethod.CorrectionInfo;
import android.view.inputmethod.ExtractedTextRequest;
@@ -56,11 +61,17 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
    private static final int DO_PERFORM_PRIVATE_COMMAND = 120;
    private static final int DO_CLEAR_META_KEY_STATES = 130;
    private static final int DO_REQUEST_UPDATE_CURSOR_ANCHOR_INFO = 140;
    private static final int DO_REPORT_FINISH = 150;

    private WeakReference<InputConnection> mInputConnection;
    @GuardedBy("mLock")
    @Nullable
    private InputConnection mInputConnection;

    private Looper mMainLooper;
    private Handler mH;
    private Object mLock = new Object();
    @GuardedBy("mLock")
    private boolean mFinished = false;

    static class SomeArgs {
        Object arg1;
@@ -80,12 +91,25 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
        }
    }
    
    public IInputConnectionWrapper(Looper mainLooper, InputConnection conn) {
        mInputConnection = new WeakReference<>(conn);
    public IInputConnectionWrapper(Looper mainLooper, @NonNull InputConnection inputConnection) {
        mInputConnection = inputConnection;
        mMainLooper = mainLooper;
        mH = new MyHandler(mMainLooper);
    }

    @Nullable
    public InputConnection getInputConnection() {
        synchronized (mLock) {
            return mInputConnection;
        }
    }

    protected boolean isFinished() {
        synchronized (mLock) {
            return mFinished;
        }
    }

    abstract protected boolean isActive();

    /**
@@ -198,6 +222,10 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                seq, callback));
    }

    public void reportFinish() {
        dispatchMessage(obtainMessage(DO_REPORT_FINISH));
    }

    void dispatchMessage(Message msg) {
        // If we are calling this from the main thread, then we can call
        // right through.  Otherwise, we need to send the message to the
@@ -216,7 +244,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
            case DO_GET_TEXT_AFTER_CURSOR: {
                SomeArgs args = (SomeArgs)msg.obj;
                try {
                    InputConnection ic = mInputConnection.get();
                    InputConnection ic = getInputConnection();
                    if (ic == null || !isActive()) {
                        Log.w(TAG, "getTextAfterCursor on inactive InputConnection");
                        args.callback.setTextAfterCursor(null, args.seq);
@@ -232,7 +260,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
            case DO_GET_TEXT_BEFORE_CURSOR: {
                SomeArgs args = (SomeArgs)msg.obj;
                try {
                    InputConnection ic = mInputConnection.get();
                    InputConnection ic = getInputConnection();
                    if (ic == null || !isActive()) {
                        Log.w(TAG, "getTextBeforeCursor on inactive InputConnection");
                        args.callback.setTextBeforeCursor(null, args.seq);
@@ -248,7 +276,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
            case DO_GET_SELECTED_TEXT: {
                SomeArgs args = (SomeArgs)msg.obj;
                try {
                    InputConnection ic = mInputConnection.get();
                    InputConnection ic = getInputConnection();
                    if (ic == null || !isActive()) {
                        Log.w(TAG, "getSelectedText on inactive InputConnection");
                        args.callback.setSelectedText(null, args.seq);
@@ -264,7 +292,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
            case DO_GET_CURSOR_CAPS_MODE: {
                SomeArgs args = (SomeArgs)msg.obj;
                try {
                    InputConnection ic = mInputConnection.get();
                    InputConnection ic = getInputConnection();
                    if (ic == null || !isActive()) {
                        Log.w(TAG, "getCursorCapsMode on inactive InputConnection");
                        args.callback.setCursorCapsMode(0, args.seq);
@@ -280,7 +308,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
            case DO_GET_EXTRACTED_TEXT: {
                SomeArgs args = (SomeArgs)msg.obj;
                try {
                    InputConnection ic = mInputConnection.get();
                    InputConnection ic = getInputConnection();
                    if (ic == null || !isActive()) {
                        Log.w(TAG, "getExtractedText on inactive InputConnection");
                        args.callback.setExtractedText(null, args.seq);
@@ -294,7 +322,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_COMMIT_TEXT: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "commitText on inactive InputConnection");
                    return;
@@ -304,7 +332,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_SET_SELECTION: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "setSelection on inactive InputConnection");
                    return;
@@ -313,7 +341,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_PERFORM_EDITOR_ACTION: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "performEditorAction on inactive InputConnection");
                    return;
@@ -322,7 +350,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_PERFORM_CONTEXT_MENU_ACTION: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "performContextMenuAction on inactive InputConnection");
                    return;
@@ -331,7 +359,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_COMMIT_COMPLETION: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "commitCompletion on inactive InputConnection");
                    return;
@@ -340,7 +368,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_COMMIT_CORRECTION: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "commitCorrection on inactive InputConnection");
                    return;
@@ -349,7 +377,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_SET_COMPOSING_TEXT: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "setComposingText on inactive InputConnection");
                    return;
@@ -359,7 +387,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_SET_COMPOSING_REGION: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "setComposingRegion on inactive InputConnection");
                    return;
@@ -368,7 +396,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_FINISH_COMPOSING_TEXT: {
                InputConnection ic = mInputConnection.get();
                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
@@ -381,7 +409,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_SEND_KEY_EVENT: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "sendKeyEvent on inactive InputConnection");
                    return;
@@ -391,7 +419,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_CLEAR_META_KEY_STATES: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "clearMetaKeyStates on inactive InputConnection");
                    return;
@@ -400,7 +428,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_DELETE_SURROUNDING_TEXT: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "deleteSurroundingText on inactive InputConnection");
                    return;
@@ -409,7 +437,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_DELETE_SURROUNDING_TEXT_IN_CODE_POINTS: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "deleteSurroundingTextInCodePoints on inactive InputConnection");
                    return;
@@ -418,7 +446,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_BEGIN_BATCH_EDIT: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "beginBatchEdit on inactive InputConnection");
                    return;
@@ -427,7 +455,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_END_BATCH_EDIT: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "endBatchEdit on inactive InputConnection");
                    return;
@@ -436,7 +464,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_REPORT_FULLSCREEN_MODE: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null) {
                    Log.w(TAG, "reportFullscreenMode on inexistent InputConnection");
                    return;
@@ -447,7 +475,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                return;
            }
            case DO_PERFORM_PRIVATE_COMMAND: {
                InputConnection ic = mInputConnection.get();
                InputConnection ic = getInputConnection();
                if (ic == null || !isActive()) {
                    Log.w(TAG, "performPrivateCommand on inactive InputConnection");
                    return;
@@ -460,7 +488,7 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
            case DO_REQUEST_UPDATE_CURSOR_ANCHOR_INFO: {
                SomeArgs args = (SomeArgs)msg.obj;
                try {
                    InputConnection ic = mInputConnection.get();
                    InputConnection ic = getInputConnection();
                    if (ic == null || !isActive()) {
                        Log.w(TAG, "requestCursorAnchorInfo on inactive InputConnection");
                        args.callback.setRequestUpdateCursorAnchorInfoResult(false, args.seq);
@@ -473,6 +501,37 @@ public abstract class IInputConnectionWrapper extends IInputContext.Stub {
                }
                return;
            }
            case DO_REPORT_FINISH: {
                // 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 DO_REPORT_FINISH messages will not be handled at the
                // same time.
                if (isFinished()) {
                    return;
                }
                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;
                    }
                    ic.finishComposingText();
                    // TODO: Make reportFinish() public method of InputConnection to remove this
                    // check.
                    if (ic instanceof BaseInputConnection) {
                        ((BaseInputConnection) ic).reportFinish();
                    }
                } finally {
                    synchronized (mLock) {
                        mInputConnection = null;
                        mFinished = true;
                    }
                }
                return;
            }
        }
        Log.w(TAG, "Unhandled message code: " + msg.what);
    }
+4 −1
Original line number Diff line number Diff line
@@ -83,8 +83,11 @@ public class EditableInputConnection extends BaseInputConnection {
        return false;
    }

    /**
     * @hide
     */
    @Override
    protected void reportFinish() {
    public void reportFinish() {
        super.reportFinish();

        synchronized(this) {