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

Commit c478c171 authored by Gilles Debunne's avatar Gilles Debunne
Browse files

Unbalanced batch edit begin and end leave TextView unresponsive 

This is a fix for http://code.google.com/p/android/issues/detail?id=17508

Adding some logs and a forced GC, I'm now reliably able to reproduce it. Here is the scenario.

1. The IME handles an event. It retrieves the current InputConnection (IC) using
   ic = getCurrentInputConnection() and calls ic.beginBatchEdit();
2. The call is propagated to the UI thread and TextView's mBatchEditNesting
   is correctly increased through beginBatchEdit()
3. A listener calls setText(), which imm.restartInput(this);
4. As a result, the InputMethodManager creates a new ControlledInputConnectionWrapper
   with a new InputConnection from the TextView
5. A GC happens at that point. The previous InputConnection is no longeri
   referenced by the InputMethodManager's mServedInputConnection.
   The weak reference in the previous ControlledInputConnectionWrapper is nulled.
6. The IME thread finishes its process and calls ic.endBatchEdit(); on its version
   of the original InputConnection.
7. The message is passed through the InputConnect, but when the weak reference in the
   original IInputConnectionWrapper is dereferenced, we get a null InputConnection in
   executeMessage().
8. As a result, the TextView's endBatchEdit() method is not called, leaving this TextView
   with a non zero mBatchEditNesting.
9. From now on, all edit actions on this TextView will be considered part of a nested edition
   and no invalidation is performed, which is the visible manifestation of this bug.

The core problem is that the begin/end batch edit contract is broken when:
1. These are initiated by the IME thread (as opposed to the UI thread)
2. The input connection is reset between these calls
3. A GC happens in the mean time and the WeakReference is lost (otherwise
   calling endBatchEdit on a no longer active InputConnection is fine

Solution to keep TextView's mBatchEditNesting balanced:

- The IMM should notify the IC when it is no longer used. We're using the
existing FINISH_INPUT_CONNECTION to do that.
- The InputConnection should keep track of its nesting contribution to the TextView.
When finished the IC makes sure its contribution is reset to 0.
Moreover, further asynchonous calls to begin/endBatchEdit that may arrive from the IME
should be ignored. This is achieved using a negative value as a flag.

Notes:

- finishComposingText may be too broad of a method to perform such a cleaning step
but is seems to only be called in cases where the IC will not be used anymore.
If that's too broad, we have to introduce a new method in the IC interface.

- This is has been implemented in EditableInputConnection and not in a more general
BaseInputConnection because this is where we have a notion of TextEdit, and the
nesting problem is here specific to TextView.
However, the same unbalanced begin/end problem will happen in these classes. They
should override finishComposingText as has been done here if that matters.

- We cannot re-use the TextView's mBatchEditNesting since it may take into account
batch edit from various sources and resetting it on InputConnection close could
then lead to an inconsistent negative count value.

Patch Set 2: added synchronized blocks around mBatchEditNesting

Change-Id: I1ec5518fdc16fb0551fbce9d13f5d92eb4bc78c0
parent 01cc1d1e

core/java/android/view/inputmethod/InputMethodManager.java

+26 −18
Original line number Diff line number Diff line
@@ -651,25 +651,31 @@ public final class InputMethodManager { 
                }

            }

            

            if (mServedInputConnection != null) {

            notifyInputConnectionFinished();

            

            mServedView = null;

            mCompletions = null;

            mServedConnecting = false;

            clearConnectionLocked();

        }

    }



    /**

     * 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.

            Handler vh = mServedView.getHandler();

            if (vh != null) {

                    // This will result in a call to reportFinishInputConnection()

                    // below.

                // This will result in a call to reportFinishInputConnection() below.

                vh.sendMessage(vh.obtainMessage(ViewRootImpl.FINISH_INPUT_CONNECTION,

                        mServedInputConnection));

            }

        }

            

            mServedView = null;

            mCompletions = null;

            mServedConnecting = false;

            clearConnectionLocked();

        }

    }



    /**
@@ -1012,6 +1018,8 @@ 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;

            IInputContext servedContext;

            if (ic != null) {
@@ -1115,7 +1123,7 @@ public final class InputMethodManager { 
        }

    }



    void scheduleCheckFocusLocked(View view) {

    static void scheduleCheckFocusLocked(View view) {

        Handler vh = view.getHandler();

        if (vh != null && !vh.hasMessages(ViewRootImpl.CHECK_FOCUS)) {

            // This will result in a call to checkFocus() below.

core/java/com/android/internal/widget/EditableInputConnection.java

+46 −8
Original line number Diff line number Diff line
@@ -35,6 +35,11 @@ public class EditableInputConnection extends BaseInputConnection { 


    private final TextView mTextView;



    // Keeps track of nested begin/end batch edit to ensure this connection always has a

    // balanced impact on its associated TextView.

    // A negative value means that this connection has been finished by the InputMethodManager.

    private int mBatchEditNesting;



    public EditableInputConnection(TextView textview) {

        super(textview, true);

        mTextView = textview;
@@ -51,15 +56,31 @@ public class EditableInputConnection extends BaseInputConnection { 


    @Override

    public boolean beginBatchEdit() {

        synchronized(this) {

            if (mBatchEditNesting >= 0) {

                mTextView.beginBatchEdit();

                mBatchEditNesting++;

                return true;

            }

        }

        return false;

    }



    @Override

    public boolean endBatchEdit() {

        synchronized(this) {

            if (mBatchEditNesting > 0) {

                // When the connection is reset by the InputMethodManager and finishComposingText

                // is called, some endBatchEdit calls may still be asynchronously received from the

                // IME. Do not take these into account, thus ensuring that this IC's final

                // contribution to mTextView's nested batch edit count is zero.

                mTextView.endBatchEdit();

                mBatchEditNesting--;

                return true;

            }

        }

        return false;

    }



    @Override

    public boolean clearMetaKeyStates(int states) {
@@ -77,6 +98,23 @@ public class EditableInputConnection extends BaseInputConnection { 
        return true;

    }



    @Override

    public boolean finishComposingText() {

        final boolean superResult = super.finishComposingText();

        synchronized(this) {

            if (mBatchEditNesting < 0) {

                // The connection was already finished

                return false;

            }

            while (mBatchEditNesting > 0) {

                endBatchEdit();

            }

            // Will prevent any further calls to begin or endBatchEdit

            mBatchEditNesting = -1;

        }

        return superResult;

    }



    @Override

    public boolean commitCompletion(CompletionInfo text) {

        if (DEBUG) Log.v(TAG, "commitCompletion " + text);