Fix leaks in InputConnectionHandlerThread

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

Index: content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java b/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
index 5953108d0dcbb022823abe77e1da4c194c84ab5f..7e98b5bf14317768b438e370366d266046bd6dfc 100644
--- a/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
+++ b/content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java
@@ -10,6 +10,7 @@ import android.view.View;
 import android.view.inputmethod.EditorInfo;
 
 import org.chromium.base.Log;
+import org.chromium.base.ThreadUtils;
 import org.chromium.base.VisibleForTesting;
 
 /**
@@ -30,7 +31,7 @@ public class ThreadedInputConnectionFactory implements ChromiumBaseInputConnecti
     // 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;
@@ -56,7 +57,6 @@ public class ThreadedInputConnectionFactory implements ChromiumBaseInputConnecti
 
     ThreadedInputConnectionFactory(InputMethodManagerWrapper inputMethodManagerWrapper) {
         mInputMethodManagerWrapper = inputMethodManagerWrapper;
-        mHandler = createHandler();
         mInputMethodUma = createInputMethodUma();
     }
 
@@ -68,6 +68,25 @@ public class ThreadedInputConnectionFactory implements ChromiumBaseInputConnecti
         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) {
@@ -117,6 +136,8 @@ public class ThreadedInputConnectionFactory implements ChromiumBaseInputConnecti
         // mServedConnecting.
         if (mCheckInvalidator != null) mCheckInvalidator.invalidate();
 
+        if (mHandler == null) mHandler = createHandler();
+
         if (shouldTriggerDelayedOnCreateInputConnection()) {
             triggerDelayedOnCreateInputConnection(view);
             return null;
@@ -145,6 +166,7 @@ public class ThreadedInputConnectionFactory implements ChromiumBaseInputConnecti
 
         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);
 
@@ -173,6 +195,7 @@ public class ThreadedInputConnectionFactory implements ChromiumBaseInputConnecti
 
                 // 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() {
@@ -251,6 +274,7 @@ public class ThreadedInputConnectionFactory implements ChromiumBaseInputConnecti
     public void onViewAttachedToWindow() {
         if (DEBUG_LOGS) Log.d(TAG, "onViewAttachedToWindow");
         if (mProxyView != null) mProxyView.onOriginalViewAttachedToWindow();
+        if (mHandler == null) mHandler = createHandler();
     }
 
     @Override
@@ -258,5 +282,12 @@ public class ThreadedInputConnectionFactory implements ChromiumBaseInputConnecti
         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();
     }
 }