Fix leaks in InputConnectionHandlerThread

content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.content.browser.input;
 
 import android.os.Handler;
 import android.os.HandlerThread;
 import android.view.View;
 import android.view.inputmethod.EditorInfo;
 
 import org.chromium.base.Log;
+import org.chromium.base.ThreadUtils;
 import org.chromium.base.VisibleForTesting;
 
 /**
  * A factory class for {@link ThreadedInputConnection}. The class also includes triggering
  * mechanism (hack) to run our InputConnection on non-UI thread.
  */
 // TODO(changwan): add unit tests once Robolectric supports Android API level >= 21.
 // See crbug.com/588547 for details.
 public class ThreadedInputConnectionFactory implements ChromiumBaseInputConnection.Factory {
     private static final String TAG = "cr_Ime";
     private static final boolean DEBUG_LOGS = false;
 
     // Most of the time we do not need to retry. But if we have lost window focus while triggering
     // delayed creation, then there is a chance that detection may fail in the following scenario:
     // InputMethodManagerService checks the window focus by directly calling
     // WindowManagerService#inputMethodClientHasFocus(). But the window focus change is
     // propagated to the view via ViewRootImpl's message queue. Therefore, it may take another
     // UI message loop until View#hasWindowFocus() is aligned with what IMMS sees.
     private static final int CHECK_REGISTER_RETRY = 1;
 
-    private final Handler mHandler;
+    private Handler mHandler;
     private final InputMethodManagerWrapper mInputMethodManagerWrapper;
     private final InputMethodUma mInputMethodUma;
     private ThreadedInputConnectionProxyView mProxyView;
     private ThreadedInputConnection mThreadedInputConnection;
     private CheckInvalidator mCheckInvalidator;
     private boolean mReentrantTriggering;
 
     // A small class that can be updated to invalidate the check when there is an external event
     // such as window focus loss or view focus loss.
     private static class CheckInvalidator {
         private boolean mInvalid;
 
         public void invalidate() {
             ImeUtils.checkOnUiThread();
             mInvalid = true;
         }
 
         public boolean isInvalid() {
             ImeUtils.checkOnUiThread();
             return mInvalid;
         }
     }
 
     ThreadedInputConnectionFactory(InputMethodManagerWrapper inputMethodManagerWrapper) {
         mInputMethodManagerWrapper = inputMethodManagerWrapper;
-        mHandler = createHandler();
         mInputMethodUma = createInputMethodUma();
     }
 
     @VisibleForTesting
     protected Handler createHandler() {
         HandlerThread thread =
                 new HandlerThread("InputConnectionHandlerThread", HandlerThread.NORM_PRIORITY);
         thread.start();
         return new Handler(thread.getLooper());
     }
 
+    private void destroyHandler() {
+        if (mHandler == null) return;
+        final Handler handler = mHandler;

[Torne, 2016/11/29 13:22:55]

Why is this posted to the handler just to post back to the UI thread?

[Changwan Ryu, 2016/11/29 23:33:18]

In most cases, InputMethodManager should access our Handler in the following
way:
1) Compare Looper against main looper.
2) Call sendMessage to it.

Once Looper.quit() is called, sendMessage should return false. It seems mostly
fine,
but I've added additional UI loop in case I missed something...

[Torne, 2016/11/30 10:33:35]

I don't understand what you mean here at all; maybe I wasn't clear, but what I
meant was why doesn't this just simply call mHandlerThread.quit() directly from
the current thread, and avoid any posting at all?

+        handler.post(new Runnable() {
+            @Override
+            public void run() {
+                ThreadUtils.postOnUiThread(new Runnable() {
+                    @Override
+                    public void run() {
+                        if (handler != null) {
+                            handler.getLooper().quit();
+                        }
+                    }
+                });
+            }
+        });
+        mHandler = null;
+    }
+
     @VisibleForTesting
     protected ThreadedInputConnectionProxyView createProxyView(
             Handler handler, View containerView) {
         return new ThreadedInputConnectionProxyView(
                 containerView.getContext(), handler, containerView);
     }
 
     @VisibleForTesting
     protected InputMethodUma createInputMethodUma() {
         return new InputMethodUma();
@@ skipping 29 matching lines @@
         ImeUtils.computeEditorInfo(
                 inputType, inputFlags, selectionStart, selectionEnd, outAttrs);
         if (DEBUG_LOGS) {
             Log.w(TAG, "initializeAndGet. outAttr: " + ImeUtils.getEditorInfoDebugString(outAttrs));
         }
 
         // IMM can internally ignore subsequent activation requests, e.g., by checking
         // mServedConnecting.
         if (mCheckInvalidator != null) mCheckInvalidator.invalidate();
 
+        if (mHandler == null) mHandler = createHandler();
+
         if (shouldTriggerDelayedOnCreateInputConnection()) {
             triggerDelayedOnCreateInputConnection(view);
             return null;
         }
         if (DEBUG_LOGS) Log.w(TAG, "initializeAndGet: called from proxy view");
 
         if (mThreadedInputConnection == null) {
             if (DEBUG_LOGS) Log.w(TAG, "Creating ThreadedInputConnection...");
             mThreadedInputConnection = new ThreadedInputConnection(view, imeAdapter, mHandler);
         } else {
             mThreadedInputConnection.resetOnUiThread();
         }
         return mThreadedInputConnection;
     }
 
     private void triggerDelayedOnCreateInputConnection(final View view) {
         if (DEBUG_LOGS) Log.w(TAG, "triggerDelayedOnCreateInputConnection");
         // Prevent infinite loop when View methods trigger onCreateInputConnection
         // on some OEM phones. (crbug.com/636197)
         if (mReentrantTriggering) return;
 
         // We need to check this before creating invalidator.
         if (!view.hasFocus()) return;
 
         mCheckInvalidator = new CheckInvalidator();
 
         if (!view.hasWindowFocus()) mCheckInvalidator.invalidate();
 
+        if (mHandler == null) return;
         // We cannot reuse the existing proxy view, if any, due to crbug.com/664402.
         mProxyView = createProxyView(mHandler, view);
 
         mReentrantTriggering = true;
         // This does not affect view focus of the real views.
         mProxyView.requestFocus();
         mReentrantTriggering = false;
 
         view.getHandler().post(new Runnable() {
             @Override
             public void run() {
                 // This is a hack to make InputMethodManager believe that the proxy view
                 // now has a focus. As a result, InputMethodManager will think that mProxyView
                 // is focused, and will call getHandler() of the view when creating input
                 // connection.
 
                 // Step 1: Set mProxyView as InputMethodManager#mNextServedView.
                 // This does not affect the real window focus.
                 mProxyView.onWindowFocusChanged(true);
 
                 // Step 2: Have InputMethodManager focus in on mNextServedView.
                 // As a result, IMM will call onCreateInputConnection() on mProxyView on the same
                 // thread as mProxyView.getHandler(). It will also call subsequent InputConnection
                 // methods on this IME thread.
                 mInputMethodManagerWrapper.isActive(view);
 
                 // Step 3: Check that the above hack worked.
                 // Do not check until activation finishes inside InputMethodManager (on IME thread).
+                if (mHandler == null) return;
                 mHandler.post(new Runnable() {
                     @Override
                     public void run() {
                         postCheckRegisterResultOnUiThread(view, mCheckInvalidator,
                                 CHECK_REGISTER_RETRY);
                     }
                 });
             }
         });
     }
@@ skipping 58 matching lines @@
     public void onViewFocusChanged(boolean gainFocus) {
         if (DEBUG_LOGS) Log.d(TAG, "onViewFocusChanged: " + gainFocus);
         if (!gainFocus && mCheckInvalidator != null) mCheckInvalidator.invalidate();
         if (mProxyView != null) mProxyView.onOriginalViewFocusChanged(gainFocus);
     }
 
     @Override
     public void onViewAttachedToWindow() {
         if (DEBUG_LOGS) Log.d(TAG, "onViewAttachedToWindow");
         if (mProxyView != null) mProxyView.onOriginalViewAttachedToWindow();
+        if (mHandler == null) mHandler = createHandler();
     }
 
     @Override
     public void onViewDetachedFromWindow() {
         if (DEBUG_LOGS) Log.d(TAG, "onViewDetachedFromWindow");
         if (mCheckInvalidator != null) mCheckInvalidator.invalidate();
         if (mProxyView != null) mProxyView.onOriginalViewDetachedFromWindow();
+        destroyHandler();

[Torne, 2016/11/29 13:22:54]

Destroying it and recreating it on detach/reattach seems unnecessary, is there
any reason to do this?

[Changwan Ryu, 2016/11/29 23:33:18]

If we're considering multiple WebView cases more carefully, I think we should be
quitting the handler when the number of webview instances go from 1 to 0.

[Torne, 2016/11/30 10:33:35]

I don't understand your answer here either; what we do when there's no webviews
doesn't have anything to do with what we should do on detach/attach, does it?

Nothing else in WebView quits threads/shuts things down when there are 0
webviews; it's not something we consider important and it's not generally
possible in most areas of Chromium (which are not designed to be shut down and
then subsequently restarted). If there was just one handler thread, then it
would be fine for it to continue running forever. It's only having a
continuously increasing number of threads that's not ok.

+    }
+
+    @Override
+    public void destroy() {

[Torne, 2016/11/29 13:22:55]

Normally we expect that destroy() methods leave objects in a permanently
destroyed state. This looks like the handler will just get recreated if the
object continues to be used? It's not really destroying the
InputConnectionFactory itself.

[Ted C, 2016/11/29 22:32:16]

Maybe reset would be a better name for this then.

[Changwan Ryu, 2016/11/29 23:33:18]

Hmm... We already have resetAndHidKeyboard() which is called by attach().
I could not come up with a good name yet, so let me keep the name as is for now.

[Torne, 2016/11/30 10:33:35]

Well, if we have a single handler thread then there's no need for a destroy
method here at all, so if that's possible then that avoids any issue here :)

+        if (mCheckInvalidator != null) mCheckInvalidator.invalidate();
+        destroyHandler();
     }
 }