Bug 1673681 - Software keyboard show/dimiss shouldn't be reentrant. r=geckoview-reviewers,agi
authorMakoto Kato <m_kato@ga2.so-net.ne.jp>
Wed, 18 Nov 2020 12:39:42 +0000
changeset 557801 f2903ccdb6ed0ba4f0e58fda306ad37306431f4d
parent 557800 c5fb03571b7289a39e78f8acd14eca6d575d1962
child 557802 46cf3717f89c04743b7c577ee3b0875185c0e95c
push id37962
push userapavel@mozilla.com
push dateWed, 18 Nov 2020 21:51:58 +0000
treeherdermozilla-central@9d797387f57c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgeckoview-reviewers, agi
bugs1673681, 1283739, 1464096
milestone85.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1673681 - Software keyboard show/dimiss shouldn't be reentrant. r=geckoview-reviewers,agi Originally, this issue was bug 1283739. But I guess that this might be regression by bug 1464096. Actually, reentrant code seems to be broken now, so I make it simple. Differential Revision: https://phabricator.services.mozilla.com/D97266
mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java
--- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java
@@ -1564,20 +1564,16 @@ import android.view.inputmethod.EditorIn
 
         ThreadUtils.runOnUiThread(new Runnable() {
             @Override
             public void run() {
                 if (DEBUG) {
                     Log.d(LOGTAG, "restartInput(" + reason + ", " + toggleSoftInput + ')');
                 }
 
-                if (toggleSoftInput) {
-                    mSoftInputReentrancyGuard.incrementAndGet();
-                }
-
                 final GeckoSession session = mSession.get();
                 if (session != null) {
                     session.getTextInput().getDelegate().restartInput(session, reason);
                 }
 
                 if (!toggleSoftInput) {
                     return;
                 }
@@ -1718,65 +1714,63 @@ import android.view.inputmethod.EditorIn
         // 3) through a system-generated onCreateInputConnection() call when the activity
         //    is restored from background, which then calls toggleSoftInput().
         // mSoftInputReentrancyGuard is needed to ensure that between the different paths,
         // the soft input is only toggled exactly once.
 
         ThreadUtils.runOnUiThread(new Runnable() {
             @Override
             public void run() {
-                final int reentrancyGuard = mSoftInputReentrancyGuard.decrementAndGet();
-                final boolean isReentrant;
-                if (reentrancyGuard < 0) {
-                    mSoftInputReentrancyGuard.incrementAndGet();
-                    isReentrant = false;
-                } else {
-                    isReentrant = reentrancyGuard > 0;
-                }
+                try {
+                    final int reentrancyGuard = mSoftInputReentrancyGuard.incrementAndGet();
+                    final boolean isReentrant =  reentrancyGuard > 1;
+
+                    // When using Find In Page, we can still receive notifyIMEContext calls due to the
+                    // selection changing when highlighting. However in this case we don't want to
+                    // show/hide the keyboard because the find box has the focus and is taking input from
+                    // the keyboard.
+                    final GeckoSession session = mSession.get();
 
-                // When using Find In Page, we can still receive notifyIMEContext calls due to the
-                // selection changing when highlighting. However in this case we don't want to
-                // show/hide the keyboard because the find box has the focus and is taking input from
-                // the keyboard.
-                final GeckoSession session = mSession.get();
+                    if (session == null) {
+                        return;
+                    }
 
-                if (session == null) {
-                    return;
-                }
+                    final View view = session.getTextInput().getView();
+                    final boolean isFocused = (view == null) || view.hasFocus();
 
-                final View view = session.getTextInput().getView();
-                final boolean isFocused = (view == null) || view.hasFocus();
-
-                final boolean isUserAction = ((flags &
-                        SessionTextInput.EditableListener.IME_FLAG_USER_ACTION) != 0);
+                    final boolean isUserAction = ((flags &
+                            SessionTextInput.EditableListener.IME_FLAG_USER_ACTION) != 0);
 
-                if (!force && (isReentrant || !isFocused || !isUserAction)) {
-                    if (DEBUG) {
-                        Log.d(LOGTAG, "toggleSoftInput: no-op, reentrant=" + isReentrant +
-                                ", focused=" + isFocused + ", user=" + isUserAction);
+                    if (!force && (isReentrant || !isFocused || !isUserAction)) {
+                        if (DEBUG) {
+                            Log.d(LOGTAG, "toggleSoftInput: no-op, reentrant=" + isReentrant +
+                                    ", focused=" + isFocused + ", user=" + isUserAction);
+                        }
+                        return;
+                    }
+                    if (state == SessionTextInput.EditableListener.IME_STATE_DISABLED) {
+                        session.getTextInput().getDelegate().hideSoftInput(session);
+                        return;
                     }
-                    return;
-                }
-                if (state == SessionTextInput.EditableListener.IME_STATE_DISABLED) {
-                    session.getTextInput().getDelegate().hideSoftInput(session);
-                    return;
+                    {
+                        final GeckoBundle bundle = new GeckoBundle();
+                        // This bit is subtle. We want to force-zoom to the input
+                        // if we're _not_ force-showing the virtual keyboard.
+                        //
+                        // We only force-show the virtual keyboard as a result of
+                        // something that _doesn't_ switch the focus, and we don't
+                        // want to move the view out of the focused editor unless
+                        // we _actually_ show toggle the keyboard.
+                        bundle.putBoolean("force", !force);
+                        session.getEventDispatcher().dispatch("GeckoView:ZoomToInput", bundle);
+                    }
+                    session.getTextInput().getDelegate().showSoftInput(session);
+                } finally {
+                    mSoftInputReentrancyGuard.decrementAndGet();
                 }
-                {
-                    final GeckoBundle bundle = new GeckoBundle();
-                    // This bit is subtle. We want to force-zoom to the input
-                    // if we're _not_ force-showing the virtual keyboard.
-                    //
-                    // We only force-show the virtual keyboard as a result of
-                    // something that _doesn't_ switch the focus, and we don't
-                    // want to move the view out of the focused editor unless
-                    // we _actually_ show toggle the keyboard.
-                    bundle.putBoolean("force", !force);
-                    session.getEventDispatcher().dispatch("GeckoView:ZoomToInput", bundle);
-                }
-                session.getTextInput().getDelegate().showSoftInput(session);
             }
         });
     }
 
     @Override // IGeckoEditableParent
     public void onSelectionChange(final IBinder token,
                                   final int start, final int end) {
         // On Gecko or binder thread.