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

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/jimnchen@gmail.com-0c65746ad523/

this build works for me

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

Comment on attachment 478504 [details] [diff] [review] Patch v1 I'm sorry, I'm not familiar with the e10s...

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.

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

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 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?

(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 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 ;).

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 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.

(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 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 :).

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

pushed http://hg.mozilla.org/mozilla-central/rev/ba32daebb14a this still needs to land on the b1 rel branch though

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

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

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?

New bug please.