835906 - Run InputConnection on a separate thread

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

InputConnection calls are one area where we can block the UI thread.

But in theory InputConnection should be able to run on a separate thread. On one end, it doesn't depend on the Android UI, so it should be safe to run it separately from the UI thread. On the other end, it only talks to the IME through an IPC channel, which means this communication can happen on any thread.

InputMethodManager dispatches IPC InputConnection calls to the Handler associated with the focused View, so if we tell InputMethodManager our View has a different Handler, everything could Just Work :)

Comment 1

[Chris Peterson [:cpeterson]]

A bold idea; I like it. However, I'm worried that Android's InputConnection or some dependency might assume it is always running on the UI thread.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Refer to IC thread instead of UI thread in existing IME code (v1)

We are switching from using UI to using a background thread in IME operations. To avoid confusion, we will refer to the IME operation thread as the IC (InputConnection) thread.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add ability for GeckoEditable to switch to a different thread (v1)

GeckoEditable should have the ability to safely switch from using one thread to using another thread. This gives us some flexibility because we can choose to remain on the UI thread, but at an opportune time, we can switch to the background thread. This opportune time is usually when the system requests a new InputConnection from us, during which time it's safe to switch to using a new thread.

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 709869 [details] [diff] [review]
Refer to IC thread instead of UI thread in existing IME code (v1)

Review of attachment 709869 [details] [diff] [review]:
-----------------------------------------------------------------

The "IC thread" name is "icky" ;) but I can deal with it.

::: mobile/android/base/GeckoEditable.java
@@ +82,5 @@
>      private GeckoEditableListener mListener;
>      private int mSavedSelectionStart;
>      private volatile int mGeckoUpdateSeqno;
> +    private int mICUpdateSeqno;
> +    private int mLastICUpdateSeqno;

Java naming conventions recommend that only the first letter of abbreviations in identifier names be capitalized. For example, `XmlHttpRequest` instead of `XMLHTTPRequest`, so we should use `mIcUpdateSeqno` instead of `mICUpdateSeqno`.

@@ +267,5 @@
>          mListener = GeckoInputConnection.create(v, this);
>      }
>  
> +    private void assertOnICThread() {
> +    }

If we're not using assertOnICThread(), we should just remove it. I think the UI thread asserts are more useful because the UI thread is special and many Android APIs only work on the UI thread.

However, since this particular patch does not actually switch over from the UI thread to an IC thread, I think we should either leave the original assertOnUiThread() checks or temporarily implement assertOnIcThread() with a call to assertOnUiThread().

@@ +278,5 @@
>             request for update. If we incremented the seqno here, geckoUpdateGecko would have
>             prevented other updates from occurring */
>          final int seqnoWhenPosted = mGeckoUpdateSeqno;
>  
> +        geckoPostToIC(new Runnable() {

This won't work because geckoPostToIC() above is a nop method!

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Let GeckoInputConnection switch to background thread (v1)

Hack alert! The code is a bit hacky: basically we detect if InputMethodManager is requesting our Handler, and if so, give it our background Handler. But for other cases we give the UI Handler. Although hacky, I think it's pretty safe to do this, and considering the benefit I think it's worth it. We can always fall back on staying on the UI thread, so no damage is done.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Handle off-thread GeckoEditable and GeckoInputConnection calls (v1)

After we switch to running on a new thread, because the system still holds a reference to the old Handler, we may still get calls on the old thread. We can safely ignore those calls.

Also, key events originate on the UI thread, so we need to forward those events to the background thread.

Comment 7

[Chris Peterson [:cpeterson]]

Comment on attachment 709901 [details] [diff] [review]
Handle off-thread GeckoEditable and GeckoInputConnection calls (v1)

Review of attachment 709901 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, but some questions:

::: mobile/android/base/GeckoEditable.java
@@ +461,5 @@
> +    public void sendEvent(final GeckoEvent event) {
> +        if (!onICThread()) {
> +            // Events may get dispatched to the main thread;
> +            // reroute to our IC thread instead
> +            mICRunHandler.post(new Runnable() {

When you say "[GeckoEvents] may get dispatched to the main thread", where else would they come from? Aren't GeckoInputConnection onKeyDown() and onKeyUp() methods called by the Android framework on the UI thread?

If sendEvent() is *only* called from the UI thread, I think we should remove the "may get dispatched" comment, extract sendEvent's Gecko event code to a separate private method, and the new sendEvent() will simply post a Runnable to that private method. I think this would make it clearer which method is running on which thread.

@@ +486,5 @@
>      public Editable getEditable() {
> +        if (!onICThread()) {
> +            // Android may be holding an old InputConnection; ignore
> +            if (DEBUG) {
> +                Log.i(LOGTAG, "getEditable() called on non-IC thread");

When might this happen? During or after the switch from the old IC thread to the new IC thread? It would be nice to have a debug flag that is set after the IC thread is switched (and pending Runnables on the UI thread have all been processed). We could use the debug flag to add error checking for methods that should no longer be called on the UI thread.

@@ +498,5 @@
>      public void setUpdateGecko(boolean update) {
> +        if (!onICThread()) {
> +            // Android may be holding an old InputConnection; ignore
> +            if (DEBUG) {
> +                Log.i(LOGTAG, "setUpdateGecko() called on non-IC thread");

ditto

Comment 8

[Chris Peterson [:cpeterson]]

Comment on attachment 709898 [details] [diff] [review]
Let GeckoInputConnection switch to background thread (v1)

Review of attachment 709898 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoInputConnection.java
@@ +348,5 @@
> +        }
> +        for (StackTraceElement frame : Thread.currentThread().getStackTrace()) {
> +            if ("startInputInner".equals(frame.getMethodName()) &&
> +                "android.view.inputmethod.InputMethodManager".equals(frame.getClassName())) {
> +                // only return our own Handler to InputMethodManager

Please add a comment describing the significance of InputMethodManager.startInputInner().

@@ +363,5 @@
> +        }
> +        // Don't use GeckoBackgroundThread because Gecko thread may block waiting on
> +        // GeckoBackgroundThread. If we were to use GeckoBackgroundThread, due to IME,
> +        // GeckoBackgroundThread may end up also block waiting on Gecko thread and a
> +        // deadlock occurs

Would this comment make more sense inside the getBackgroundHandler() method?

@@ +366,5 @@
> +        // GeckoBackgroundThread may end up also block waiting on Gecko thread and a
> +        // deadlock occurs
> +        final Handler newHandler = sBackgroundHandler != null
> +                                 ? sBackgroundHandler
> +                                 : getBackgroundHandler();

Are you checking whether sBackgroundHandler is null to avoid calling the synchronized method getBackgroundHandler()? You should either add a comment explaining that or (my slight preference) always call getBackgroundHandler().

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 709891 [details] [diff] [review]
Add ability for GeckoEditable to switch to a different thread (v1)

Review of attachment 709891 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoEditable.java
@@ +171,5 @@
>          }
> +
> +        static Action newSetHandler(Handler handler) {
> +            final Action action = new Action(TYPE_SET_HANDLER);
> +            action.mHandler = handler; // reusing mSpanObject field

Where is mSpanObject reused?

@@ +515,5 @@
> +        if (DEBUG) {
> +            assertOnICThread();
> +        }
> +        // There are three threads at this point: Gecko thread, old IC thread, and new IC
> +        // thread, and we want to safely switch from old IC thread to new IC thread.

I think the terms "old IC thread" and "new IC thread" are a little confusing. I think "UI thread" and "IC thread" would be clearer.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Chris Peterson (:cpeterson) from comment #7)
> Comment on attachment 709901 [details] [diff] [review]
> Handle off-thread GeckoEditable and GeckoInputConnection calls (v1)
> 
> > +    public void sendEvent(final GeckoEvent event) {
> > +        if (!onICThread()) {
> > +            // Events may get dispatched to the main thread;
> > +            // reroute to our IC thread instead
> > +            mICRunHandler.post(new Runnable() {
> 
> When you say "[GeckoEvents] may get dispatched to the main thread", where
> else would they come from? Aren't GeckoInputConnection onKeyDown() and
> onKeyUp() methods called by the Android framework on the UI thread?

Yes, onKeyDown() and onKeyUp() are called from the UI thread, but I like the option of being able to call sendEvent() from either the UI or the IC thread. For example, if we ever decide to translate some IME operations to key strokes, we would be able to use sendEvent() to send a key event from the IC thread. Of course, sendEvent() is not limited to key events; we don't use other events now, but they are possible.

> >      public Editable getEditable() {
> > +        if (!onICThread()) {
> > +            // Android may be holding an old InputConnection; ignore
> > +            if (DEBUG) {
> > +                Log.i(LOGTAG, "getEditable() called on non-IC thread");
> 
> When might this happen? During or after the switch from the old IC thread to
> the new IC thread? It would be nice to have a debug flag that is set after
> the IC thread is switched (and pending Runnables on the UI thread have all
> been processed). We could use the debug flag to add error checking for
> methods that should no longer be called on the UI thread.

I think it happens during/immediately after the switch. The problem is that when Android is cleaning up the old InputConnection, it may end up calling the old InputConnection on the old thread. So that's not something we can fix. What we can do is to return null for Editable, and effectively the InputConnection call is ignored.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Refer to IC thread instead of UI thread in existing IME code (v2)

(In reply to Chris Peterson (:cpeterson) from comment #4)
> Comment on attachment 709869 [details] [diff] [review]
> Refer to IC thread instead of UI thread in existing IME code (v1)
> 
> >      private int mSavedSelectionStart;
> >      private volatile int mGeckoUpdateSeqno;
> > +    private int mICUpdateSeqno;
> > +    private int mLastICUpdateSeqno;
> 
> Java naming conventions recommend that only the first letter of
> abbreviations in identifier names be capitalized. For example,
> `XmlHttpRequest` instead of `XMLHTTPRequest`, so we should use
> `mIcUpdateSeqno` instead of `mICUpdateSeqno`.

Changed.

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Let GeckoInputConnection switch to background thread (v2)

(In reply to Chris Peterson (:cpeterson) from comment #8)
> Comment on attachment 709898 [details] [diff] [review]
> Let GeckoInputConnection switch to background thread (v1)
> 
> > +        for (StackTraceElement frame : Thread.currentThread().getStackTrace()) {
> > +            if ("startInputInner".equals(frame.getMethodName()) &&
> > +                "android.view.inputmethod.InputMethodManager".equals(frame.getClassName())) {
> > +                // only return our own Handler to InputMethodManager
> 
> Please add a comment describing the significance of
> InputMethodManager.startInputInner().

Added.

> > +        // Don't use GeckoBackgroundThread because Gecko thread may block waiting on
> > +        // GeckoBackgroundThread. If we were to use GeckoBackgroundThread, due to IME,
> > +        // GeckoBackgroundThread may end up also block waiting on Gecko thread and a
> > +        // deadlock occurs
> 
> Would this comment make more sense inside the getBackgroundHandler() method?

Yup; moved to getBackgroundHandler().

> > +        final Handler newHandler = sBackgroundHandler != null
> > +                                 ? sBackgroundHandler
> > +                                 : getBackgroundHandler();
> 
> Are you checking whether sBackgroundHandler is null to avoid calling the
> synchronized method getBackgroundHandler()? You should either add a comment
> explaining that or (my slight preference) always call getBackgroundHandler().

Added a comment. To me getHandler() should be a "lightweight" method, so I want to avoid locking if we don't have to.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Chris Peterson (:cpeterson) from comment #9)
> Comment on attachment 709891 [details] [diff] [review]
> Add ability for GeckoEditable to switch to a different thread (v1)
> 
> > +
> > +        static Action newSetHandler(Handler handler) {
> > +            final Action action = new Action(TYPE_SET_HANDLER);
> > +            action.mHandler = handler; // reusing mSpanObject field
> 
> Where is mSpanObject reused?

Oops, forgot to take out the comment. Originally I had used mSpanObject to hold the handler, but later on I added a separate mHandler field.

> > +        // There are three threads at this point: Gecko thread, old IC thread, and new IC
> > +        // thread, and we want to safely switch from old IC thread to new IC thread.
> 
> I think the terms "old IC thread" and "new IC thread" are a little
> confusing. I think "UI thread" and "IC thread" would be clearer.

Hmm, hard to avoid confusion. If we define IC thread as the thread that run InputConnection calls, then the UI thread *is* the IC thread before we switch to a background thread.

Comment 14

[Chris Peterson [:cpeterson]]

Comment on attachment 711685 [details] [diff] [review]
Refer to IC thread instead of UI thread in existing IME code (v2)

Review of attachment 711685 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 15

[Chris Peterson [:cpeterson]]

Comment on attachment 711687 [details] [diff] [review]
Let GeckoInputConnection switch to background thread (v2)

Review of attachment 711687 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 16

[Chris Peterson [:cpeterson]]

Comment on attachment 709891 [details] [diff] [review]
Add ability for GeckoEditable to switch to a different thread (v1)

LGTM

Comment 17

[Chris Peterson [:cpeterson]]

Comment on attachment 709901 [details] [diff] [review]
Handle off-thread GeckoEditable and GeckoInputConnection calls (v1)

LGTM

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe60781d9ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/364d4dd9ffbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/1579121d48d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/97f6e73073dd

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cbe60781d9ee
https://hg.mozilla.org/mozilla-central/rev/364d4dd9ffbd
https://hg.mozilla.org/mozilla-central/rev/1579121d48d2
https://hg.mozilla.org/mozilla-central/rev/97f6e73073dd