Let ImeAdapterAndroid have the same lifecycle as its Java peer

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

Index: content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java b/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
index db0e84c923bdc66a374a3c3260a6a03482da1809..a31d175b62a4adb3cb1a17166b42c2b9c63eb9b1 100644
--- a/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
+++ b/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
@@ -92,7 +92,7 @@ public class ImeAdapter {
     static char[] sSingleCharArray = new char[1];
     static KeyCharacterMap sKeyCharacterMap;
 
-    private long mNativeImeAdapterAndroid;
+    private final long mNativeImeAdapterAndroid;
     private InputMethodManagerWrapper mInputMethodManagerWrapper;
     private ChromiumBaseInputConnection mInputConnection;
     private ChromiumBaseInputConnection.Factory mInputConnectionFactory;
@@ -117,6 +117,7 @@ public class ImeAdapter {
     private int mLastCompositionStart;
     private int mLastCompositionEnd;
     private boolean mRestartInputOnNextStateUpdate;
+    private boolean mIsConnected;
 
     /**
      * @param wrapper InputMethodManagerWrapper that should receive all the call directed to
@@ -157,6 +158,11 @@ public class ImeAdapter {
         } else {
             mCursorAnchorInfoController = null;
         }
+        mNativeImeAdapterAndroid = nativeInit();
+    }
+
+    public long getNativePointer() {
+        return mNativeImeAdapterAndroid;
     }
 
     private void createInputConnectionFactory() {
@@ -190,7 +196,7 @@ public class ImeAdapter {
                     false /* not an immediate request */, false /* disable monitoring */,
                     mViewEmbedder.getAttachedView());
         }
-        if (mNativeImeAdapterAndroid != 0) {
+        if (mIsConnected) {
             nativeRequestCursorUpdate(mNativeImeAdapterAndroid,
                     false /* not an immediate request */, false /* disable monitoring */);
         }
@@ -325,27 +331,6 @@ public class ImeAdapter {
     }
 
     /**
-     * Attaches the imeAdapter to its native counterpart. This is needed to start forwarding
-     * keyboard events to WebKit.
-     * @param nativeImeAdapter The pointer to the native ImeAdapter object.
-     */
-    public void attach(long nativeImeAdapter) {
-        if (DEBUG_LOGS) Log.i(TAG, "attach");
-        if (mNativeImeAdapterAndroid == nativeImeAdapter) return;
-        if (mNativeImeAdapterAndroid != 0) {
-            nativeResetImeAdapter(mNativeImeAdapterAndroid);
-        }
-        if (nativeImeAdapter != 0) {
-            nativeAttachImeAdapter(nativeImeAdapter);
-        }
-        mNativeImeAdapterAndroid = nativeImeAdapter;
-        if (nativeImeAdapter != 0) {
-            createInputConnectionFactory();
-        }
-        resetAndHideKeyboard();
-    }
-
-    /**
      * Show soft keyboard only if it is the current keyboard configuration.
      */
     private void showSoftKeyboard() {
@@ -485,6 +470,14 @@ public class ImeAdapter {
         hideKeyboard();
     }
 
+    public void destroy() {
+        resetAndHideKeyboard();
+        nativeDestroy(mNativeImeAdapterAndroid);

[Changwan Ryu, 2017/03/17 23:47:19]

Could you also set mNativeImeAdapterAndroid to 0 here?

Calling ImeAdapter#destroy() and then calling some other methods may lead to a
crash that is difficult to root-cause.

I prefer not to leave such possibility.

[Jinsuk Kim, 2017/03/19 23:44:20]

I believe the intention of |mNativeImeAdapterAndroid == 0| was to check if
the adapter  is attached or not. After the CL, it cannot be used for that
purpose - it is always non-zero since the native object is instantiated together
with this Java class and has the same lifetime. Attached/Detached state is
reported through |onConnectedRenderProcess| now, and checking |mIsConnected|
can do exactly what |mNativeImeAdapterAndroid == 0| did. 

Is there any tests in place that will safe-guard or steps to create situations
you're concerned of? Will test that.

[Changwan Ryu, 2017/03/20 18:23:00]

ImeAdapter#destroy() -> ImeAdapter#finishComposingText() will crash in a way
that is very difficult to understand. (You can try...)

This may happen if ImeAdapter gets destroyed and then IME thread calls IME
operations on the next UI message loop.

Also I think this concern is still valid even if this is not a possible path for
now because such safety logic should be self contained regardless of potential
caller path changes.

[Jinsuk Kim, 2017/03/21 00:09:11]

Added back the checking against mNativeImeAdapterAndroid, as it was clarified
that ThreadedInputConnection still can call on ImeAdapter after destroy() is
called.

+        if (mCursorAnchorInfoController != null) {
+            mCursorAnchorInfoController.focusedNodeChanged(false);
+        }
+    }
+
     /**
      * Update selection to input method manager.
      *
@@ -517,7 +510,7 @@ public class ImeAdapter {
     }
 
     boolean performEditorAction(int actionCode) {
-        if (mNativeImeAdapterAndroid == 0) return false;

[Changwan Ryu, 2017/03/17 23:47:19]

I prefer to keep all these native pointer checks for the same reason mentioned
above.

[Jinsuk Kim, 2017/03/19 23:44:20]

mIsConnected is equivalent to the native pointer in terms of representing the
attach/detach state.

+        if (!mIsConnected) return false;
         if (actionCode == EditorInfo.IME_ACTION_NEXT) {
             sendSyntheticKeyPress(KeyEvent.KEYCODE_TAB,
                     KeyEvent.FLAG_SOFT_KEYBOARD | KeyEvent.FLAG_KEEP_TOUCH_MODE
@@ -549,7 +542,7 @@ public class ImeAdapter {
 
     boolean sendCompositionToNative(
             CharSequence text, int newCursorPosition, boolean isCommit, int unicodeFromKeyEvent) {
-        if (mNativeImeAdapterAndroid == 0) return false;
+        if (!mIsConnected) return false;
 
         // One WebView app detects Enter in JS by looking at KeyDown (http://crbug/577967).
         if (TextUtils.equals(text, "\n")) {
@@ -577,13 +570,13 @@ public class ImeAdapter {
 
     @VisibleForTesting
     boolean finishComposingText() {
-        if (mNativeImeAdapterAndroid == 0) return false;
+        if (!mIsConnected) return false;
         nativeFinishComposingText(mNativeImeAdapterAndroid);
         return true;
     }
 
     boolean sendKeyEvent(KeyEvent event) {
-        if (mNativeImeAdapterAndroid == 0) return false;
+        if (!mIsConnected) return false;
 
         int action = event.getAction();
         int type;
@@ -615,7 +608,7 @@ public class ImeAdapter {
      */
     boolean deleteSurroundingText(int beforeLength, int afterLength) {
         mViewEmbedder.onImeEvent();
-        if (mNativeImeAdapterAndroid == 0) return false;
+        if (!mIsConnected) return false;
         nativeSendKeyEvent(mNativeImeAdapterAndroid, null, WebInputEventType.RawKeyDown, 0,
                 SystemClock.uptimeMillis(), COMPOSITION_KEY_CODE, 0, false, 0);
         nativeDeleteSurroundingText(mNativeImeAdapterAndroid, beforeLength, afterLength);
@@ -634,7 +627,7 @@ public class ImeAdapter {
      */
     boolean deleteSurroundingTextInCodePoints(int beforeLength, int afterLength) {
         mViewEmbedder.onImeEvent();
-        if (mNativeImeAdapterAndroid == 0) return false;
+        if (!mIsConnected) return false;
         nativeSendKeyEvent(mNativeImeAdapterAndroid, null, WebInputEventType.RawKeyDown, 0,
                 SystemClock.uptimeMillis(), COMPOSITION_KEY_CODE, 0, false, 0);
         nativeDeleteSurroundingTextInCodePoints(
@@ -651,7 +644,7 @@ public class ImeAdapter {
      * @return Whether the native counterpart of ImeAdapter received the call.
      */
     boolean setEditableSelectionOffsets(int start, int end) {
-        if (mNativeImeAdapterAndroid == 0) return false;
+        if (!mIsConnected) return false;
         nativeSetEditableSelectionOffsets(mNativeImeAdapterAndroid, start, end);
         return true;
     }
@@ -663,7 +656,7 @@ public class ImeAdapter {
      * @return Whether the native counterpart of ImeAdapter received the call.
      */
     boolean setComposingRegion(int start, int end) {
-        if (mNativeImeAdapterAndroid == 0) return false;
+        if (!mIsConnected) return false;
         if (start <= end) {
             nativeSetComposingRegion(mNativeImeAdapterAndroid, start, end);
         } else {
@@ -690,7 +683,7 @@ public class ImeAdapter {
      * Send a request to the native counterpart to give the latest text input state update.
      */
     boolean requestTextInputStateUpdate() {
-        if (mNativeImeAdapterAndroid == 0) return false;
+        if (!mIsConnected) return false;
         // You won't get state update anyways.
         if (mInputConnection == null) return false;
         return nativeRequestTextInputStateUpdate(mNativeImeAdapterAndroid);
@@ -705,7 +698,7 @@ public class ImeAdapter {
         final boolean monitorRequest =
                 (cursorUpdateMode & InputConnection.CURSOR_UPDATE_MONITOR) != 0;
 
-        if (mNativeImeAdapterAndroid != 0) {
+        if (mIsConnected) {
             nativeRequestCursorUpdate(mNativeImeAdapterAndroid, immediateRequest, monitorRequest);
         }
         if (mCursorAnchorInfoController == null) return false;
@@ -770,14 +763,21 @@ public class ImeAdapter {
     }
 
     @CalledByNative
-    private void detach() {
-        if (DEBUG_LOGS) Log.i(TAG, "detach");
-        mNativeImeAdapterAndroid = 0;
-        if (mCursorAnchorInfoController != null) {
-            mCursorAnchorInfoController.focusedNodeChanged(false);
+    private void onConnectedToRenderProcess(boolean connected) {
+        if (DEBUG_LOGS) Log.w(TAG, "onConnectedToRenderPrcess: " + connected);
+        mIsConnected = connected;
+        if (connected) {
+            createInputConnectionFactory();
+            resetAndHideKeyboard();
+        } else {
+            if (mCursorAnchorInfoController != null) {
+                mCursorAnchorInfoController.focusedNodeChanged(false);
+            }
         }
     }
 
+    private native long nativeInit();
+    private native void nativeDestroy(long nativeImeAdapterAndroid);
     private native boolean nativeSendKeyEvent(long nativeImeAdapterAndroid, KeyEvent event,
             int type, int modifiers, long timestampMs, int keyCode, int scanCode,
             boolean isSystemKey, int unicodeChar);
@@ -789,7 +789,6 @@ public class ImeAdapter {
     private native void nativeCommitText(
             long nativeImeAdapterAndroid, CharSequence text, String textStr, int newCursorPosition);
     private native void nativeFinishComposingText(long nativeImeAdapterAndroid);
-    private native void nativeAttachImeAdapter(long nativeImeAdapterAndroid);
     private native void nativeSetEditableSelectionOffsets(long nativeImeAdapterAndroid,
             int start, int end);
     private native void nativeSetComposingRegion(long nativeImeAdapterAndroid, int start, int end);
@@ -797,7 +796,6 @@ public class ImeAdapter {
             int before, int after);
     private native void nativeDeleteSurroundingTextInCodePoints(
             long nativeImeAdapterAndroid, int before, int after);
-    private native void nativeResetImeAdapter(long nativeImeAdapterAndroid);
     private native boolean nativeRequestTextInputStateUpdate(long nativeImeAdapterAndroid);
     private native void nativeRequestCursorUpdate(long nativeImeAdapterAndroid,
             boolean immediateRequest, boolean monitorRequest);