1248683 - [e10s] Find bar sometimes gets keypresses in the wrong order when system is under heavy load

Status: NEW

(Toolkit :: Find Toolbar, defect, P3)

Description

[Dave Townsend [:mossop]]

I've hit this a few times. Basically when my system is very loaded (building Firefox in the background) the findbar starts to see keypresses in the wrong order. As an example I just very quickly typed Cmd-f and then "hidden". The text the findbar ended up with was "denhid".

Looking over bug 1218351 I think I have a plausible explanation:

1. Content process is focused and so receives key events.
2. Content process sees "Cmd-f" and sends a message to the parent to start FAYT
3. Content process sees some more keypresses and sends those up to the parent.
4. Main process receives the event to start FAYT, the find bar gets focus and so now all key events go there directly.
5. Main process receives some keypresses and puts them in the find bar.
6. The keypress event from the content process finally arrives with what should have been the first characters for the find bar.

Comment 1

[Jim Mathies [:jimm]]

Hey Dave, how often do you see this?

Comment 2

[Dave Townsend [:mossop]]

Intermittently, maybe just a couple of times a week

Comment 3

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

I'd suspect that you're key presses are not out of order, but instead your cursor position is being reset in the middle of typing.

Comment 4

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #3)
> I'd suspect that you're key presses are not out of order, but instead your
> cursor position is being reset in the middle of typing.

I don't think the cursor changes position, it's always at the end of the word in the findbar's textbox. Bug 1218351 made it so the findbar can recognize characters typed after pressing Ctrl+F but before the findbar is actually focused. So in that example, pressing ctrl+F and typing "hidden", "den" is processed immediately into the findbar because it's already focused, then "hid" is input as if it was typed afterwards because those come from the content's keypress handlers.

I never ran into this scenario before when writing the patch for bug 1218351 though, I wonder if it needs some very specific heavy-load conditions or if I was just very (un)lucky at the time.

Comment 5

[Mike de Boer [:mikedeboer]]

Since we're sending KeyEvents from chrome to content through the message manager, would it make sense to simply add a sequence number to the messages from the sending end?

'b': 1, 'a': 2, 'r': 3.

On the receiving end we can detect if a sequence number is missing from the range

'b': 1, 'r': 3, 'a': 2.

In this example the receiving end would queue the 'r' because we're expecting key no. 2 first. Key number two comes in, thus allowing to (re-)play the keys that were received before.

Just a thought ;-)

Comment 6

[Dave Townsend [:mossop]]

If I understand correctly the process that receives the key event changes halfway through so that wouldn't help. Using timestamps for the events might.

Comment 7

[Luís Miguel [:quicksaver] (not reading bugmail)]

Apologies in advance for writing mostly "in theory mode", but for the foreseeable future I'm stuck in a more consultant-only role with these things. :(

> Since we're sending KeyEvents from chrome to content through the message manager
I assume you meant from content to chrome, otherwise I may be missing something very obvious.

> would it make sense to simply add a sequence number to the messages from the sending end?
I suspect not. Keys that come from content seem to be registered in the right order; notice how it's still "hid" and not any other combination of those characters.

The problem I think comes when intertwining keys from content with keys in chrome (i.e. Ctrl+F, type "hid" before findbar is focused, type "den" after findbar is focused):
- Do content keys *always* come before chrome ones?
- Should they *always* be appended at the beginning of a string?
- How reliably could it keep and reset an internal pointer of where "content keys" should be inserted within the whole find query?
- Not all keys are insertion keys: type "hif", backspace, type "dden"; worst case scenario, the backspace acts on the webpage and goes back in history because the findbar isn't focused yet.
- Same for the arrow keys and enter and other non-char ones because those modifying/handling keys could happen before or after their expected order depending on where they'd fall relative to the focus shift to the findbar's textbox: type "hidden", shift+home, del, type "shown". As long as the shift in focus occurs while typing "hidden", the end-result possibilities here are numerous...

Most importantly, I believe this bug is only "visible" in the findbar because it is the only element (that I know of) that tries to account for lost keys when quickly shifting focus from content to chrome, due to its FindAsYouType.
-- BEGIN NI:mossop
F.i., in theory the same could happen when you press Ctrl+L and quickly type in an address, except in that case you end up with only a partial address with the first typed keys lost, because the location bar doesn't interpret content keys at all. Dave, any chance you can completely redefine your muscle memory and try to hit this behavior with the location bar or something else? To prove me right or wrong at least, however (not) useful that would be. :)
-- END NI:mossop

My main point is, must this be fixed individually per input field (i.e. findbar in this case) considering it may not be so simple to do that (see points above), or if my theory is correct, try to figure out how to ensure (synchronous! [1]) content keys are handled before chrome keys?

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/content/browser-content.js#653

Comment 8

[Dave Townsend [:mossop]]

Unfortunately I haven't really been able to make myself do that reliably enough to say.

Comment 9

[Haik Aftandilian [:haik]]

I was able to reproduce this on OS X 10.11.3 with the OS being heavily loaded and visiting an image heavy website (500px.com.) To load the system, I ran a build while also had 35 of these processes running "gzip -c < /dev/urandom > /dev/null".

To try to learn how this all works, I started tracing KeyboardEvent::KeyboardEvent() calls and noticed that (for a typical key press) they always occur first in the parent and then the child. And that BrowserChild::RecvRealKeyEvent is used which suggests the events flow from chrome to content.

Comment 10

[Jim Mathies [:jimm]]

Mossop, do you have 'accessibility.typeaheadfind' [1] set? It looks like the forwarding of key events only happens [2] if this pref happens to be set.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#4282
[2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#4221

Comment 11

[Dave Townsend [:mossop]]

(In reply to Jim Mathies [:jimm] from comment #10)
> Mossop, do you have 'accessibility.typeaheadfind' [1] set? It looks like the
> forwarding of key events only happens [2] if this pref happens to be set.

No I do not.

Comment 12

[Haik Aftandilian [:haik]]

I can now easily reproduce this by lowering the scheduling priority of the content process and visiting a page with lots of images and reloading or scrolling and then typing Cmd-f following by a search query. On OS X, I use renice(8) to lower the priority of the content process as low as possible.

  $ renice 20 -p `content.pid`

Comment 13

[Haik Aftandilian [:haik]]

When the problem occurs, this is what I see happening. Feedback or tips on understanding this are always appreciated.

In short, the keys entered immediately after cmd-f, but before the findbar input field is receiving input, go to Content via IPDL and then get sent back to Chrome via MessageManager after keys entered later have already been put in the findbar input.

In this example, I typed Cmd-f followed by "abc123", but the find bar ended up with "bc1a23" where 'a' should have been at the beginning of the string.

1) On the Cmd-f, Chrome sends a Cmd-f keypress event to Content by calling TabParent::SendRealKeyEvent. In Content, this is hanlded with TabChild::RecvRealKeyEvent. FindBar.handleEvent (in browser-content.js) receives the Cmd-f and does nothing with that keypress. (At this point, I don't know how we get from TabChild::RecvRealKeyEvent to FindBar.handleEvent. Or, if Chrome requires Content to ignore the Cmd-f before it is handled locally.)

2) Chrome then sends a 'a' (the first character in my find query string) keypress event to Content, but Content is presumably busy and Findbar.handleEvent is not invoked for now. 

3) Then, on the Chrome side, the Cmd-f is handled resulting in the following code from findbar.xml running.

	findbar.onFindCommand()
	findbar.startFind()
	findbar.open()
	findbar._updateBrowserWithState()
	findbar._updateFindUI()
	findbar._updateCaseSensitivity()
	findbar._shouldBeCaseSensitive()
	RemoteFinder: sending Finder:CaseSensitive // to Content
	findbar._updateStatusUI()
	findbar._updateMatchesCount()
	findbar._dispatchFindEvent()
	startFindfindbar.onCurrentSelection()
	findbar._enableFindButtons()
	findbar-textbox.handler(event=focus)
	findbar._onFindFieldFocus()
	
4) Next the 'b' character is received by findbar.xml code. Note the 'b' and later characters in the search string are not sent to Content via SendRealKeyEvent->RecvRealKeyEvent.

	findbar-textbox.handler(event=keypress, this.value=windows)
	  arguments[0].key=b
	findbar-textbox.handler(event=input, this.value=b)
	  calling this.findbar._find(b)
	  
followed by more calls to findbar.xml code building up "bc1" in the findbar input field. This missing 'a' was sent to Content earlier, before onFindCommand was run in Chrome. 

5) Content code finally gets to handle the 'a' keypress event sent earlier. Findbar.handleEvent receives the keypress event for 'a' and uses the MessageManager / RemoteFinder to send a "Findbar:Keypress" message back to Chrome which receives the 'a' and appends it to the findbar input field resulting in "bc1a".

6) Content handles the "Finder:CaseSensitive" message sent in #3 above as well as some other message not listed.

7) The remaining "23" characters are handled in Chrome and appended to "bc1a" to make "bc1a23".

As to a solution, should Chrome know to not send the 'a' to Content because a Cmd-f previously occurred?

Comment 14

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Haik Aftandilian [:haik] from comment #13)
> As to a solution, should Chrome know to not send the 'a' to Content because
> a Cmd-f previously occurred?

I would say not, because chrome has no idea Cmd+f will do something until it actually does it, as happens for pretty much every other shortcut in the browser.

BTW, since you seem to more easily reproduce this, perhaps you can get a better answer to my question in comment 7:
> F.i., in theory the same could happen when you press Ctrl+L and quickly type
> in an address, except in that case you end up with only a partial address
> with the first typed keys lost, because the location bar doesn't interpret
> content keys at all.
In that very same abc123 example, if instead of cmd+f you hit cmd+l to focus the location bar, do you end up with "bc123" only?

Perhaps it's my inexperience in handling more complex issues like these, but I feel an answer there is much needed to know how to tackle the problem, depending on where the underlying problem really is: fix the findbar's jumbled up query, or fix a miscommunication in keypresses between chrome and content.

Comment 15

[Haik Aftandilian [:haik]]

(In reply to Luís Miguel [:quicksaver] from comment #14)
> (In reply to Haik Aftandilian [:haik] from comment #13)
> > As to a solution, should Chrome know to not send the 'a' to Content because
> > a Cmd-f previously occurred?
> 
> I would say not, because chrome has no idea Cmd+f will do something until it
> actually does it, as happens for pretty much every other shortcut in the
> browser.

OK, thanks.

> BTW, since you seem to more easily reproduce this, perhaps you can get a
> better answer to my question in comment 7:
> > F.i., in theory the same could happen when you press Ctrl+L and quickly type
> > in an address, except in that case you end up with only a partial address
> > with the first typed keys lost, because the location bar doesn't interpret
> > content keys at all.
> In that very same abc123 example, if instead of cmd+f you hit cmd+l to focus
> the location bar, do you end up with "bc123" only?

Yes, you're right. After renice'ing the content process, I could reproduce a variant of this with the location-bar.

I reloaded a webpage and, before it was fully loaded, typed Cmd-l followed by "abcd123".

The location bar ended up only with "123". Debug output revealed that the "abcd" got sent to the Content process, but "123" did not.

In the location-bar case, the leading "abcd" characters get lost. The Content process does receive them and send them back to Chrome via "Findbar:Keypress" messages, but since the findbar is not initialized and (in my case) FAYT (find-as-you-type) is not enabled, the keypresses are dropped. This is in tabbrowser.xml.

I tried with FAYT enabled and found that in that case, the "abcd" ended up in the findbar and the findbar input had focus. The URL bar ended up with the "123" and did not have focus. Note, FAYT is not a default setting.

Comment 16

[Luís Miguel [:quicksaver] (not reading bugmail)]

Hmm... I very much doubt a perfect solution will be easy here, because keypresses sent to the content process can't possibly know in time that something in chrome has been focused after the key being was pressed but before it's processed in content and the event is sent back to chrome. (Can it?)

If it was just up to me (and please don't just go by me!! I really don't know all the facts behind all of this), I would go with the solution that causes the least amount of "destruction":

1) Leave as-is, because -technically- it's behaving as expected; keys in chrome are being handled in chrome, and keys in content are being handled in content.
1a) Try to deal with it on a case-by-case. For instance, here worry about the findbar only, which might be the easiest one to address because of all the handlers for FAYT already in place (it's already processing the keys into the findbar, so it's just a matter of putting them in the right order).
1b) Keep in mind the findbar and the location bar are just two examples, there's a world of shortcuts that shift focus elsewhere... Including other windows and dialogs, oh it hurts my head thinking about addressing all separate possible instances...

2) "Repeat" previous keypresses on newly focused chrome nodes if there hasn't been a response from content on those keys yet at the point of the focus shift.
2a) A "keypress nulled" signal to content on these cases might be useless, as when that signal reaches content, the processing for those backlogged keys will likely have finished already. Which is to say, any specific handling for keys in content should be ready on "unhandle" or "undo" or whatever they did... Even just in theory this seems problematic.

3) Maybe "hold" keypresses in a queue in chrome until the last one has finished processing in content, only send down one at a time.
3a) Not sure how that would play with the whole down-up-press event cascade.

4) Increase the priority of keypress handling in content, so that they are less likely to be backlogged like this.
4a) Is this even possible? I know it's way past Christmas, but maybe we could ask Santa in a few months if we behave... Oh wait it's Easter, nearly forgot! Mr. Bunny takes wishes too, right?...

Or maybe I'm just going wildly to the extremes of possibilities, and there's a mild and simple solution somewhere? :)

Comment 17

[Jim Mathies [:jimm]]

(In reply to Haik Aftandilian [:haik] from comment #15)
> > BTW, since you seem to more easily reproduce this, perhaps you can get a
> > better answer to my question in comment 7:
> > > F.i., in theory the same could happen when you press Ctrl+L and quickly type
> > > in an address, except in that case you end up with only a partial address
> > > with the first typed keys lost, because the location bar doesn't interpret
> > > content keys at all.
> > In that very same abc123 example, if instead of cmd+f you hit cmd+l to focus
> > the location bar, do you end up with "bc123" only?
> 
> Yes, you're right. After renice'ing the content process, I could reproduce a
> variant of this with the location-bar.
> 
> I reloaded a webpage and, before it was fully loaded, typed Cmd-l followed
> by "abcd123".
> 
> The location bar ended up only with "123". Debug output revealed that the
> "abcd" got sent to the Content process, but "123" did not.

I would have expected cmd-l to synchronously set focus to the address bar and hence, no lost characters. We should file this as a dependent bug so we can triage it.

For the specific case of the findbar, I took a look at the dom spec [1] for key events hoping to find a sequence or timing value, but don't see anything. 

Masayuki, any ideas here?

[1] https://w3c.github.io/uievents/#events-keyboardevents

Comment 18

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Jim Mathies [:jimm] from comment #17)
> I would have expected cmd-l to synchronously set focus to the address bar
> and hence, no lost characters.

Why expect this for cmd+l and not for cmd+f? Is there perhaps some end-function-related factor I'm not seeing?

Comment 19

[Jim Mathies [:jimm]]

(In reply to Luís Miguel [:quicksaver] from comment #18)
> (In reply to Jim Mathies [:jimm] from comment #17)
> > I would have expected cmd-l to synchronously set focus to the address bar
> > and hence, no lost characters.
> 
> Why expect this for cmd+l and not for cmd+f? Is there perhaps some
> end-function-related factor I'm not seeing?

Haik mentioned a good reason in a discussion we just had, the address bar is always visible and available for input, where as the findbar requires initialization and display before it can accept input.

Comment 20

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Jim Mathies [:jimm] from comment #19)
> Haik mentioned a good reason in a discussion we just had, the address bar is
> always visible and available for input, where as the findbar requires
> initialization and display before it can accept input.

Valid point!

The following is for the sake of argument only, since if whatever fix is applied in this bug can be somehow re-used for other cases, it might be worthwhile at least discussing it in advance.

Cmd+B opens the bookmarks sidebar and focuses its search box. I'm almost sure it will suffer from the same issue here; Haik can probably confirm better than I ever will be able to. How would you look at that from the same "where/how to fix it" perspective? I wouldn't expect that to synchronously open the sidebar and focus it as suggested for the location bar, and it would be significantly harder to fix in the same way as in this bug without tapping into findbar-specific code, or at least reproducing it elsewhere or maybe even generalizing it. (Which would happen in another possible dependent bug of course.)

Comment 21

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

Hmm, sounds like very difficult issue...

I don't think it's good way to add sequential number to KeyboardEvent.

I guess that instead of that, chrome script and C++ code should be able to mark WidgetKeyboardEvent if it may change focus to chrome like "reserved" shortcut key handlers before sending the event to a remote process.

Then, TabParent should stop following keyboard events to send the remote process until the marked shortcut key is back to the chrome process.  If the keyboard event is consumed by the content, TabParent should send the pending keyboard events to the remote process. Otherwise, PresShell should dispatch the stopped keyboard events on the new target again.

I think that this approach can fix the root cause of this bug. However, even with this approach, IME cannot handle the event like bug 610821... (it might be impossible to use asynchronous IPC for handling key input...)

Comment 22

[Mike de Boer [:mikedeboer]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #21)
> I don't think it's good way to add sequential number to KeyboardEvent.

That sounds like a hack, indeed.

> I guess that instead of that, chrome script and C++ code should be able to
> mark WidgetKeyboardEvent if it may change focus to chrome like "reserved"
> shortcut key handlers before sending the event to a remote process.
> 
> Then, TabParent should stop following keyboard events to send the remote
> process until the marked shortcut key is back to the chrome process.  If the
> keyboard event is consumed by the content, TabParent should send the pending
> keyboard events to the remote process. Otherwise, PresShell should dispatch
> the stopped keyboard events on the new target again.

This sounds like a good solution! Does that still allow content pages to cancel the shortcut behavior with .preventDefault() (thinking from a web-compat perspective)? This won't work for FAYT and Quickfind shortcuts, I guess, but Quickfind is becoming an eyesore regardless.

> I think that this approach can fix the root cause of this bug. However, even
> with this approach, IME cannot handle the event like bug 610821... (it might
> be impossible to use asynchronous IPC for handling key input...)

Well, as long as the main shortcuts are accompanied with modifier keys (CTRL, CMD, etc), there shouldn't be a problem, right? So yes, just like I mentioned above, for FAYT your idea will not work for composed characters. But is that a big problem? It's already considered bad UX to have keyboard shortcuts to chrome/ browser functionality _without_ modifier keys, right?

Comment 23

[Haik Aftandilian [:haik]]

Thanks for the help. I have a few follow up questions:

For the Cmd-f key, which is not a "reserved" command, is there an ordering for how this key event is delivered to Chrome and Content processes? The discussion above makes it sound like the event goes to Content first and only comes back to Chrome if Content doesn't consume it. Is that correct?

Does anyone think Cmd-f should be a reserved command? My understanding is that that would prevent web content from ever overriding Cmd-f. And if so that should be done in another bug.

Comment 24

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

(In reply to Mike de Boer [:mikedeboer] from comment #22)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #21)
> > I don't think it's good way to add sequential number to KeyboardEvent.
> 
> That sounds like a hack, indeed.
> 
> > I guess that instead of that, chrome script and C++ code should be able to
> > mark WidgetKeyboardEvent if it may change focus to chrome like "reserved"
> > shortcut key handlers before sending the event to a remote process.
> > 
> > Then, TabParent should stop following keyboard events to send the remote
> > process until the marked shortcut key is back to the chrome process.  If the
> > keyboard event is consumed by the content, TabParent should send the pending
> > keyboard events to the remote process. Otherwise, PresShell should dispatch
> > the stopped keyboard events on the new target again.
> 
> This sounds like a good solution! Does that still allow content pages to
> cancel the shortcut behavior with .preventDefault() (thinking from a
> web-compat perspective)?

Of course, yes.

> This won't work for FAYT and Quickfind shortcuts, I
> guess, but Quickfind is becoming an eyesore regardless.

Yeah, FAYT should use <key> for handling to start itself.

> > I think that this approach can fix the root cause of this bug. However, even
> > with this approach, IME cannot handle the event like bug 610821... (it might
> > be impossible to use asynchronous IPC for handling key input...)
> 
> Well, as long as the main shortcuts are accompanied with modifier keys
> (CTRL, CMD, etc), there shouldn't be a problem, right?

No, for example, user may type following order:

0. Open IME in an input field and click body or something which IME isn't available.
1. Accel+F
2. Type some characters

Then, the typed characters in #2 should be handled by IME before dispatching keyboard events. However, if #1 is handled after content process handled, some key input in #2 won't be sent to IME until focus is moved to the findbar's editor.

> So yes, just like I
> mentioned above, for FAYT your idea will not work for composed characters.
> But is that a big problem? It's already considered bad UX to have keyboard
> shortcuts to chrome/ browser functionality _without_ modifier keys, right?

Ideally, I agree. But actually, we shouldn't change the shortcut key combinations because most users will complain about the change.

(In reply to Haik Aftandilian [:haik] from comment #23)
> For the Cmd-f key, which is not a "reserved" command, is there an ordering
> for how this key event is delivered to Chrome and Content processes? The
> discussion above makes it sound like the event goes to Content first and
> only comes back to Chrome if Content doesn't consume it. Is that correct?

Partially, yes. But actually, Chrome can listen keyboard events before content because capturing phase is started at the root window of the focused window. The root window must be a chrome window.

> Does anyone think Cmd-f should be a reserved command? My understanding is
> that that would prevent web content from ever overriding Cmd-f. And if so
> that should be done in another bug.

It shouldn't be reserved since some web applications use the shortcut key. E.g., Google Spreadsheet. Additionally, Accel+F is not so important shortcut key even if it's killed by web contents. "Reserved" shortcut keys should be only focus/tab navigation related keys. Currently, Accel+W, Accel+T, Accel+N, Accel+Shift+W and Accel+Shift+P are reserved.

Comment 25

[Haik Aftandilian [:haik]]

Attachment: Proof of concept / hack code for feedback

Need some feedback on this proof-of-concept / hack patch.

For now, my main question is how do I re-dispatch keys after the Cmd-f is sent back from Content? With what I have in my hack code, the re-dispatched keys don't make it to the find dialog.

1) The patch attempts to "hold" keys in TabParent::SendRealKeyEvent that come after a Cmd-f is sent to the child, but before the Cmd-f is returned back from the child in TabParent::RecvReplyKeyEvent. (Being a proof-of-concept, the code checks for the Cmd-f keyevent, eventually Cmd-f would be designated in some way where the shortcut is defined similar to how shortcuts can be marked as reserved.) When Cmd-f is seen, mHoldingKeys is set to true and the following key events are appended to an nsTArray mHeldKeys.

2) When the Cmd-f is received back from the child in TabParent::RecvReplyKeyEvent, the Cmd-f is re-dispatched locally and then all the "held" keys are also re-dispatched. Then we set mHoldingKeys back to false.

Testing showed the correct keys are held, but when they are dispatched in TabParent::RecvReplyKeyEvent, they never make it to the find dialog input field.

How should the keys be re-dispatched so that they end up in the find dialog input field?

Comment 26

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Haik Aftandilian [:haik] from comment #25)
> Created attachment 8746154 [details] [diff] [review]
> Proof of concept / hack code for feedback
> 
> Need some feedback on this proof-of-concept / hack patch.

The shortcut is defined in localization files [1], so -> in theory <- it's not always necessarily "F".  I'm not sure of how that fits with the whole concept-patch, so I'm throwing it out there to be safe.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#294

Comment 27

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

Comment on attachment 8746154 [details] [diff] [review]
Proof of concept / hack code for feedback

> bool TabParent::SendRealKeyEvent(WidgetKeyboardEvent& event)
> {
>+  // XXX we have to handle, keyup, keydown, etc.
>   if (event.mMessage == eKeyPress) {
>     MOZ_LOG(GetLog(), LogLevel::Error,
>+      ("__Chrome___: 0x%llx mHoldingKeys %d", mach_absolute_time(),
>+        mHoldingKeys));
>+
>+    if (mHoldingKeys) {
>+      MOZ_LOG(GetLog(), LogLevel::Error,
>+        ("__Chrome___: 0x%llx HOLDING %c %s", mach_absolute_time(),
>+        CodeNameIndexToChar(event.mCodeNameIndex),
>+        MessageToString(event.mMessage)));
>+
>+      WidgetEvent *dupeEvent = event.Duplicate();

I guess that we shouldn't copy event targets here. Perhaps, Duplicate() should have a bool argument. (but not 100% sure.)

And also, you can use WidgetKeyboardEvent array instead of WidgetEvent array.

>   WidgetKeyboardEvent localEvent(event);
>   // Mark the event as not to be dispatched to remote process again.
>   localEvent.StopCrossProcessForwarding();
> 
>   // Here we convert the WidgetEvent that we received to an nsIDOMEvent
>   // to be able to dispatch it to the <browser> element as the target element.
>   nsIDocument* doc = mFrameElement->OwnerDoc();
>   nsIPresShell* presShell = doc->GetShell();
>   NS_ENSURE_TRUE(presShell, true);
>   nsPresContext* presContext = presShell->GetPresContext();
>   NS_ENSURE_TRUE(presContext, true);
> 
>   EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+
>+  // Use begin/end iteration
>+  while (mHeldKeys.Length() > 0) {

Using for loop and after that, clearing mHoldKeys is simpler.

>+      WidgetEvent *w = mHeldKeys[0];
>+      WidgetKeyboardEvent *k = (WidgetKeyboardEvent *)w;
>+      mHeldKeys.RemoveElementAt(0);
>+
>+      MOZ_LOG(GetLog(), LogLevel::Error,
>+        ("__Chrome___: 0x%llx DELAYED DISPATCH %c %s", mach_absolute_time(),
>+        CodeNameIndexToChar(k->mCodeNameIndex),
>+        MessageToString(k->mMessage)));
>+
>+      WidgetKeyboardEvent delayedLocalEvent(*k);
>+      delayedLocalEvent.StopCrossProcessForwarding();

No, don't call this. The event may need to go to the remote process. E.g., Cmd+F may be consumed by web apps.

>+      EventDispatcher::Dispatch(mFrameElement, presContext, &delayedLocalEvent);

Hmm, the focus may be changed already. So, I think that we need to recompute event target in PresShell. So, cannot you use nsIPresShell::HandleEvent()?

>+  bool mHoldingKeys;
>+  nsTArray<WidgetEvent*> mHeldKeys;

So, this should be nsTArray<WidgetKeyboardEvent*> and I like mPendingKeyboardEvents better.

Comment 28

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

(In reply to Haik Aftandilian [:haik] from comment #25)
> For now, my main question is how do I re-dispatch keys after the Cmd-f is
> sent back from Content? With what I have in my hack code, the re-dispatched
> keys don't make it to the find dialog.

Because the event holds event target and EventDispatcher does not recompute the event target with new focused element.

> 1) The patch attempts to "hold" keys in TabParent::SendRealKeyEvent that
> come after a Cmd-f is sent to the child, but before the Cmd-f is returned
> back from the child in TabParent::RecvReplyKeyEvent. (Being a
> proof-of-concept, the code checks for the Cmd-f keyevent, eventually Cmd-f
> would be designated in some way where the shortcut is defined similar to how
> shortcuts can be marked as reserved.) When Cmd-f is seen, mHoldingKeys is
> set to true and the following key events are appended to an nsTArray
> mHeldKeys.

I wonder, perhaps, we shouldn't hold keyboard events in TabParent. Instead, TabParent notifies PresShell stopping dispatching keyboard events.  Then, the keyboard events after Cmd+F won't be fired in chrome process's chrome DOM nodes too. I think that storing in TabParent may cause same keyboard events fired twice.

Additionally, you need to check IME state. If composition is started after Cmd+F, pending keyboard event should be flushed before dispatching compositionstart event into the DOM tree.