Closed Bug 1309587 Opened 8 years ago Closed 8 years ago

Text inputs on VIA Rail website behave badly with e10s

Categories

(Core :: DOM: Core & HTML, defect, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: jdm, Assigned: jessica)

References

Details

Attachments

(2 files, 4 obsolete files)

patch, v5.
2.71 KB, patch
jessica
: review+
Details | Diff | Splinter Review
patch for aurora.
2.71 KB, patch
jcristau
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
STR:
1. visit any page that is not viarail.ca in order to have a previous session history entry
2. visit viarail.ca/en
3. click on field labelled "From:"
4. type "toronto", but start pressing backspace after a couple letters

Expected:
Most recently typed letters are removed.

Actual:
With e10s enabled, the browser returns to the previous page. Without e10s enabled, the letters are removed.

The STR are a bit dicey, because this specifically happens when the page notices input occurring in the field and starts a sync XHR for all train station names after waiting for 150ms. There's a noticeable pause (especially on less optimal internet connections) when this occurs, and that's when the problem crops up.

Relevant HTML for the field:
```
<input onblur="InputLostFocus('divStation0', 'cmbStationsFrom',2)" onclick="select();" onkeypress="return KeyPress();" onkeydown="KeyDownEv(this.value, event, 'divStation0', 'cmbStationsFrom',2)" onkeyup="KeyUp(this.value, event, 'divStation0', 'depart','','')" onfocus="ListOnFocus(this.value, 'divStation0',2,'depart','', 'cmbStationsFrom','')" autocomplete="off" value="" id="cmbStationsFrom" class="text reservation_mod" aria-required="true" type="text">
```
The JS handlers can be found in www.viarail.ca/sites/all/themes/custom/viarail/js/stationCtrl.js
Note: this is not a regression; something about the site changed recently to start triggering this problem. I tried older versions of FF with e10s enabled and had the same issue.
Mike, any thoughts?
Flags: needinfo?(mconley)
I actually noticed a similar thing recently, but I don't remember what site it was.
Hm. I know that we have some event gunk that checks to see if content should consume a key event, or if the parent process should... clearly, something in there is going wrong.

Hey masayuki, I seem to recall you working in this space. Do you have any idea what might be happening?
Flags: needinfo?(mconley) → needinfo?(masayuki)
Hmm, I cannot reproduce this in my environment.

When an editor has focus, Backspace key event is consumed by it. Then, Backspace key handle ignores the event later. So, if it might be the editor to lose focus temporarily when something occurs. But I don't see such code in the HTML and JS files.
Flags: needinfo?(masayuki)
FWIW, I can't reproduce on Windows 10 with Nightly.
I can still reproduce this on OS X nightlies with internet connections that aren't super zippy.
(In reply to Josh Matthews [:jdm] from comment #7)
> I can still reproduce this on OS X nightlies with internet connections that
> aren't super zippy.
I managed to reproduce this, too.
See Also: → 1310913
Is anybody able to reproduce this problem on a platform other than OS X?
(In reply to Mike Conley (:mconley) - (high latency on reviews and needinfos) from comment #9)
> Is anybody able to reproduce this problem on a platform other than OS X?

Yes, I can. I am able to reproduce this on both Mac and Win10. As comment 7 says, internet connections that aren't super zippy are keys to reproducing this.
Priority: -- → P2
Some updates on this bug...

This issue is only reproducible on Windows and Mac and not on Linux, cause going history-back with backspace is not enabled in Linux [1].

What I found is that when this issue occurs, keypress events in content side were suppressed due to xhr sync, so EditorEventListener didn't get the events. However, these events were not suppressed in parent side, but consumed by browser.xul which does BrowserHandleBackspace() [2]. In normal cases, if the event is consumed by the editor, BrowserHandleBackspace() does not get called.
Hmm, does this make sense?


[1] http://searchfox.org/mozilla-central/source/browser/app/profile/firefox.js#673
[2] http://hg.mozilla.org/mozilla-central/file/944cb0fd0552/browser/base/content/browser.js#l1764
(In reply to Jessica Jong [:jessica] from comment #11)
> Some updates on this bug...
> 
> This issue is only reproducible on Windows and Mac and not on Linux, cause
> going history-back with backspace is not enabled in Linux [1].
> 
> What I found is that when this issue occurs, keypress events in content side
> were suppressed due to xhr sync, so EditorEventListener didn't get the
> events. However, these events were not suppressed in parent side, but
> consumed by browser.xul which does BrowserHandleBackspace() [2]. In normal
> cases, if the event is consumed by the editor, BrowserHandleBackspace() does
> not get called.
> Hmm, does this make sense?
> 
> 
> [1]
> http://searchfox.org/mozilla-central/source/browser/app/profile/firefox.
> js#673
> [2]
> http://hg.mozilla.org/mozilla-central/file/944cb0fd0552/browser/base/content/
> browser.js#l1764

That definitely sounds plausible. I'm guessing it'd be best to not consume non-reserved key events in the parent if the remote <xul:browser> is focused and (effectively, during a sync XHR) hung.
(In reply to Mike Conley (:mconley) - (digging out of review / needinfo backlog) from comment #12)
> That definitely sounds plausible. I'm guessing it'd be best to not consume
> non-reserved key events in the parent if the remote <xul:browser> is focused
> and (effectively, during a sync XHR) hung.

Thank you Mike for the input. Could you elaborate more on this? where should this be done? what are non-reserved key events? is it possible to know if the remote browser is hung due to sync XHR? sorry, kinda lost...

(CCing :smaug to see if he has any other ideas)
Flags: needinfo?(mconley)
(In reply to Jessica Jong [:jessica] from comment #13)
> Thank you Mike for the input. Could you elaborate more on this? where should
> this be done? what are non-reserved key events? is it possible to know if
> the remote browser is hung due to sync XHR? sorry, kinda lost...
> 
> (CCing :smaug to see if he has any other ideas)

That's quite alright. I'm not 100% familiar with our key event stuff myself, to be honest. :)

"reserved" key events are for keys that the parent has indicated that the content cannot override. This is stuff like Ctrl-T, Ctrl-N, Alt-F4, etc. These were added in bug 1203059.

I think felipe might have a better understanding then me about how events are dispatched to content and echo'd back up to the parent (I think that's what happens?).

Hey felipe - my understanding is that if a remote <xul:browser> is focused, we send key events to the content process, and then the content process messages us back to say whether or not they've been consumed, right? Since the content process is temporarily stuck with the sync XHR and slow network that the STR requires, is there some kind of timeout where the parent goes "oh well, better bubble this thing up anyways", and doesn't wait for the message to come back from the child? That'd explain why the parent suddenly responds to the backspace key event when we really should have waited for content to free up and deal with it...
Flags: needinfo?(mconley) → needinfo?(felipc)
(In reply to Mike Conley (:mconley) - (digging out of review / needinfo backlog) from comment #14)
> That's quite alright. I'm not 100% familiar with our key event stuff myself,
> to be honest. :)
> 
> "reserved" key events are for keys that the parent has indicated that the
> content cannot override. This is stuff like Ctrl-T, Ctrl-N, Alt-F4, etc.
> These were added in bug 1203059.

I see, thanks for the clarification.

> 
> I think felipe might have a better understanding then me about how events
> are dispatched to content and echo'd back up to the parent (I think that's
> what happens?).
> 
> Hey felipe - my understanding is that if a remote <xul:browser> is focused,
> we send key events to the content process, and then the content process
> messages us back to say whether or not they've been consumed, right? Since
> the content process is temporarily stuck with the sync XHR and slow network
> that the STR requires, is there some kind of timeout where the parent goes
> "oh well, better bubble this thing up anyways", and doesn't wait for the
> message to come back from the child? That'd explain why the parent suddenly
> responds to the backspace key event when we really should have waited for
> content to free up and deal with it...

Actually, most of the times, these key are simply *suppressed* (not delayed). In this case, I think a naive solution is to set a 'suppressed' flag, and ignore the event in nsXBLWindowKeyHandler::HandleEvent if this flag is set. But there are still a few cases where keypress events are *delayed* (if keydown is already handled, then the following key events are delayed [1]), this should be the case you mentioned above.

[1] http://hg.mozilla.org/mozilla-central/file/3e73fd638e68/layout/base/nsPresShell.cpp#l7447
In summary, nsXBLWindowKeyHandler, when handling key events, will wait for content process' reply. If content process does not capture it, nsXBLWindowKeyHandler will fall back triggering the corresponding handlers. In this case, because events were suppressed/delayed in content side, hence not captured by editor, so nsXBLWindowKeyHandler triggers BrowserHandleBackspace().

It'd be simple to know whether event is suppressed and ignore it as well in nsXBLWindowKeyHandler. But what about delayed events? should content process send the reply after the delayed event is fired? (now it sends the reply in in TabChild::RecvRealKeyEvent).

Olli, do you have any comment about this?
Flags: needinfo?(bugs)
FYI: I'm rewriting KeyboardEvent propagation in e10s mode in bug 1257617. After fixing it, Chrome will handle keyboard events after remote process handles it. See bug 1257617 comment 20 for knowing what I want to do. Although, I'm not familiar with delayed events when content process is busy.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #17)
> FYI: I'm rewriting KeyboardEvent propagation in e10s mode in bug 1257617.
> After fixing it, Chrome will handle keyboard events after remote process
> handles it. See bug 1257617 comment 20 for knowing what I want to do.
> Although, I'm not familiar with delayed events when content process is busy.

Thanks Masayuki for the information.
In PresShell, key events may be suppressed/delayed if doc's EventHandlingSuppressed() is true [1] (e.g. due to sync XHR). If event is added to the delayed event queue, event will be dispatched later when PresShell::FireOrClearDelayedEvents() [2] is called. In your new implementation, will it wait for this case to send the reply to TabParent?


[1] https://hg.mozilla.org/mozilla-central/file/f13e90d496cf/layout/base/nsPresShell.cpp#l7450
[2] https://hg.mozilla.org/mozilla-central/file/f13e90d496cf/layout/base/nsPresShell.cpp#l9058
Flags: needinfo?(felipc)
Oh, delayed events. I guess child process could effectively consume all the keyevents while its input event handling is suppressed.
Flags: needinfo?(bugs)
Attached patch patch, v1. (obsolete) — — Splinter Review 
Hi Olli, per our discussion on IRC, parent should should just drop all the key events delayed/suppressed in content side. Is this something you have in mind?
Attachment #8810315 - Flags: feedback?(bugs)
Comment on attachment 8810315 [details] [diff] [review]
patch, v1.

I was thinking to just preventDefault on child process. Does that not work?
Even though the separate flag makes this special case more clear, there is the risk that not all the event listeners check it. 

Is there some reason why preventDefault approach couldn't be taken?
Attachment #8810315 - Flags: feedback?(bugs)
(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8810315 [details] [diff] [review]
> patch, v1.
> 
> I was thinking to just preventDefault on child process. Does that not work?
> Even though the separate flag makes this special case more clear, there is
> the risk that not all the event listeners check it. 
> 
> Is there some reason why preventDefault approach couldn't be taken?

preventDefault on child process works as well, just didn't want to affect other cases. Will come up with a new patch.
Attached patch patch, v2. (obsolete) — — Splinter Review 
using preventDefault on events that are sent back to parent.
Attachment #8810315 - Attachment is obsolete: true
Attachment #8810749 - Flags: feedback?(bugs)
Comment on attachment 8810749 [details] [diff] [review]
patch, v2.

I was thinking to call PreventDefault() in PresShell.
If wanted, we could limit that only to cases when mWantReplyFromContentProcess is true, but not sure why even that would be needed.
Attachment #8810749 - Flags: feedback?(bugs)
(In reply to Olli Pettay [:smaug] from comment #24)
> Comment on attachment 8810749 [details] [diff] [review]
> patch, v2.
> 
> I was thinking to call PreventDefault() in PresShell.
> If wanted, we could limit that only to cases when
> mWantReplyFromContentProcess is true, but not sure why even that would be
> needed.

Hmm, wouldn't that affect listeners in content side when the delayed events are actually fired? I mean, when events are fired after they are unsuppressed, listeners will get a already default-prevented event.
How so? You'd preventDefault() after creating DelayedEvent. DelayedEvent creates a copy of the event.
(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #26)
> How so? You'd preventDefault() after creating DelayedEvent. DelayedEvent
> creates a copy of the event.

Oh, right. I'll upload a new patch.
Attached patch patch, v3. (obsolete) — — Splinter Review 
New patching coming..
Attachment #8810749 - Attachment is obsolete: true
Attachment #8811554 - Flags: feedback?(bugs)
Comment on attachment 8811554 [details] [diff] [review]
patch, v3.

Yeah, I think this might be enough, assuming it fixes this bug :)
Please test both e10s and non-e10s

(sync XHR is so horrible)
Attachment #8811554 - Flags: feedback?(bugs) → feedback+
Attached patch patch, v4. (obsolete) — — Splinter Review 
- Tested on e10s, non-e10s and ran try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b5eb7c8bd216a431690a345833473b5cbf7acfc&group_state=expanded
- Updated commit message and reviewer name.
Assignee: nobody → jjong
Attachment #8811554 - Attachment is obsolete: true
Comment on attachment 8812087 [details] [diff] [review]
patch, v4.

Olli, if you don't mind to review this tiny patch... :)
Flags: needinfo?(bugs)
Comment on attachment 8810749 [details] [diff] [review]
patch, v2.

After thinking about bug 1316330, I think we should take this approach after all.
bug 1316330 will need to add some special handling to preventDefault() and suppressed events, so better to mess with that stuff there.

Makes sense?
Flags: needinfo?(bugs)
Attachment #8810749 - Flags: review+
Comment on attachment 8810749 [details] [diff] [review]
patch, v2.

Sorry that I didn't realize that before.
Attachment #8810749 - Attachment is obsolete: false
(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #32)
> so better to mess with that stuff there.
> 
to not mess
(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #32)
> Comment on attachment 8810749 [details] [diff] [review]
> patch, v2.
> 
> After thinking about bug 1316330, I think we should take this approach after
> all.
> bug 1316330 will need to add some special handling to preventDefault() and
> suppressed events, so better to mess with that stuff there.
> 
> Makes sense?

Sure, I'll land this first and let's discuss bug 1316330 there. Thanks.
Attached patch patch, v5. — — Splinter Review 
same as patch v2, with updated commit message and reviewer, carrying r+.
Attachment #8810749 - Attachment is obsolete: true
Attachment #8812087 - Attachment is obsolete: true
Attachment #8812766 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b3e82cf6ec0
PreventDefault() on suppressed/delayed key events that are sent back to parent. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1b3e82cf6ec0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Approval Request Comment
[Feature/Bug causing the regression]: e10s
[User impact if declined]: pressing return key in input box may go history-back in Windows and OSX
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, manually
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal
[Why is the change risky/not risky?]: just drop some key events in parent in very special cases
[String changes made/needed]: None
Attachment #8827812 - Flags: approval-mozilla-aurora?
Comment on attachment 8827812 [details] [diff] [review]
patch for aurora.

event handling fix for e10s, aurora52+
Attachment #8827812 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: