Last Comment Bug 617298 - Text input causes SwiftKey keyboard to force close
: Text input causes SwiftKey keyboard to force close
Status: VERIFIED FIXED
[vkb]
: relnote
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: ARM Android
: P1 major with 2 votes (vote)
: Firefox 5
Assigned To: Michael Wu [:mwu]
:
Mentors:
: 628302 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-07 04:28 PST by Brian Carpenter [:geeknik]
Modified: 2011-07-19 15:10 PDT (History)
20 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Log file (39.16 KB, text/plain)
2010-12-07 13:26 PST, Brian Carpenter [:geeknik]
no flags Details
Log File 01.25.11 (158.13 KB, text/plain)
2011-01-25 10:22 PST, Brian Carpenter [:geeknik]
no flags Details
Logfile 01.25.11 w/ new SwiftKey Beta (157.69 KB, text/plain)
2011-01-25 16:20 PST, Brian Carpenter [:geeknik]
no flags Details
patch (822 bytes, patch)
2011-04-06 12:01 PDT, Brad Lassey [:blassey] (use needinfo?)
mwu.code: review-
Details | Diff | Review
Bandaid over unreliable text extraction from child process (1.77 KB, patch)
2011-04-14 16:20 PDT, Michael Wu [:mwu]
nchen: review+
Details | Diff | Review
alternative patch (4.96 KB, patch)
2011-04-15 11:19 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Review
alternative patch v2 (5.59 KB, patch)
2011-04-15 11:27 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Review
alternative patch v3 (7.01 KB, patch)
2011-04-15 12:47 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Review
Bandaid over unreliable text extraction from child process, v2 (1.77 KB, patch)
2011-04-28 17:33 PDT, Michael Wu [:mwu]
mbrubeck: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description Brian Carpenter [:geeknik] 2010-12-07 04:28:12 PST
Any text input with the latest nightly Fennec build using the SwiftKey keyboard causes the keyboard to force close. I can get 2-3 characters out before this occurs. I will attach logfiles as soon as I can.
Comment 1 Brian Carpenter [:geeknik] 2010-12-07 13:26:21 PST
Created attachment 495943 [details]
Log file

This log file was grabbed right after SwiftKey crashed while using Fennec. I tried to grab the /data/data/com.touchtype.swiftkey/files/1.0.951-52907.stacktrace which was referenced in the log, but was told it didn't exist.
Comment 2 Tony Chung [:tchung] 2010-12-20 10:04:46 PST
How do we want to handle external android keyboards in fennec?   I'm assuming this works fine against the android browser?
Comment 3 Brian Carpenter [:geeknik] 2010-12-21 16:41:17 PST
With the latest version of SwiftKey and the latest nightly build of Firefox Mobile 4.0b4pre, I'm not seeing a crash any more. Closing as WORKSFORME.
Comment 4 Brian Carpenter [:geeknik] 2010-12-21 16:46:47 PST
I spoke too soon, just as I was closing this and typing something into the Google search box, SwiftKey force closed. I re-opened the browser and chose Swype instead, and this keyboard works fine with Firefox Mobile. Typing in the location bar is fine, no force close, typing in the search box @ google.com though, does cause a force close.
Comment 5 Christian Grasser 2011-01-03 01:29:45 PST
Same to me with SwiftKey.
Sometimes it works and sometimes its crashing.

Please let me know if i could help. I use Beta 4 from the Android Market on the Samsung Galaxy Tab ! Firmware 2.2
Comment 6 Tony Chung [:tchung] 2011-01-24 12:52:35 PST
*** Bug 628302 has been marked as a duplicate of this bug. ***
Comment 7 Tony Chung [:tchung] 2011-01-24 13:34:18 PST
Despite http://www.swiftkey.net/blog/?p=148, it sounds like there are still reports of swiftkey and fennec having compatibility issues today.   Flagging for blocking radar as it affects users.

Related reports:
- http://support.mozilla.com/sr-CYRL/questions/757791
- http://groups.google.com/group/mozilla.feedback.firefox.mobile.prerelease/browse_thread/thread/2ebe0c0ecaaf4774
- http://support.mozilla.com/en-US/questions/758463
- bug 598617
Comment 8 Brian Carpenter [:geeknik] 2011-01-24 17:00:08 PST
Once again I thought that this had fixed itself, however, using the latest nightly build of Fennec and the latest market version of Swiftkey, this problem is still crashing the keyboard. I have access to the latest beta build of Swiftkey and will report back if it works.
Comment 9 Tony Chung [:tchung] 2011-01-25 07:49:51 PST
brian, can you attach a more recent logfile during the crash?   Thanks.
Comment 10 Brian Carpenter [:geeknik] 2011-01-25 10:22:46 PST
Created attachment 506791 [details]
Log File 01.25.11

Okay, this is a logfile generated by the Android SendLog app. I tried to update Fennec already installed, but Fennec started Force Closing, so I just uninstalled and reinstalled the latest nightly build (Mozilla/5.0 (Android; Linux armv7l; rv:2.0b10pre) Gecko/20110125 Firefox/4.0b10pre Fennec/4.0b4pre. Swiftkey is the latest market version, v1.1.91.
Comment 11 Brian Carpenter [:geeknik] 2011-01-25 10:30:25 PST
The log file references Stacktrace in file '/data/data/com.touchtype.swiftkey/files/1.1.91-58713.stacktrace' belongs to version 1.1.91, however, by the time I got there, the directory was empty.
Comment 12 Tony Chung [:tchung] 2011-01-25 15:55:22 PST
adding relnote since this users are seeing this
Comment 13 Brian Carpenter [:geeknik] 2011-01-25 16:20:11 PST
Created attachment 506947 [details]
Logfile 01.25.11 w/ new SwiftKey Beta

Okay, I've downloaded the SwiftKey Beta v1.98.1.172 from their VIP forum, and it still crashes with Fennect. Log file attached.
Comment 14 Brian Crowder 2011-02-10 14:20:02 PST
Pretty sure this is a SwiftKey bug, so I'm not sure how we can block on it.
Comment 15 Tony Chung [:tchung] 2011-02-10 15:01:24 PST
if you are certain this is external, we'll need to hand this over to tech evangelism or partners.  Do we have someone that deals with mobile applications?   cc'ing kev and blizzard

More searches on feedback indicates its an issue worth following up with.  Some of these already have comments by mbrubeck:

- http://input.mozilla.com/en-US/beta/search?product=mobile&version=&date_start=&date_end=&q=swiftkey
- http://support.mozilla.com/te/questions/781301
- http://support.mozilla.com/en-US/questions/781093
- https://support.mozilla.com/en-US/questions/781272
Comment 16 Brian Crowder 2011-02-10 16:24:25 PST
I had an email from one of the SwiftKey devs who is going to be looking at it soon, I hope.  They have multiple reports regarding it in their database.  The Java stack for the SwiftKey crash doesn't even include us, so while something we're doing may be causing trouble for them (and indeed may even be non-standard in some way), I don't think we'll be able to track it down without their help.  For now, at least, the ball is pretty solidly in their court.
Comment 17 Brian Carpenter [:geeknik] 2011-02-10 17:00:03 PST
Text input isn't even working in the latest Fennec nightly. I can load Google.com and select the search field, but nothing I type in Swiftkey even shows up. Switching to the stock keyboard and Swype work just fine. If I remember correctly, text input worked briefly sometime between 1/25 and I'd say 2/1 maybe 2/5. I haven't had time to keep up with testing on the mobile side, but if someone can link an archive to all of the Fennec nightly builds, I'll spend some time this weekend looking for builds over the last 2 weeks that work and don't work and then maybe we can narrow down the changes that might be conflicting with the keyboard.
Comment 18 Matt Brubeck (:mbrubeck) 2011-02-10 17:11:09 PST
(In reply to comment #17)
> but if someone can link an archive to all of the Fennec nightly builds, I'll
> spend some time this weekend looking for builds over the last 2 weeks that work
> and don't work and then maybe we can narrow down the changes that might be
> conflicting with the keyboard.

You'll want to use the mozilla-central-android-r7 builds from here:
http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/

This shell script may be helpful for quickly donwloading nightlies for specific dates (requires the "adb" tool from the Android SDK):
http://www.lassey.us/getnightly.sh
Comment 19 Brian Crowder 2011-02-22 14:09:51 PST
Tried this again and SwiftKey is still crashing, though less frequently.  Pinged SwiftKey on their support page again.
Comment 20 Brian Crowder 2011-02-22 15:19:26 PST
I don't think we can or should block on a crack in SwiftKey, esp. considering how difficult they seem to be to talk with.
Comment 21 Paul Butcher 2011-02-23 09:23:33 PST
All,

I'm Paul Butcher - Chief Software Architect here at TouchType (makers of SwiftKey). We're aware of this issue, and it is on our list of things to look at. Unfortunately it's on a very long list of things to look at (which hasn't been helped by the interest generated by Mobile World Congress last week).

We will get to this, I promise, but I can't give you a date right at the moment.

Sorry that I can't be more definitive.
Comment 22 Matt Brubeck (:mbrubeck) 2011-02-23 09:27:27 PST
Thanks, Paul!  If we can help by providing reduced test cases or other information, let us know.
Comment 23 Brad Lassey [:blassey] (use needinfo?) 2011-04-06 12:01:44 PDT
Created attachment 524228 [details] [diff] [review]
patch
Comment 24 Michael Wu [:mwu] 2011-04-14 14:05:26 PDT
Comment on attachment 524228 [details] [diff] [review]
patch

r- for not actually working. swiftkey still crashes here.
Comment 25 Michael Wu [:mwu] 2011-04-14 16:20:24 PDT
Created attachment 526144 [details] [diff] [review]
Bandaid over unreliable text extraction from child process

So, certain operations requires synchronization. We have an IME specific synchronization mechanism between Java and Gecko, but there is no synchronization between the parent and child process. This patch has the code wait and hope that the child process updates the parent process before the end of 20ms so that NS_QUERY_TEXT_CONTENT/mIMECacheText is accurate. I wish there were some way to ensure that mIMECacheText has been updated recently..
Comment 26 Brad Lassey [:blassey] (use needinfo?) 2011-04-14 16:38:21 PDT
that's working for me
Comment 27 Michael Wu [:mwu] 2011-04-14 16:48:52 PDT
(In reply to comment #26)
> that's working for me

I just tested it again and it failed after typing 4 characters on google.com. Tested on a trunk build with a copy of the swiftkey trial I downloaded this morning on my N1.
Comment 28 Brad Lassey [:blassey] (use needinfo?) 2011-04-15 11:19:00 PDT
Created attachment 526317 [details] [diff] [review]
alternative patch

this patch waits for the child process to catch up
Comment 29 Brad Lassey [:blassey] (use needinfo?) 2011-04-15 11:27:41 PDT
Created attachment 526319 [details] [diff] [review]
alternative patch v2

might as well use this for selection as well
Comment 30 Doug Turner (:dougt) 2011-04-15 12:08:00 PDT
Comment on attachment 526319 [details] [diff] [review]
alternative patch v2

Needs docs.  look at the comments about |seqno| to see what I am after.

> 
>+    async AckTextEvent(PRUint32 seqno);
>+



>+{
>+  if (seqno > mAckedIMESeqno)
>+    mAckedIMESeqno = seqno;
>+  return true;

Worry about overflow?



>     case AndroidGeckoEvent::IME_GET_TEXT:
>         {
>+            if (XRE_GetProcessType() == GeckoProcessType_Default &&
>+                mozilla::dom::TabParent::GetIMETabParent() &&
>+                !mozilla::dom::TabParent::GetIMETabParent()->AllSeqNosAcked()) {
>+                nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
>+                usleep(5000);
>+                return;
>+            }



I don't understand the usleep() here.


Also, won't this cause events to get out over order?
Comment 31 JP Rosevear [:jpr] 2011-04-15 12:24:26 PDT
Not a regression from FF4, this can land on m-c when there is a reviewed patch.  If its important enough it can be re-nomed or be requested for approval when there is a reviewed patch.
Comment 32 Brad Lassey [:blassey] (use needinfo?) 2011-04-15 12:25:14 PDT
(In reply to comment #30)

> >+{
> >+  if (seqno > mAckedIMESeqno)
> >+    mAckedIMESeqno = seqno;
> >+  return true;
> 
> Worry about overflow?
nope, seqno would have to overflow first

> >     case AndroidGeckoEvent::IME_GET_TEXT:
> >         {
> >+            if (XRE_GetProcessType() == GeckoProcessType_Default &&
> >+                mozilla::dom::TabParent::GetIMETabParent() &&
> >+                !mozilla::dom::TabParent::GetIMETabParent()->AllSeqNosAcked()) {
> >+                nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
> >+                usleep(5000);
> >+                return;
> >+            }
> 
> 
> 
> I don't understand the usleep() here.
just want to give the content process time to run, sched_yield works as well in my testing if you prefer that
> 
> Also, won't this cause events to get out over order?
the java main thread is blocking until we call ReturnIMEQueryResult, so we shouldn't get anymore IME events until this gets processed. I can add some code to protect against that.
Comment 33 Brad Lassey [:blassey] (use needinfo?) 2011-04-15 12:47:06 PDT
Created attachment 526345 [details] [diff] [review]
alternative patch v3
Comment 34 Jim Chen [:jchen] [:darchons] 2011-04-15 13:26:11 PDT
Comment on attachment 526144 [details] [diff] [review]
Bandaid over unreliable text extraction from child process

Canceling mwu's review request assuming we are going with blassey's patch.
Comment 35 Doug Turner (:dougt) 2011-04-15 13:46:09 PDT
Comment on attachment 526345 [details] [diff] [review]
alternative patch v3



comment needed above here.
> 
>+    async AckTextEvent(PRUint32 seqno);
>+
>     /** 
>      * Starts an offline application cache update.

You can probably drop this comment and just more fully document |AckTextEvent|.


>+    AndroidGeckoEvent(AndroidGeckoEvent* ae):
>+        mAction(ae->mAction), mType(ae->mType), mTime(ae->mTime), mP0(ae->mP0),
>+        mP1(ae->mP1), mRect(ae->mRect), mFlags(ae->mFlags), mMetaState(ae->mMetaState),
>+        mKeyCode(ae->mKeyCode), mUnicodeChar(ae->mUnicodeChar), mOffset(ae->mOffset),
>+        mCount(ae->mCount), mRangeType(ae->mRangeType), mRangeStyles(ae->mRangeStyles),
>+        mRangeForeColor(ae->mRangeForeColor), mRangeBackColor(ae->mRangeBackColor),
>+        mX(ae->mX), mY(ae->mY), mZ(ae->mZ), mCharacters(ae->mCharacters),
>+        mGeoPosition(ae->mGeoPosition), mGeoAddress(ae->mGeoAddress)
>+    {
>+    }

is there a better way, like a copy constructor or some such thing?


>@@ -1572,6 +1575,14 @@ nsWindow::OnIMEAddRange(AndroidGeckoEven
> void
> nsWindow::OnIMEEvent(AndroidGeckoEvent *ae)
> {
>+    if (mDelayedEvent) {
>+        if (mDelayedEvent == ae) {
>+            mDelayedEvent = nsnull;
>+        } else {
>+            nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
>+            return;
>+        }
>+    }

I do not understand this change.


>     switch (ae->Action()) {
>     case AndroidGeckoEvent::IME_COMPOSITION_END:
>         {
>@@ -1614,6 +1625,13 @@ nsWindow::OnIMEEvent(AndroidGeckoEvent *
>         return;
>     case AndroidGeckoEvent::IME_GET_TEXT:
>         {
>+            if (XRE_GetProcessType() == GeckoProcessType_Default &&
>+                mozilla::dom::TabParent::GetIMETabParent() &&
>+                !mozilla::dom::TabParent::GetIMETabParent()->AllSeqNosAcked()) {
>+                nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
>+                sched_yield();
>+                return;
>+            }

Not sure what sched_yield() gets you.
Comment 36 Brad Lassey [:blassey] (use needinfo?) 2011-04-15 13:53:17 PDT
> 
> >@@ -1572,6 +1575,14 @@ nsWindow::OnIMEAddRange(AndroidGeckoEven
> > void
> > nsWindow::OnIMEEvent(AndroidGeckoEvent *ae)
> > {
> >+    if (mDelayedEvent) {
> >+        if (mDelayedEvent == ae) {
> >+            mDelayedEvent = nsnull;
> >+        } else {
> >+            nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
> >+            return;
> >+        }
> >+    }
> 
> I do not understand this change.
this is to protect against dougt's concern in comment 32 that IME events might get out of order. The intent here is that is any IME events come in after we've delayed an IME event they'll get shuffled to back of the event queue so they all get processed in the right order.

> 
> 
> >     switch (ae->Action()) {
> >     case AndroidGeckoEvent::IME_COMPOSITION_END:
> >         {
> >@@ -1614,6 +1625,13 @@ nsWindow::OnIMEEvent(AndroidGeckoEvent *
> >         return;
> >     case AndroidGeckoEvent::IME_GET_TEXT:
> >         {
> >+            if (XRE_GetProcessType() == GeckoProcessType_Default &&
> >+                mozilla::dom::TabParent::GetIMETabParent() &&
> >+                !mozilla::dom::TabParent::GetIMETabParent()->AllSeqNosAcked()) {
> >+                nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
> >+                sched_yield();
> >+                return;
> >+            }
> 
> Not sure what sched_yield() gets you.

this was usleep(5000) in the previous patch, I just want to make sure the child process has the opportunity to run.
Comment 37 Jim Chen [:jchen] [:darchons] 2011-04-15 14:26:22 PDT
Comment on attachment 526345 [details] [diff] [review]
alternative patch v3

>+bool TabParent::RecvAckTextEvent(const PRUint32& seqno)
>+{
>+  if (seqno > mAckedIMESeqno)
>+    mAckedIMESeqno = seqno;
>+  return true;
>+}

"if (seqno > mAckedIMESeqno)" not necessary?

>@@ -1614,6 +1625,13 @@ nsWindow::OnIMEEvent(AndroidGeckoEvent *
>         return;
>     case AndroidGeckoEvent::IME_GET_TEXT:
>         {
>+            if (XRE_GetProcessType() == GeckoProcessType_Default &&
>+                mozilla::dom::TabParent::GetIMETabParent() &&
>+                !mozilla::dom::TabParent::GetIMETabParent()->AllSeqNosAcked()) {
>+                nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
>+                sched_yield();
>+                return;
>+            }
>             ALOGIME("IME: IME_GET_TEXT: o=%u, l=%u", ae->Offset(), ae->Count());
> 
>             nsQueryContentEvent event(PR_TRUE, NS_QUERY_TEXT_CONTENT, this);
>@@ -1664,6 +1682,13 @@ nsWindow::OnIMEEvent(AndroidGeckoEvent *
>     case AndroidGeckoEvent::IME_GET_SELECTION:
>         {
>             ALOGIME("IME: IME_GET_SELECTION");
>+            if (XRE_GetProcessType() == GeckoProcessType_Default &&
>+                mozilla::dom::TabParent::GetIMETabParent() &&
>+                !mozilla::dom::TabParent::GetIMETabParent()->AllSeqNosAcked()) {
>+                nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae));
>+                sched_yield();
>+                return;
>+            }
> 

Maybe refactor into one block of code before the switch?

>+
>+    mozilla::AndroidGeckoEvent* mDelayedEvent;
> };
> 

Is mDelayedEvent assigned to somewhere?
Comment 38 Brad Lassey [:blassey] (use needinfo?) 2011-04-15 14:42:07 PDT
thanks for pointing that out, this:
> nsAppShell::gAppShell->PostEvent(new AndroidGeckoEvent(ae)); 
is supposed to be:
> nsAppShell::gAppShell->PostEvent(mDelayedEvent = new AndroidGeckoEvent(ae));

also, swift key is crashing when I hit the delete key with the latest patch, so I think I broke something between v1 and v3
Comment 39 Michael Wu [:mwu] 2011-04-15 16:16:33 PDT
Comment on attachment 526144 [details] [diff] [review]
Bandaid over unreliable text extraction from child process

Given that one change between v1 and v3 of the alternate patch was the removal of a 20ms sleep, and v3 is broken, I don't know if the alternate patch is actually doing any better. A proper synchronization option is welcome, but only if it works. Either way, reviewing this patch is fine. It's a backup in case we want a fix sooner.
Comment 40 Doug Turner (:dougt) 2011-04-15 21:13:17 PDT
i'd much rather not have to add more ipc code in the TabChild/TabParent for IME.
Comment 41 Doug Turner (:dougt) 2011-04-15 21:20:55 PDT
again.....
 
I'd much rather not to have to add more IME handling code in the TabChild/TabParent.  If mwu's patch fixes this ime app, lets do that.
Comment 42 Brad Lassey [:blassey] (use needinfo?) 2011-04-15 23:43:20 PDT
let's review the bandaid patch and get it landed for 4.0.1. We can work on a better fix for a future release.
Comment 43 Jim Chen [:jchen] [:darchons] 2011-04-17 11:10:26 PDT
Comment on attachment 526144 [details] [diff] [review]
Bandaid over unreliable text extraction from child process

>+        // 1. Sleep for 20 milliseconds and hope the child updates us with the new text.
>+        //    Very evil and, consequentially, most effective.
>+        try {
>+            Thread.sleep(20);
>+        } catch (InterruptedException e) {}
>+

For a future followup, we can probably get rid of this by updating the cache on the chrome side, before sending text events to content.

>+            // 2. Use mComposingText instead of what gecko gives us, since this lie works
>+            //    better for swiftkey
>+            if (extract.selectionEnd > extract.text.length())
>+                extract.text = mComposingText;

Do you know how well / why this works? We should return the entire content of the textfield, but mComposingText is only a portion of the whole text.
Comment 44 Kevin Brosnan [:kbrosnan] 2011-04-18 10:06:03 PDT
This missed the 4.0.1 release. Re-nominating for Firefox 5.
Comment 45 Brad Lassey [:blassey] (use needinfo?) 2011-04-18 10:10:27 PDT
no reason to drop this back to a nom
Comment 46 Tony Chung [:tchung] 2011-04-18 10:32:22 PDT
note to self: this patch missed 4.0.1 build 1, but continues to be flagged to blocking-fennec 4.0.1+ to track future respin work.
Comment 47 Michael Wu [:mwu] 2011-04-19 13:41:35 PDT
(In reply to comment #43)
> Comment on attachment 526144 [details] [diff] [review]
> Bandaid over unreliable text extraction from child process
> 
> >+        // 1. Sleep for 20 milliseconds and hope the child updates us with the new text.
> >+        //    Very evil and, consequentially, most effective.
> >+        try {
> >+            Thread.sleep(20);
> >+        } catch (InterruptedException e) {}
> >+
> 
> For a future followup, we can probably get rid of this by updating the cache on
> the chrome side, before sending text events to content.
> 

I'll file it.

> >+            // 2. Use mComposingText instead of what gecko gives us, since this lie works
> >+            //    better for swiftkey
> >+            if (extract.selectionEnd > extract.text.length())
> >+                extract.text = mComposingText;
> 
> Do you know how well / why this works? We should return the entire content of
> the textfield, but mComposingText is only a portion of the whole text.

Swiftkey wants to know what the current word is, and if the info from gecko is wrong, we'd rather have the current word be accurate than pass info about all the text in the field.
Comment 48 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-20 14:18:40 PDT
This is pretty ridiculous.  There's really still no word about what's wrong on swiftkey's end?

I'm not familiar with the Java code being patched here but a 20ms sleep on an input-event path seems like a potentially noticeable lag.  Can we somehow detect swiftkey and only do these hacks?  Or better yet, blacklist swiftkey somehow?
Comment 49 Michael Wu [:mwu] 2011-04-20 14:48:42 PDT
(In reply to comment #48)
> This is pretty ridiculous.  There's really still no word about what's wrong on
> swiftkey's end?
> 

Oh I know what's wrong. We're giving them some text from the field and two indexes indicating where the selection starts and ends. Under certain circumstances, the parent process will not give the right text for the field and we end up giving the IME some text with an out of bound selection. Swiftkey dies because it doesn't bounds check.

> I'm not familiar with the Java code being patched here but a 20ms sleep on an
> input-event path seems like a potentially noticeable lag.  Can we somehow
> detect swiftkey and only do these hacks?  Or better yet, blacklist swiftkey
> somehow?

I've tested it and noticed no difference. I suspect this will only be noticeable if you hit the next key within 20ms since this call is executed after every key entry instead of before. The gecko thread itself isn't blocked.
Comment 50 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-20 14:58:06 PDT
(In reply to comment #49)
> Oh I know what's wrong. We're giving them some text from the field and two
> indexes indicating where the selection starts and ends. Under certain
> circumstances, the parent process will not give the right text for the field
> and we end up giving the IME some text with an out of bound selection. Swiftkey
> dies because it doesn't bounds check.

Wait --- the cached text and indices in the chrome process should always be consistent, even if they're slightly out-of-date compared to what's in the content process.  Or is there some other problem?

> 
> > I'm not familiar with the Java code being patched here but a 20ms sleep on an
> > input-event path seems like a potentially noticeable lag.  Can we somehow
> > detect swiftkey and only do these hacks?  Or better yet, blacklist swiftkey
> > somehow?
> 
> I've tested it and noticed no difference. I suspect this will only be
> noticeable if you hit the next key within 20ms since this call is executed
> after every key entry instead of before. The gecko thread itself isn't blocked.

That's the case I had in mind, successive events hitting this code.  This hack would allow fast typists to build up an unbounded UI delay.  That sounds bad.

If there's really truly no other way to fix or work around this, can we please only do the hacks for swiftkey?
Comment 51 Michael Wu [:mwu] 2011-04-20 15:27:05 PDT
(In reply to comment #50)
> (In reply to comment #49)
> > Oh I know what's wrong. We're giving them some text from the field and two
> > indexes indicating where the selection starts and ends. Under certain
> > circumstances, the parent process will not give the right text for the field
> > and we end up giving the IME some text with an out of bound selection. Swiftkey
> > dies because it doesn't bounds check.
> 
> Wait --- the cached text and indices in the chrome process should always be
> consistent, even if they're slightly out-of-date compared to what's in the
> content process.  Or is there some other problem?
> 

That's the problem - they're inconsistent.

> > 
> > > I'm not familiar with the Java code being patched here but a 20ms sleep on an
> > > input-event path seems like a potentially noticeable lag.  Can we somehow
> > > detect swiftkey and only do these hacks?  Or better yet, blacklist swiftkey
> > > somehow?
> > 
> > I've tested it and noticed no difference. I suspect this will only be
> > noticeable if you hit the next key within 20ms since this call is executed
> > after every key entry instead of before. The gecko thread itself isn't blocked.
> 
> That's the case I had in mind, successive events hitting this code.  This hack
> would allow fast typists to build up an unbounded UI delay.  That sounds bad.
> 

20ms puts an upper limit of 50 characters per second. Lets say the real limit is around 20 characters per second. Wikipedia says "The fastest typing speed ever, 216 words per minute, was achieved by Stella Pajunas-Garnand from Chicago in 1946 in one minute on an IBM electric." Assuming each word is 5 characters, that's 18 characters per second.

20 ms is the simplest fix I can come with. 10ms fails sometimes. Maybe 15ms could work. I don't consider this issue particularly swiftkey specific. I'd like the text reporting to be reasonably accurate for all soft keyboards. Simply fixing the selection indexes causes odd typos on swiftkey. Perhaps there are keyboards which correct the selection indexes and then screw things up.

Ideally, we'd just have the information be consistent. I suspect you or jchen would know how to fix it right.
Comment 52 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-20 15:35:23 PDT
(In reply to comment #51)
> (In reply to comment #50)
> > Wait --- the cached text and indices in the chrome process should always be
> > consistent, even if they're slightly out-of-date compared to what's in the
> > content process.  Or is there some other problem?
> > 
> 
> That's the problem - they're inconsistent.

That just sounds like a plain-jane PuppetWidget bug then.  Why not fix that?

> 20ms puts an upper limit of 50 characters per second. Lets say the real limit
> is around 20 characters per second. Wikipedia says "The fastest typing speed
> ever, 216 words per minute, was achieved by Stella Pajunas-Garnand from Chicago
> in 1946 in one minute on an IBM electric." Assuming each word is 5 characters,
> that's 18 characters per second.

What happens if I'm playing a game and hold down a key for repeated input?  Also, a 20ms pause will cause us to drop frames in 60fps animations/videos on key input.  That sucks.

> Ideally, we'd just have the information be consistent. I suspect you or jchen
> would know how to fix it right.

I'd sure prefer to do that.
Comment 53 Michael Wu [:mwu] 2011-04-20 15:43:42 PDT
(In reply to comment #52)
> (In reply to comment #51)
> > (In reply to comment #50)
> > > Wait --- the cached text and indices in the chrome process should always be
> > > consistent, even if they're slightly out-of-date compared to what's in the
> > > content process.  Or is there some other problem?
> > > 
> > 
> > That's the problem - they're inconsistent.
> 
> That just sounds like a plain-jane PuppetWidget bug then.  Why not fix that?
> 

I don't know how. I also don't like the cache to be inconsistent - I suspect that will cause typos.

> > 20ms puts an upper limit of 50 characters per second. Lets say the real limit
> > is around 20 characters per second. Wikipedia says "The fastest typing speed
> > ever, 216 words per minute, was achieved by Stella Pajunas-Garnand from Chicago
> > in 1946 in one minute on an IBM electric." Assuming each word is 5 characters,
> > that's 18 characters per second.
> 
> What happens if I'm playing a game and hold down a key for repeated input? 

You're generally not using an IME in that case because IMEs can't be counted on to fire keyevents.

> Also, a 20ms pause will cause us to drop frames in 60fps animations/videos on
> key input.  That sucks.
> 

Drawing is on the gecko thread so drawing isn't affected.
Comment 54 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-20 15:45:58 PDT
(In reply to comment #53)
> Drawing is on the gecko thread so drawing isn't affected.

We composite to window on the java main thread though, right?
Comment 55 Michael Wu [:mwu] 2011-04-20 16:20:30 PDT
(In reply to comment #54)
> (In reply to comment #53)
> > Drawing is on the gecko thread so drawing isn't affected.
> 
> We composite to window on the java main thread though, right?

We specifically use the API (SurfaceView) that android provides for doing graphics operations on a separate thread. Ideally such an API would not block on the main thread. The docs describe GLSurfaceView, a subclass of SurfaceView, as rendering "on a dedicated thread to decouple rendering performance from the UI thread."

I changed the 20 ms delay to a 1500 delay to check this assumption and it holds. The caret continues to blink during the delay.
Comment 56 Jim Chen [:jchen] [:darchons] 2011-04-25 22:35:58 PDT
Comment on attachment 526144 [details] [diff] [review]
Bandaid over unreliable text extraction from child process

(In reply to comment #47)
> (In reply to comment #43)
> > Comment on attachment 526144 [details] [diff] [review]
> > Bandaid over unreliable text extraction from child process
> > 
> > >+            // 2. Use mComposingText instead of what gecko gives us, since this lie works
> > >+            //    better for swiftkey
> > >+            if (extract.selectionEnd > extract.text.length())
> > >+                extract.text = mComposingText;
> > 
> > Do you know how well / why this works? We should return the entire content of
> > the textfield, but mComposingText is only a portion of the whole text.
> 
> Swiftkey wants to know what the current word is, and if the info from gecko is
> wrong, we'd rather have the current word be accurate than pass info about all
> the text in the field.

Sorry, I'm still a little wary about this, Maybe something like this?

> if (mComposing && extract.selectionEnd > extract.text.length())
>   extract.text = extract.text.subSequence(0, mCompositionStart) + mComposingText;

This tries to retain everything before the composition. Everything else looks ok to me.
Comment 57 Michael Wu [:mwu] 2011-04-28 16:30:40 PDT
(In reply to comment #56)
> Sorry, I'm still a little wary about this, Maybe something like this?
> 
> > if (mComposing && extract.selectionEnd > extract.text.length())
> >   extract.text = extract.text.subSequence(0, mCompositionStart) + mComposingText;
> 
> This tries to retain everything before the composition. Everything else looks
> ok to me.

Gotcha. I like this better too.
Comment 58 Michael Wu [:mwu] 2011-04-28 17:33:32 PDT
Created attachment 529002 [details] [diff] [review]
Bandaid over unreliable text extraction from child process, v2
Comment 59 Matt Brubeck (:mbrubeck) 2011-04-29 19:28:46 PDT
http://hg.mozilla.org/mozilla-central/rev/e365bff10705
Comment 60 Matt Brubeck (:mbrubeck) 2011-04-29 19:33:45 PDT
Comment on attachment 529002 [details] [diff] [review]
Bandaid over unreliable text extraction from child process, v2

Nominating this patch for Aurora.  This is one of our top 10 user issues on Android [1].  It makes Firefox completely unusable with SwiftKey, one of the most popular Android IMEs.  It was previous triaged as blocking 4.0.1+, though it was dropped from that release because the patch was not ready on time.

The patch touches only Android Java code, so there is zero risk for desktop Firefox.

1. https://wiki.mozilla.org/Mobile/StatusMeetings/04-14-2011#Feedback_Summary

(Carrying r=jchen)
Comment 61 Brad Lassey [:blassey] (use needinfo?) 2011-04-29 19:42:23 PDT
I'll approve this after its baked on trunk for a few days
Comment 62 Michael Wu [:mwu] 2011-04-29 21:11:33 PDT
I've filed bug 653895 for a real fix.
Comment 63 Brad Lassey [:blassey] (use needinfo?) 2011-05-03 10:35:22 PDT
Comment on attachment 529002 [details] [diff] [review]
Bandaid over unreliable text extraction from child process, v2

haven't heard of any issues resulting from this, approval-mozilla-aurora=blassey
Comment 64 Matt Brubeck (:mbrubeck) 2011-05-03 12:59:56 PDT
Pushed to Aurora (Firefox 5).
http://hg.mozilla.org/mozilla-aurora/rev/689679008349

Nominating for tracking-fennec:4.0.2 since it missed 4.0.1.
Comment 65 Andreea Pod 2011-05-05 07:54:46 PDT
Verified fixed on build: Mozilla /5.0 (Android;Linux armv7l;rv:6.0a1) Gecko/20110505 Firefox/6.0a1 Fennec/6.0a1 
Device: LG Optimus 2X
Comment 66 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-05-09 11:47:21 PDT
Verified on Aurora Branch:
Mozilla/5.0 (Android; Linux armv71; rv5.0a2) Gecko/20110509 Firefox/5.0a2 Fennec/5.0a2
Device: Thunderbolt
OS: Android 2.2
Comment 67 Samat Jain 2011-05-13 16:15:58 PDT
Could this be backported to a Firefox 4 Mobile minor release?
Comment 68 Matt Brubeck (:mbrubeck) 2011-05-13 16:20:32 PDT
This has approval to be included in a 4.0.2 release if there is one, but there is no such release currently planned.  The next planned update to Firefox is the Firefox 5 release next month.
Comment 69 Chetan 2011-05-25 07:36:27 PDT
Hi guys,

We just tested Firefox and Firefox Beta from the Android market and with the new SwiftKey Beta (not yet available to the world). It seems to work well apart from two things. On >= Android 2.3 we use InputConnection.setComposingRegion to set the current word being edited (e.g. after backspacing) but Firefox ignores it! On < 2.3 we delete the characters and re-insert the text to mimic this behaviour, but this seems to cause Firefox to get stuck in a loop of entering and removing the text.

It seems like Firefox has its own implementation of InputConnection. Can you update your implementation to resolve these issues?

Regards,
Chetan. (Software Engineer @ TouchType)
Comment 70 Brad Lassey [:blassey] (use needinfo?) 2011-05-25 07:47:30 PDT
(In reply to comment #69)
> Hi guys,
> 
> We just tested Firefox and Firefox Beta from the Android market and with the
> new SwiftKey Beta (not yet available to the world). It seems to work well
> apart from two things. On >= Android 2.3 we use
> InputConnection.setComposingRegion to set the current word being edited
> (e.g. after backspacing) but Firefox ignores it! On < 2.3 we delete the
> characters and re-insert the text to mimic this behaviour, but this seems to
> cause Firefox to get stuck in a loop of entering and removing the text.
> 
> It seems like Firefox has its own implementation of InputConnection. Can you
> update your implementation to resolve these issues?
> 
> Regards,
> Chetan. (Software Engineer @ TouchType)

The change to make isn't obvious to me, can you provide a patch to illustrate it? Our implementation is here https://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoInputConnection.java
Comment 71 Chetan 2011-05-25 08:14:25 PDT
Currently Firefox builds with Android API level 5 (see: https://mxr.mozilla.org/mozilla-central/source/embedding/android/AndroidManifest.xml.in). InputConnection.setComposingRegion was introduced in API level 9.

Firefox must be compiled against API >= 9 and https://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoInputConnection.java must override and implement setComposingRegion. See http://developer.android.com/reference/android/view/inputmethod/BaseInputConnection.html#setComposingRegion(int,%20int) for documentation.
(BaseInputConnection already implements this but Firefox must override this implementation to pass the event to Gecko.)

Regards,
Chetan.
Comment 72 Chetan 2011-07-19 03:21:55 PDT
I have tested this with the latest SwiftKey X (released last week) with Firefox 6.0 Beta and the problem still exists. It is reproducible on any >= Android 2.3 device. (User video of issue here: http://www.youtube.com/watch?v=n-DWDFPjiuM)
Please note: All SwiftKey users get a free upgrade to SwiftKey X.

Please reopen this bug!
Comment 73 Josh Matthews [:jdm] 2011-07-19 10:10:47 PDT
Chetan, thanks for the detailed video showing the problems you're experiencing. However, it shows different symptoms than the ones that this bug report was addressing. Would you mind filing a new bug for your report, so we can keep the issues and fixes logically separate?
Comment 74 Matt Brubeck (:mbrubeck) 2011-07-19 15:10:20 PDT
I've started testing Swiftkey X, and I've filed bug 672661 for one problem I found.  I'll continue filing bugs as I reproduce more problems.

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