Last Comment Bug 769520 - Fix IME race conditions by ensuring Gecko callbacks post results to UI thread
: Fix IME race conditions by ensuring Gecko callbacks post results to UI thread
Status: RESOLVED DUPLICATE of bug 805162
:
Product: Firefox for Android
Classification: Client Software
Component: Keyboards and IME (show other bugs)
: 14 Branch
: ARM Android
: -- normal with 1 vote (vote)
: ---
Assigned To: Chris Peterson [:cpeterson]
:
Mentors:
Depends on: 788600 805162
Blocks: 760396 767791 800370 747629 751513 764025 770291 772225 790631 794492
  Show dependency treegraph
 
Reported: 2012-06-28 19:10 PDT by Chris Peterson [:cpeterson]
Modified: 2012-11-05 06:52 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
-
affected
-
affected
-
affected
-
affected


Attachments
part-1-post-IME-callbacks-to-UI-thread.patch (10.35 KB, patch)
2012-06-28 19:14 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review-
Details | Diff | Splinter Review
part-2-reindent-whitespace.patch (12.85 KB, patch)
2012-06-28 19:15 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review+
Details | Diff | Splinter Review
part-3-add-thread-asserts.patch (15.67 KB, patch)
2012-06-28 19:21 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review-
Details | Diff | Splinter Review
part-1-post-IME-callbacks-v2.patch (14.31 KB, patch)
2012-06-29 11:19 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review-
Details | Diff | Splinter Review
part-2-add-thread-asserts-v2.patch (19.03 KB, patch)
2012-06-29 11:21 PDT, Chris Peterson [:cpeterson]
no flags Details | Diff | Splinter Review
part-1-post-IME-callbacks-v3.patch (12.53 KB, patch)
2012-06-29 16:15 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
part-2-add-thread-asserts-v3.patch (20.70 KB, patch)
2012-06-29 16:22 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2012-06-28 19:10:16 PDT
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] 2012-06-28 19:14:42 PDT
Created attachment 637761 [details] [diff] [review]
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] 2012-06-28 19:15:22 PDT
Created attachment 637762 [details] [diff] [review]
part-2-reindent-whitespace.patch

Part 2: Reindent GeckoInputConnection.java. Whitespace changes only!
Comment 3 Chris Peterson [:cpeterson] 2012-06-28 19:21:43 PDT
Created attachment 637764 [details] [diff] [review]
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] 2012-06-28 19:24:51 PDT
This patch fixes bug 768105 and (partially fixes) bug 751513.
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-06-28 21:21:10 PDT
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?) 2012-06-28 21:26:46 PDT
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?) 2012-06-28 21:31:29 PDT
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] 2012-06-29 10:41:34 PDT
(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] 2012-06-29 10:46:42 PDT
(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] 2012-06-29 11:19:33 PDT
Created attachment 637946 [details] [diff] [review]
part-1-post-IME-callbacks-v2.patch

patch 1 v2 fixes Runnables' indentation (thus obsoleting original patch 2).
Comment 11 Chris Peterson [:cpeterson] 2012-06-29 11:21:37 PDT
Created attachment 637947 [details] [diff] [review]
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?) 2012-06-29 13:31:11 PDT
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] 2012-06-29 16:15:45 PDT
Created attachment 638046 [details] [diff] [review]
part-1-post-IME-callbacks-v3.patch

v3 moves postToUiThread() from GeckoApp to private utility method of GeckoInputConnection.
Comment 14 Chris Peterson [:cpeterson] 2012-06-29 16:22:07 PDT
Created attachment 638050 [details] [diff] [review]
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) 2012-06-30 00:16:40 PDT
Crashes that depends on this bug are #19 top crasher in 14.0.
Comment 17 Chris Peterson [:cpeterson] 2012-07-09 11:27:35 PDT
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] 2012-07-09 11:29:54 PDT
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 21 Alex Keybl [:akeybl] 2012-07-12 15:07:44 PDT
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 23 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-05 16:02:59 PDT
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 2012-09-05 16:04:01 PDT
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 27 Alex Keybl [:akeybl] 2012-09-17 16:32:22 PDT
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 2012-10-18 02:13:14 PDT
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.
Comment 29 Jim Chen [:jchen] [:darchons] 2012-11-05 06:52:35 PST

*** This bug has been marked as a duplicate of bug 805162 ***

Note You need to log in before you can comment on or make changes to this bug.