Closed Bug 1119074 Opened 6 years ago Closed 5 years ago
[e10s] keypress while popup menu is active should not be delivered to the web page
STR: 1. Open an e10s window with two tabs, one containing a GitHub page such as https://github.com/heycam/webidl/pull/22 and the other can be anything. 2. Select the GitHub page tab. 3. Right click on the other tab's tab. 4. Press "C" to select the "Close Tab" menu item. 5. Notice that the GitHub page has navigated to the "Create a new issue" page, presumably because the page got a keypress event for the "C".
I can't reproduce this. Has it been fixed?
I can reproduce this. github doesn't seem to respond to a 'c' keypress anymore (or at least, on my machine it doesn't), but if you press 's' it focuses the search box when it shouldn't.
I'm not sure if this is the correct fix here, but basically what's happening is that we're dispatching the event to the DOM here: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#8156, then receiving it at http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp#2150. consume is set to true, and we mark the event to stop propagation, but it still gets sent to EventStateManager::PostHandleEvent, and eventually still ends up being sent across the process boundary here: http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp#3363 Setting mNoCrossProcessBoundaryForwarding (obviously) ensures it doesn't get sent across, but I'm unsure as to the wider implications of this. Does it make sense to stop cross process forwarding if propagation is stopped (I think it does?).
Attachment #8605481 - Flags: review?(felipc)
Let's see how much this breaks... https://treeherder.mozilla.org/#/jobs?repo=try&revision=45cebc870132
Comment on attachment 8605481 [details] [diff] [review] 0001-Bug-1119074-Mark-events-which-have-been-set-to-no-lo.patch Review of attachment 8605481 [details] [diff] [review]: ----------------------------------------------------------------- Looked to me like a reasonable thing to do, but there were tests failures from that change, right? Perhaps it's just those tests that are broken.. You might wanna check what smaug thinks about this too.
Attachment #8605481 - Flags: review?(felipc) → feedback+
Why not just call preventDefaut() on the event?
... in case the context menu handles the event.
But yeah, I guess that patch works too, and it is probably something we want.
nsXULPopupManager already calls PreventDefault() at http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp#2223; it doesn't seem to have the desired effect? Is there another bug here at play?
I reported something (bug 1168058) which seems to be a duplicate of this bug report. This is a security issue. Pages should not have access to my typing unless I intend to type on the page.
I think this is a safer approach. Rather than hit the event's StopPropagation method with the no-cross-process hammer, I've added a new method to disallow cross-process forwarding explicitly, and only called it from within nsXULPopupManager. I think this should pass the tests now.
Comment on attachment 8610223 [details] [diff] [review] 0001-Bug-1119074-If-we-re-stopping-event-propagation-from.patch Review of attachment 8610223 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/events/nsIDOMEvent.idl @@ +124,5 @@ > /** > + * The StopCrossProcessForwarding method is used to disallow this event > + * from crossing process boundaries > + */ > + void stopCrossProcessForwarding(); this will expose this function to web content. You'll need something like [chromeonly] or [notxpcom] (not sure which is the right one to use here). But I think it would be fine to just set the flag manually from nsXULPopupManager. Maybe you tried and that didn't work because you need a concrete class to do that? Try using GetInternalNSEvent to get access to mFlags.
Attachment #8610223 - Flags: review?(felipc) → review+
Updated as per smaug and felipe's comments on IRC. - Added [noscript] attribute to stopCrossProcessForwarding - Removed random comments from Event.h
Comment on attachment 8610241 [details] [diff] [review] 0001-Bug-1119074-If-we-re-stopping-event-propagation-from.patch So do we actually need StopCrossProcessForwarding() calls in this case? What if nsXULPopupManager::KeyDown called PreventDefault() ?
Comment on attachment 8610241 [details] [diff] [review] 0001-Bug-1119074-If-we-re-stopping-event-propagation-from.patch StopCrossProcessForwarding() implementation looks ok to me, but because of http://hg.mozilla.org/mozilla-central/rev/d5dc2d51f002 I'd prefer if masayuki looked at this. gwright, could you test if just adding aKeyEvent->PreventDefault(); to nsXULPopupManager::KeyDown where it used to be before http://hg.mozilla.org/mozilla-central/rev/d5dc2d51f002 would fix this.
Attachment #8610241 - Flags: review?(bugs) → review?(masayuki)
OK, so it looks like we can't just call PreventDefault() because that stops the popup from handling a keypress that's not a navigation event (ie - you should be able to hit 'P' when the popup from right-clicking a tab is open to pin it). If PreventDefault() is called unconditionally in KeyDown(), then the event doesn't get sent to content as this bug wants, but then KeyPress() never gets the key event and so the popup doesn't get to handle it. This makes sense based on masayuki's comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=501496#c16 I think we can probably do away with the StopCrossProcessForwarding() calls everywhere except in KeyDown, but I think we should probably keep them in as they aren't expensive.
Comment on attachment 8610241 [details] [diff] [review] 0001-Bug-1119074-If-we-re-stopping-event-propagation-from.patch I see. The StopCrossProcessForwarding calls are very cheap.
Attachment #8610241 - Flags: review?(masayuki) → review+
You need to log in before you can comment on or make changes to this bug.