599550 - If I type too fast characters get inserted in the wrong order

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Stuart Parmenter]

I see this more often in password fields than I do in content, but can reproduce it there as well

Comment 1

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

Attachment: Patch v1

Patch that should fix the bug; not sure who to ask for review?

Try builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-0c65746ad523/

Comment 2

[Stuart Parmenter]

this build works for me

Comment 3

[Doug Turner (:dougt)]

Masayuki, can you take a look at this ime/e10s bug?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 478504 [details] [diff] [review]
Patch v1

I'm sorry, I'm not familiar with the e10s...

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

With this patch, I noticed something of the opposite if I *delete* too fast --- the caret jumps to the beginning of the input, and subsequent deletes are dropped.  I can repro this reliably on a google results page.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

And I can't repro on http://people.mozilla.com/~cjones/input.html.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Jim, I think the next reviewer in line for this patch is roc, but he's gone for too long.  Next after him would probably be blassey or me.  If blassey passes, I'd be happy to review if you could point me at some basic documentation of how the IME events are supposed to work.  (I think I mostly understand the synchronization problems async IME presents, but I don't know what view of input state we need to present to code consuming the events being dispatched.)

Comment 8

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

Comment on attachment 478504 [details] [diff] [review]
Patch v1

Chris, I can take this review if you don't mind making sure all the e10s related bits are right.

Jim, a couple questions:
What is the consequence of dropping these events?

Under what circumstances would the syncIds get out of sync? Are the events actually coming out of order? If so, that seems bad in general and maybe we should save them until they can be processed in the correct order.

If the id's do get out of sync, how do they get back in sync?

Comment 9

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

(In reply to comment #8)
> Comment on attachment 478504 [details] [diff] [review]
> Patch v1
> 
> Jim, a couple questions:
> What is the consequence of dropping these events?
> 
There shouldn't be any negative consequence. If we detect a notification is out-of-date, it means an up-to-date notification is on its way, and the right choice is to discard the out-of-date one.

> Under what circumstances would the syncIds get out of sync? Are the events
> actually coming out of order? If so, that seems bad in general and maybe we
> should save them until they can be processed in the correct order.
> 
The events are in the right order, but the notifications are not matched up to the events.

In roc's words (bug 591047 #50), "So the basic problem in part 7 is that the chrome's idea of what the current selection is could be obsolete. In particular, chrome could send a message that assumes a particular selection state S1, and that message can cross in-flight with a message from content to chrome telling it that *content* has just updated the selection."

So we prevent this situation by simulating where the selection is on the chrome side, and discarding out-of-date notifications by detecting syncId

> If the id's do get out of sync, how do they get back in sync?

The id's get out of sync when we are not getting the latest notification, so all we have to do is to discard until we do get the latest notification.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 478504 [details] [diff] [review]
Patch v1

ere's a fair amount here I don't get, but I think I get the gist.

Terminology note: in these kinds of algorithms, what you call the
"sync ID" is usually called a "generation counter" (also "sequence
number" in networking stuff).  For convention's sake, I'd recommend
s/syncId/generation/, so e.g. |mIME*Generation|.

There are two big synchronization issues with this patch (I think).
The first is that it's vulnerable to what's called the A/B/A problem.
When you reset the TabParent's generation counter in
RecvNotifyIMEFocus, you might have outstanding chrome-->content IME
messages tagged with the old generation counter.  So the content
process cam handle an IME event destined for the previously-focused
target, reply to chrome, and then chrome validate the reply because
the generation counter just happens to be the same.  This seems like
it can cause bad things to happen in chrome.

The second problem is that content needs to be able to toss out stale
events from chrome, (I think) after each NotifyIMEFocus().  This means
that PuppetWidget probably needs its own mIMEFocusGeneration counter
that it stamps on messages sent to chrome.  Then both processes keep
local copies of the other's generation counters, and throw out events
when expected values aren't present.

Also you need comments that describe hand-wavily-formally the
definition of a valid, consistent IME event on both sides in terms of
the generation counters you'll keep.

Let me know if I've misunderstood, and your patch handles this.  It
wasn't obvious to me.  If it does, then I think you need more comments
;).

Comment 11

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

Attachment: Patch v2

Addressed cjones's comments here and over IRC:

> Terminology note: in these kinds of algorithms, what you call the
> "sync ID" is usually called a "generation counter" (also "sequence
> number" in networking stuff).  For convention's sake, I'd recommend
> s/syncId/generation/, so e.g. |mIME*Generation|.

Renamed syncId to seqNo

> The first is that it's vulnerable to what's called the A/B/A problem.

No longer reset counter on Focus(). Should prevent counter out-of-sync condition

> The second problem is that content needs to be able to toss out stale
> events from chrome, (I think) after each NotifyIMEFocus(). 

Content now compares counter values on Focus(), calculates the difference, and discards that number of events so content catches up with chrome.
* It's ok to discard IME events because the focus is shifting away and there's nothing more we can do
* This prevents events targeted at old focused element from being sent to new focused element

This should only be needed when content shifts focus (script, etc.). When focus changes through mouse or key, the IME IPC messages will get processed before the mouse and key messages.

> Also you need comments that describe hand-wavily-formally the
> definition of a valid, consistent IME event on both sides in terms of
> the generation counters you'll keep.

Added some more comments (might still need more?)

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 479704 [details] [diff] [review]
Patch v2

Purely a style thing, but |seqno| is conventional and perfectly fine
with me; |seqNo| doesn't help anything IMHO.

>diff --git a/widget/src/xpwidgets/PuppetWidget.h b/widget/src/xpwidgets/PuppetWidget.h
>--- a/widget/src/xpwidgets/PuppetWidget.h
>+++ b/widget/src/xpwidgets/PuppetWidget.h
>@@ -211,15 +219,16 @@ private:
>   PRPackedBool mEnabled;
>   PRPackedBool mVisible;
>   // XXX/cjones: keeping this around until we teach LayerManager to do
>   // retained-content-only transactions
>   nsRefPtr<gfxASurface> mSurface;
>   // IME
>   nsIMEUpdatePreference mIMEPreference;
>   PRPackedBool mIMEComposing;
>-  PRPackedBool mIMESuppressNotifySel;
>+  PRUint32 mIMESeqNo;
>+  PRUint32 mIMEEventDiscardCount;

The name |mIMESeqno| here is a bit confusing, IMHO; I think something
like |mIMELastSeenSeqno| makes it clearer that its value can be
different from the chrome process's seqno.

I think that |mIMEEventDiscardCount| is going to be error prone.
Instead, how about |mIMEOnFocusSeqno|, with the equivalent "stale
event" check |event.seqno < mIMEOnFocusSeqno|.

This patch looks OK to me, except for style and maintenance issues.

r- because this patch is causing bad problems on my phone when just
loading google.com: fail-to-load (infinite spinner with no response),
swype keyboard disappearing after focus, swype keyboard not
reappearing on re-focus, crashes.

Comment 14

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

Attachment: Patch v3

(In reply to comment #12)
> Comment on attachment 479704 [details] [diff] [review]
> Patch v2
> 
> Purely a style thing, but |seqno| is conventional and perfectly fine
> with me; |seqNo| doesn't help anything IMHO.
> 
OK fixed.

> The name |mIMESeqno| here is a bit confusing, IMHO; I think something
> like |mIMELastSeenSeqno| makes it clearer that its value can be
> different from the chrome process's seqno.
> 
How about mIMEReceivedSeqno? Added comment too.

> I think that |mIMEEventDiscardCount| is going to be error prone.
> Instead, how about |mIMEOnFocusSeqno|, with the equivalent "stale
> event" check |event.seqno < mIMEOnFocusSeqno|.
> 
Fixed. I used mIMELastBlurSeqno since that's when it's used. Added comment.
This assumes that seqno will not overflow (should be ok on this one since user interaction is relatively slow)

> r- because this patch is causing bad problems on my phone when just
> loading google.com: fail-to-load (infinite spinner with no response),
> swype keyboard disappearing after focus, swype keyboard not
> reappearing on re-focus, crashes.

Hm, google.com/m works on my Droid. I'm seeing the ARGB part of bug 599562 on Google.com (trying to allocate 26MB buffer at once when zoomed in). This code shouldn't cause crashes/load problems in any case, and showing/hiding keyboard is through a different code path than the events involved here.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 479945 [details] [diff] [review]
Patch v3

(In reply to comment #14)
> > The name |mIMESeqno| here is a bit confusing, IMHO; I think something
> > like |mIMELastSeenSeqno| makes it clearer that its value can be
> > different from the chrome process's seqno.
> > 
> How about mIMEReceivedSeqno? Added comment too.
> 

That's better, but I would prefer LastReceivedSeqno.

> > I think that |mIMEEventDiscardCount| is going to be error prone.
> > Instead, how about |mIMEOnFocusSeqno|, with the equivalent "stale
> > event" check |event.seqno < mIMEOnFocusSeqno|.
> > 
> Fixed. I used mIMELastBlurSeqno since that's when it's used. Added comment.
> This assumes that seqno will not overflow (should be ok on this one since user
> interaction is relatively slow)
> 

OK, sounds good.  Yes, if a user kept the same tab open and generated an IME event once every millisecond, it would take about 50 days to overflow the counter.  And if the counter overflowed, it would just cause us to discard events until the next focus/blur.  I can live with that.  In fact, it's worth documenting that.

> > r- because this patch is causing bad problems on my phone when just
> > loading google.com: fail-to-load (infinite spinner with no response),
> > swype keyboard disappearing after focus, swype keyboard not
> > reappearing on re-focus, crashes.
> 
> Hm, google.com/m works on my Droid. I'm seeing the ARGB part of bug 599562 on
> Google.com (trying to allocate 26MB buffer at once when zoomed in). This code
> shouldn't cause crashes/load problems in any case, and showing/hiding keyboard
> is through a different code path than the events involved here.

Yeah, it's working for me now just fine today.  Must have caught a bad cset :S.  (BTW I looked into the transparency issue a bit earlier, and we're using an opaque surface on desktop and initially on mobile, then on mobile something changes and we switch to transparent.  Got sidetracked on JS stuff though.)

Let's get this landed :).

Comment 16

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

Attachment: Patch v4

Changed to LastReceived. Added overflow comment. Carried over r+.

Comment 17

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

pushed http://hg.mozilla.org/mozilla-central/rev/ba32daebb14a

this still needs to land on the b1 rel branch though

Comment 18

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

pushed http://hg.mozilla.org/mozilla-central/rev/e82996480172 to the relbranch

Comment 19

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note : Bug 602013.  Hardware keyboard still misplaces the caret on android.

Comment 20

[Mike Hommey [:glandium]]

This still happens for me with the hardware keyboard on the ASUS Transformer. Should we reopen this bug of should I clone this to a new one?

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

New bug please.