769520 - Fix IME race conditions by ensuring Gecko callbacks post results to UI thread

Status: RESOLVED DUPLICATE of bug 805162

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

Description

[Chris Peterson [:cpeterson]]

The Gecko thread and the IMEStateUpdater TimerTask thread both call GeckoInputConnection.java callbacks while the UI thread may be running simultaneously. I believe this race condition may be the source of many IME crashes and selection bugs.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: part-1-post-IME-callbacks-to-UI-thread.patch

Part 1: Post Gecko's IME callbacks from Gecko thread to UI thread.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-2-reindent-whitespace.patch

Part 2: Reindent GeckoInputConnection.java. Whitespace changes only!

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: part-3-add-thread-asserts.patch

Part 3: In debug builds, assert IME code is running on UI thread. These thread checks are ONLY run in local builds where GeckoInputConnection.java has been manually tweaked to set `DEBUG = true`.

We may want to add assertOnUiThread() calls to other critical threaded code. For example, gfx/TouchEventHandler.java has many loud comments about "This function MUST be called on the UI thread", but no runtime checks.

Comment 4

[Chris Peterson [:cpeterson]]

This patch fixes bug 768105 and (partially fixes) bug 751513.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 637761 [details] [diff] [review]
part-1-post-IME-callbacks-to-UI-thread.patch

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

::: mobile/android/base/GeckoInputConnection.java
@@ +999,1 @@
>          View v = GeckoApp.mAppContext.getLayerController().getView();

nit, fix indentation

You could also just do:

public void notifyIME(final int type, final int state) {
		GeckoApp.postToUiThread(new Runnable() {
		public void run() {
                    notifyIMEImpl(type, state);
                }
            });
}

public void notifyIMEImpl(int type, int state) {

@@ +1047,1 @@
>          View v = GeckoApp.mAppContext.getLayerController().getView();

same as above

@@ +1112,1 @@
>              final View v = GeckoApp.mAppContext.getLayerController().getView();

same

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 637761 [details] [diff] [review]
part-1-post-IME-callbacks-to-UI-thread.patch

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

::: mobile/android/base/GeckoApp.java
@@ +3189,5 @@
>          return false;
>      }
>  
> +    public static void postToUiThread(Runnable runnable) {
> +        mAppContext.mMainHandler.post(runnable);

this already exists: http://developer.android.com/reference/android/app/Activity.html#runOnUiThread%28java.lang.Runnable%29

so drop this and use runOnUiThread

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 637764 [details] [diff] [review]
part-3-add-thread-asserts.patch

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

::: mobile/android/base/GeckoApp.java
@@ +85,5 @@
>      public static boolean mDOMFullScreen = false;
>      protected MenuPanel mMenuPanel;
>      protected Menu mMenu;
> +    private static GeckoThread sGeckoThread;
> +    private static final Thread sUiThread = Thread.currentThread();

this looks like we're asking for pain. Do we have any guarantee that our static variables will be constructed on the main ui thread?

I'd prefer you set this on onCreate() if you need it staticly. Also keep in mind its always queriable from a static context with:

 GeckoApp.mAppContext.getMainLooper().getThread();

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Brad Lassey [:blassey] from comment #6)
> this already exists:
> http://developer.android.com/reference/android/app/Activity.
> html#runOnUiThread%28java.lang.Runnable%29
> 
> so drop this and use runOnUiThread

I don't like runOnUiThread() because:

1. It increases nondeterminism. If it is called on the UI thread, then the Runnable will be executed before queued Runnables that had been posted earlier.

2. If the caller wants to execute code on the UI thread but doesn't even know if they _are_ the UI thread, then that code probably needs to be redesigned.

Comment 9

[Chris Peterson [:cpeterson]]

(In reply to Brad Lassey [:blassey] from comment #5)
> ::: mobile/android/base/GeckoInputConnection.java
> @@ +999,1 @@
> >          View v = GeckoApp.mAppContext.getLayerController().getView();
> 
> nit, fix indentation

I knew patch 1's indentation was wrong. I was just postponing the whitespace changes to patch 2 so patch 1 would be easier to review (since it already dealt with sensitive threading issues). But I can just fix the indentation in patch 1 (thus obsoleting patch 2).

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: part-1-post-IME-callbacks-v2.patch

patch 1 v2 fixes Runnables' indentation (thus obsoleting original patch 2).

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: part-2-add-thread-asserts-v2.patch

patch 2 v2 replaces questionable static initialization with mAppContext.getMainLooper().getThread() (and fixes some indentation).

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 637946 [details] [diff] [review]
part-1-post-IME-callbacks-v2.patch

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

::: mobile/android/base/GeckoApp.java
@@ +3190,5 @@
>      }
>  
> +    public static void postToUiThread(Runnable runnable) {
> +        mAppContext.mMainHandler.post(runnable);
> +    }

just talked about this on vidyo, use the function pointed at in the last review

Comment 13

[Chris Peterson [:cpeterson]]

Attachment: part-1-post-IME-callbacks-v3.patch

v3 moves postToUiThread() from GeckoApp to private utility method of GeckoInputConnection.

Comment 14

[Chris Peterson [:cpeterson]]

Attachment: part-2-add-thread-asserts-v3.patch

part 2 v3 adds GeckoApp changes no longer included in patch part 1 v3.

Comment 15

[Scoobidiver (away)]

Crashes that depends on this bug are #19 top crasher in 14.0.

Comment 16

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9ad5793fbba
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad4fd5dcc088

Comment 17

[Chris Peterson [:cpeterson]]

Comment on attachment 638046 [details] [diff] [review]
part-1-post-IME-callbacks-v3.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Intermittent text input problems from a Gecko/IME race condition.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): This is a medium risky change for Android IME, but it fixes a race condition that caused many text input bugs. I think we should fix this in Firefox 15, but it is too risky for 14.0.1.
String or UUID changes made by this patch:

Comment 18

[Chris Peterson [:cpeterson]]

Comment on attachment 638050 [details] [diff] [review]
part-2-add-thread-asserts-v3.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Intermittent text input problems from a Gecko/IME race condition.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): This is a medium risky change for Android IME, but it fixes a race condition that caused many text input bugs. I think we should fix this in Firefox 15, but it is too risky for 14.0.1.
String or UUID changes made by this patch: None

Comment 19

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/mozilla-central/rev/6ffe01582f9d
https://hg.mozilla.org/mozilla-central/rev/93e9bfe61602

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b9ad5793fbba
https://hg.mozilla.org/mozilla-central/rev/ad4fd5dcc088

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 638046 [details] [diff] [review]
part-1-post-IME-callbacks-v3.patch

[Triage Comment]
Bug 751513 is the best reason to move forward with a medium risk fix here. I'm tempted to let this bake on Nightly longer before uplifting, but would actually rather we had this sooner on Aurora 15 given next week's Beta uplift. Let's collect as much feedback as possible.

Comment 22

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8b9b2a2465a9
https://hg.mozilla.org/releases/mozilla-aurora/rev/4680c335d0ad

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

We're going to need to back this out of mozilla-central today and, after checking with QA in the morning, also backout of branches and mozilla-release for our mobile 15.0.1 respin.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

please adjust status flags once this is done.  We'll have to look into a different fix for this to m-c and uplifting where possible.

Comment 25

[Chris Peterson [:cpeterson]]

Backed out from m-c:
https://hg.mozilla.org/mozilla-central/rev/3974efe8d584
https://hg.mozilla.org/mozilla-central/rev/1f1e5cdae68d

Comment 26

[Scoobidiver (away)]

Backout from Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/6210771c474b
http://hg.mozilla.org/releases/mozilla-aurora/rev/53bcbf4def76

Backout from Beta:
http://hg.mozilla.org/releases/mozilla-beta/rev/dd8f72317c3f
http://hg.mozilla.org/releases/mozilla-beta/rev/c43988d1b0b7

Backout from Release:
http://hg.mozilla.org/releases/mozilla-release/rev/5b80d834dff7
http://hg.mozilla.org/releases/mozilla-release/rev/d90936ea0e2e

Comment 27

[Alex Keybl [:akeybl]]

Let's leave FF16 as is (and as FF15 was), which means this no longer needs to be tracked. IME changes late in the game are too high risk.

Comment 28

[Marc Diethelm]

So probably no chance this will be backported (to Fx16 at least), right? I really think 770291 which is blocked by this bug is critical.