Let ImeAdapterAndroid have the same lifecycle as its Java peer

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

 // Copyright 2012 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.content.res.Configuration;
 import android.os.Build;
 import android.os.ResultReceiver;
 import android.os.SystemClock;
@@ skipping 74 matching lines @@
 
         /**
          * @return Object that should be called for all keyboard show and hide requests.
          */
         ResultReceiver getNewShowKeyboardReceiver();
     }
 
     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;
 
     private final ImeAdapterDelegate mViewEmbedder;
     // This holds the information necessary for constructing CursorAnchorInfo, and notifies to
     // InputMethodManager on appropriate timing, depending on how IME requested the information
     // via InputConnection. The update request is per InputConnection, hence for each time it is
     // re-created, the monitoring status will be reset.
     private final CursorAnchorInfoController mCursorAnchorInfoController;
 
     private int mTextInputType = TextInputType.NONE;
     private int mTextInputFlags;
     private int mTextInputMode = WebTextInputMode.kDefault;
 
     // Keep the current configuration to detect the change when onConfigurationChanged() is called.
     private Configuration mCurrentConfig;
 
     private int mLastSelectionStart;
     private int mLastSelectionEnd;
     private String mLastText;
     private int mLastCompositionStart;
     private int mLastCompositionEnd;
     private boolean mRestartInputOnNextStateUpdate;
+    private boolean mIsConnected;
 
     /**
      * @param wrapper InputMethodManagerWrapper that should receive all the call directed to
      *                InputMethodManager.
      * @param embedder The view that is used for callbacks from ImeAdapter.
      */
     public ImeAdapter(InputMethodManagerWrapper wrapper, ImeAdapterDelegate embedder) {
         mInputMethodManagerWrapper = wrapper;
         mViewEmbedder = embedder;
         // Deep copy newConfig so that we can notice the difference.
@@ skipping 20 matching lines @@
                             return mLastCompositionStart;
                         }
                         @Override
                         public int getComposingTextEnd() {
                             return mLastCompositionEnd;
                         }
                     });
         } else {
             mCursorAnchorInfoController = null;
         }
+        mNativeImeAdapterAndroid = nativeInit();
+    }
+
+    public long getNativePointer() {
+        return mNativeImeAdapterAndroid;
     }
 
     private void createInputConnectionFactory() {
         if (mInputConnectionFactory != null) return;
         mInputConnectionFactory = new ThreadedInputConnectionFactory(mInputMethodManagerWrapper);
     }
 
     /**
      * @see View#onCreateInputConnection(EditorInfo)
      */
@@ skipping 13 matching lines @@
         setInputConnection(mInputConnectionFactory.initializeAndGet(mViewEmbedder.getAttachedView(),
                 this, mTextInputType, mTextInputFlags, mTextInputMode, mLastSelectionStart,
                 mLastSelectionEnd, outAttrs));
         if (DEBUG_LOGS) Log.i(TAG, "onCreateInputConnection: " + mInputConnection);
 
         if (mCursorAnchorInfoController != null) {
             mCursorAnchorInfoController.onRequestCursorUpdates(
                     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 */);
         }
         return mInputConnection;
     }
 
     private void setInputConnection(ChromiumBaseInputConnection inputConnection) {
         if (mInputConnection == inputConnection) return;
         // The previous input connection might be waiting for state update.
         if (mInputConnection != null) mInputConnection.unblockOnUiThread();
@@ skipping 114 matching lines @@
         }
 
         if (mInputConnection == null) return;
         boolean singleLine = mTextInputType != TextInputType.TEXT_AREA
                 && mTextInputType != TextInputType.CONTENT_EDITABLE;
         mInputConnection.updateStateOnUiThread(text, selectionStart, selectionEnd, compositionStart,
                 compositionEnd, singleLine, replyToRequest);
     }
 
     /**
-     * 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() {
         if (DEBUG_LOGS) Log.i(TAG, "showSoftKeyboard");
         mInputMethodManagerWrapper.showSoftInput(
                 mViewEmbedder.getAttachedView(), 0, mViewEmbedder.getNewShowKeyboardReceiver());
         if (mViewEmbedder.getAttachedView().getResources().getConfiguration().keyboard
                 != Configuration.KEYBOARD_NOKEYS) {
             mViewEmbedder.onKeyboardBoundsUnchanged();
         }
@@ skipping 119 matching lines @@
     public void resetAndHideKeyboard() {
         if (DEBUG_LOGS) Log.i(TAG, "resetAndHideKeyboard");
         mTextInputType = TextInputType.NONE;
         mTextInputFlags = 0;
         mTextInputMode = WebTextInputMode.kDefault;
         mRestartInputOnNextStateUpdate = false;
         // This will trigger unblocking if necessary.
         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.
      *
      * @param selectionStart The selection start.
      * @param selectionEnd The selection end.
      * @param compositionStart The composition start.
      * @param compositionEnd The composition end.
      */
     void updateSelection(
             int selectionStart, int selectionEnd, int compositionStart, int compositionEnd) {
@@ skipping 12 matching lines @@
 
     /**
      * @see BaseInputConnection#performContextMenuAction(int)
      */
     boolean performContextMenuAction(int id) {
         if (DEBUG_LOGS) Log.i(TAG, "performContextMenuAction: id [%d]", id);
         return mViewEmbedder.performContextMenuAction(id);
     }
 
     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
                     | KeyEvent.FLAG_EDITOR_ACTION);
         } else {
             sendSyntheticKeyPress(KeyEvent.KEYCODE_ENTER,
                     KeyEvent.FLAG_SOFT_KEYBOARD | KeyEvent.FLAG_KEEP_TOUCH_MODE
                     | KeyEvent.FLAG_EDITOR_ACTION);
         }
         return true;
@@ skipping 11 matching lines @@
                 KeyCharacterMap.VIRTUAL_KEYBOARD, 0,
                 flags));
         sendKeyEvent(new KeyEvent(eventTime, eventTime,
                 KeyEvent.ACTION_UP, keyCode, 0, 0,
                 KeyCharacterMap.VIRTUAL_KEYBOARD, 0,
                 flags));
     }
 
     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")) {
             sendSyntheticKeyPress(KeyEvent.KEYCODE_ENTER,
                     KeyEvent.FLAG_SOFT_KEYBOARD | KeyEvent.FLAG_KEEP_TOUCH_MODE);
             return true;
         }
 
         mViewEmbedder.onImeEvent();
         long timestampMs = SystemClock.uptimeMillis();
         nativeSendKeyEvent(mNativeImeAdapterAndroid, null, WebInputEventType.RawKeyDown, 0,
                 timestampMs, COMPOSITION_KEY_CODE, 0, false, unicodeFromKeyEvent);
 
         if (isCommit) {
             nativeCommitText(mNativeImeAdapterAndroid, text, text.toString(), newCursorPosition);
         } else {
             nativeSetComposingText(
                     mNativeImeAdapterAndroid, text, text.toString(), newCursorPosition);
         }
 
         nativeSendKeyEvent(mNativeImeAdapterAndroid, null, WebInputEventType.KeyUp, 0, timestampMs,
                 COMPOSITION_KEY_CODE, 0, false, unicodeFromKeyEvent);
         return true;
     }
 
     @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;
         if (action == KeyEvent.ACTION_DOWN) {
             type = WebInputEventType.KeyDown;
         } else if (action == KeyEvent.ACTION_UP) {
             type = WebInputEventType.KeyUp;
         } else {
             // In theory, KeyEvent.ACTION_MULTIPLE is a valid value, but in practice
             // this seems to have been quietly deprecated and we've never observed
@@ skipping 11 matching lines @@
     /**
      * Send a request to the native counterpart to delete a given range of characters.
      * @param beforeLength Number of characters to extend the selection by before the existing
      *                     selection.
      * @param afterLength Number of characters to extend the selection by after the existing
      *                    selection.
      * @return Whether the native counterpart of ImeAdapter received the call.
      */
     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);
         nativeSendKeyEvent(mNativeImeAdapterAndroid, null, WebInputEventType.KeyUp, 0,
                 SystemClock.uptimeMillis(), COMPOSITION_KEY_CODE, 0, false, 0);
         return true;
     }
 
     /**
      * Send a request to the native counterpart to delete a given range of characters.
      * @param beforeLength Number of code points to extend the selection by before the existing
      *                     selection.
      * @param afterLength Number of code points to extend the selection by after the existing
      *                    selection.
      * @return Whether the native counterpart of ImeAdapter received the call.
      */
     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(
                 mNativeImeAdapterAndroid, beforeLength, afterLength);
         nativeSendKeyEvent(mNativeImeAdapterAndroid, null, WebInputEventType.KeyUp, 0,
                 SystemClock.uptimeMillis(), COMPOSITION_KEY_CODE, 0, false, 0);
         return true;
     }
 
     /**
      * Send a request to the native counterpart to set the selection to given range.
      * @param start Selection start index.
      * @param end Selection end index.
      * @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;
     }
 
     /**
      * Send a request to the native counterpart to set composing region to given indices.
      * @param start The start of the composition.
      * @param end The end of the composition.
      * @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 {
             nativeSetComposingRegion(mNativeImeAdapterAndroid, end, start);
         }
         return true;
     }
 
     @CalledByNative
     private void focusedNodeChanged(boolean isEditable) {
         if (DEBUG_LOGS) Log.i(TAG, "focusedNodeChanged: isEditable [%b]", isEditable);
 
         // Update controller before the connection is restarted.
         if (mCursorAnchorInfoController != null) {
             mCursorAnchorInfoController.focusedNodeChanged(isEditable);
         }
 
         if (mTextInputType != TextInputType.NONE && mInputConnection != null && isEditable) {
             mRestartInputOnNextStateUpdate = true;
         }
     }
 
     /**
      * 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);
     }
 
     /**
      * Notified when IME requested Chrome to change the cursor update mode.
      */
     public boolean onRequestCursorUpdates(int cursorUpdateMode) {
         final boolean immediateRequest =
                 (cursorUpdateMode & InputConnection.CURSOR_UPDATE_IMMEDIATE) != 0;
         final boolean monitorRequest =
                 (cursorUpdateMode & InputConnection.CURSOR_UPDATE_MONITOR) != 0;
 
-        if (mNativeImeAdapterAndroid != 0) {
+        if (mIsConnected) {
             nativeRequestCursorUpdate(mNativeImeAdapterAndroid, immediateRequest, monitorRequest);
         }
         if (mCursorAnchorInfoController == null) return false;
         return mCursorAnchorInfoController.onRequestCursorUpdates(immediateRequest, monitorRequest,
                 mViewEmbedder.getAttachedView());
     }
 
     /**
      * Notified when a frame has been produced by the renderer and all the associated metadata.
      * @param renderCoordinates coordinate information to convert CSS (document) coordinates to
@@ skipping 44 matching lines @@
     }
 
     @CalledByNative
     private void setCharacterBounds(float[] characterBounds) {
         if (mCursorAnchorInfoController == null) return;
         mCursorAnchorInfoController.setCompositionCharacterBounds(characterBounds,
                 mViewEmbedder.getAttachedView());
     }
 
     @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);
     private static native void nativeAppendUnderlineSpan(long underlinePtr, int start, int end);
     private static native void nativeAppendBackgroundColorSpan(long underlinePtr, int start,
             int end, int backgroundColor);
     private native void nativeSetComposingText(long nativeImeAdapterAndroid, CharSequence text,
             String textStr, int newCursorPosition);
     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);
     private native void nativeDeleteSurroundingText(long nativeImeAdapterAndroid,
             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);
 }