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

RESOLVED DUPLICATE of bug 805162

Status

()

Firefox for Android
Keyboards and IME
RESOLVED DUPLICATE of bug 805162
5 years ago
5 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks: 3 bugs)

14 Branch
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 affected, firefox15- affected, firefox16- affected, firefox17- affected, firefox18- affected)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Attachment #637761 - Flags: review?(blassey.bugs)
(Assignee)

Comment 2

5 years ago
Created attachment 637762 [details] [diff] [review]
part-2-reindent-whitespace.patch

Part 2: Reindent GeckoInputConnection.java. Whitespace changes only!
Attachment #637762 - Flags: review?(blassey.bugs)
(Assignee)

Comment 3

5 years ago
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.
Attachment #637764 - Flags: review?(blassey.bugs)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

5 years ago
This patch fixes bug 768105 and (partially fixes) bug 751513.
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
Attachment #637761 - Flags: review?(blassey.bugs) → review+
Attachment #637762 - Flags: review?(blassey.bugs) → review+
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
Attachment #637761 - Flags: review+ → review-
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();
Attachment #637764 - Flags: review?(blassey.bugs) → review-
(Assignee)

Updated

5 years ago
Blocks: 764025
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
(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).
(Assignee)

Comment 10

5 years ago
Created attachment 637946 [details] [diff] [review]
part-1-post-IME-callbacks-v2.patch

patch 1 v2 fixes Runnables' indentation (thus obsoleting original patch 2).
Attachment #637761 - Attachment is obsolete: true
Attachment #637762 - Attachment is obsolete: true
Attachment #637946 - Flags: review?(blassey.bugs)
(Assignee)

Comment 11

5 years ago
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).
Attachment #637764 - Attachment is obsolete: true
Attachment #637947 - Flags: review?(blassey.bugs)
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
Attachment #637946 - Flags: review?(blassey.bugs) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 638046 [details] [diff] [review]
part-1-post-IME-callbacks-v3.patch

v3 moves postToUiThread() from GeckoApp to private utility method of GeckoInputConnection.
Attachment #637946 - Attachment is obsolete: true
Attachment #638046 - Flags: review?(blassey.bugs)
(Assignee)

Comment 14

5 years ago
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.
Attachment #637947 - Attachment is obsolete: true
Attachment #637947 - Flags: review?(blassey.bugs)
Attachment #638050 - Flags: review?(blassey.bugs)
(Assignee)

Updated

5 years ago
Blocks: 767791

Comment 15

5 years ago
Crashes that depends on this bug are #19 top crasher in 14.0.
tracking-fennec: --- → ?
Attachment #638046 - Flags: review?(blassey.bugs) → review+
Attachment #638050 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9ad5793fbba
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad4fd5dcc088
(Assignee)

Comment 17

5 years ago
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:
Attachment #638046 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 18

5 years ago
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
Attachment #638050 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/mozilla-central/rev/6ffe01582f9d
https://hg.mozilla.org/mozilla-central/rev/93e9bfe61602
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox16: affected → fixed
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 772225
https://hg.mozilla.org/mozilla-central/rev/b9ad5793fbba
https://hg.mozilla.org/mozilla-central/rev/ad4fd5dcc088
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.
Attachment #638046 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #638050 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/8b9b2a2465a9
https://hg.mozilla.org/releases/mozilla-aurora/rev/4680c335d0ad
status-firefox15: affected → fixed
(Assignee)

Updated

5 years ago
tracking-fennec: ? → ---
status-firefox17: --- → unaffected

Updated

5 years ago
Blocks: 747629

Updated

5 years ago
Blocks: 760396
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.
tracking-firefox15: --- → +
tracking-firefox16: --- → +
tracking-firefox17: --- → +
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.
tracking-firefox18: --- → +
(Assignee)

Comment 25

5 years ago
Backed out from m-c:
https://hg.mozilla.org/mozilla-central/rev/3974efe8d584
https://hg.mozilla.org/mozilla-central/rev/1f1e5cdae68d
Status: RESOLVED → REOPENED
status-firefox17: unaffected → affected
Resolution: FIXED → ---

Updated

5 years ago
status-firefox17: affected → fixed
status-firefox18: --- → affected
Target Milestone: Firefox 16 → ---

Comment 26

5 years ago
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
status-firefox15: fixed → affected
status-firefox16: fixed → affected
status-firefox17: fixed → affected
Depends on: 788600
Version: Firefox 16 → Firefox 14
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.
tracking-firefox15: + → -
tracking-firefox16: + → -
tracking-firefox17: + → -
tracking-firefox18: + → -

Updated

5 years ago
No longer blocks: 768105
(Assignee)

Updated

5 years ago
Blocks: 794492
(Assignee)

Updated

5 years ago
Blocks: 790631
(Assignee)

Updated

5 years ago
Blocks: 800370
Blocks: 770291

Comment 28

5 years ago
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.
Depends on: 805162
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 805162
You need to log in before you can comment on or make changes to this bug.