From c478c171e92b2f255e9699d9c9306b001368ac20 Mon Sep 17 00:00:00 2001 From: Gilles Debunne Date: Mon, 19 Dec 2011 17:29:24 -0800 Subject: 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 --- .../view/inputmethod/InputMethodManager.java | 44 ++++++++++-------- .../internal/widget/EditableInputConnection.java | 54 ++++++++++++++++++---- 2 files changed, 72 insertions(+), 26 deletions(-) diff --git a/core/java/android/view/inputmethod/InputMethodManager.java b/core/java/android/view/inputmethod/InputMethodManager.java index b41e6f5e7907..0985e1465b0b 100644 --- a/core/java/android/view/inputmethod/InputMethodManager.java +++ b/core/java/android/view/inputmethod/InputMethodManager.java @@ -651,19 +651,7 @@ public final class InputMethodManager { } } - if (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. - vh.sendMessage(vh.obtainMessage(ViewRootImpl.FINISH_INPUT_CONNECTION, - mServedInputConnection)); - } - } + notifyInputConnectionFinished(); mServedView = null; mCompletions = null; @@ -671,7 +659,25 @@ public final class InputMethodManager { 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. + vh.sendMessage(vh.obtainMessage(ViewRootImpl.FINISH_INPUT_CONNECTION, + mServedInputConnection)); + } + } + } + /** * Called from the FINISH_INPUT_CONNECTION message above. * @hide @@ -681,7 +687,7 @@ public final class InputMethodManager { ic.finishComposingText(); } } - + public void displayCompletions(View view, CompletionInfo[] completions) { checkFocus(); synchronized (mH) { @@ -831,7 +837,7 @@ public final class InputMethodManager { * shown with {@link #SHOW_FORCED}. */ public static final int HIDE_NOT_ALWAYS = 0x0002; - + /** * Synonym for {@link #hideSoftInputFromWindow(IBinder, int, ResultReceiver)} * without a result: request to hide the soft input window from the @@ -993,7 +999,7 @@ public final class InputMethodManager { tba.fieldId = view.getId(); InputConnection ic = view.onCreateInputConnection(tba); if (DEBUG) Log.v(TAG, "Starting input: tba=" + tba + " ic=" + ic); - + synchronized (mH) { // Now that we are locked again, validate that our state hasn't // changed. @@ -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. diff --git a/core/java/com/android/internal/widget/EditableInputConnection.java b/core/java/com/android/internal/widget/EditableInputConnection.java index 32e733baa98a..9579bce468da 100644 --- a/core/java/com/android/internal/widget/EditableInputConnection.java +++ b/core/java/com/android/internal/widget/EditableInputConnection.java @@ -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; @@ -48,19 +53,35 @@ public class EditableInputConnection extends BaseInputConnection { } return null; } - + @Override public boolean beginBatchEdit() { - mTextView.beginBatchEdit(); - return true; + synchronized(this) { + if (mBatchEditNesting >= 0) { + mTextView.beginBatchEdit(); + mBatchEditNesting++; + return true; + } + } + return false; } - + @Override public boolean endBatchEdit() { - mTextView.endBatchEdit(); - return true; + 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) { final Editable content = getEditable(); @@ -76,7 +97,24 @@ 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); -- cgit v1.2.3-59-g8ed1b