Events handled by both content and chrome are not supported well in electrolysis

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

22 Branch
mozilla31
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Blocks: fxe10s
Ugh, somehow hit enter too soon.

I'm not sure what's the case, but it might be in the event code. It looks like we're not getting a keypress event for backspace in the content process.
Whiteboard: [e10s]
I assume that this happens in very simple forms, right? (basically just an input type=text let's say?)
Assignee: nobody → ehsan
Yes, it seems to happen in all input fields.
Posted patch hack (obsolete) — Splinter Review
This hack fixes the problem for me. Of course none of the keyset handlers in browser.xul are going to work. The previous code here was added in Bug 831421. (As a side note, how do you even trigger the backspace behavior in a normal build?)
backspace is handled by the editor here: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/nsPlaintextEditor.cpp#367> and then we call PreventDefault on it (in nsEditor::HandleKeyPressEvent) to prevent it from being processed later.  If the editor doesn't capture this event, we fall back to handle it using <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#301> which involves the code you're patching here.

It seems like the content process just doesn't see the backspace event, perhaps because we're handling it in the chrome process before passing it down to the content process?  (Note sure where that logic lives.)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> It seems like the content process just doesn't see the backspace event,
> perhaps because we're handling it in the chrome process before passing it
> down to the content process?
Yes, that is why evilpie's patch fixes this. But need to figure out some better, more generic
solution.
I've been thinking quite a bit about the system that we would want here (for a generic solution), and here are some thoughts.

The gist of the problem is that we need to be able to dispatch an event to the content process, have it call preventDefault(), and have the parent process be aware of that. This means that the parent process needs to wait for the content process to handle the event before continuing with its action, which is not done at the moment.

I'd like a solution that doesn't involve front-end handling in any way, similar to how it was done in bug 583976 (it might require front-end changes but no specific code to handle this). I think this is doable.

I think we also have a great opportunity here to fix the problem of webpages being able to block fundamental brower shortcuts such as ctrl+t, if we want. (note that this doesn't include plugins)

So I propose the following idea:

*Some* shortcut keys (but not all of them), will need to be marked with an attribute saying that they need async handling in the process boundary. We could do this with:

<key keycode="VK_BACK" command="cmd_handleBackspace" needsasync="true"/>

nsXBLWindowKeyHandler.cpp will look for this attribute and properly handle it.
- For keys not marked with it, it will keep working as it is now:
event hits the process boundary, is async-dispatched to the content process, and starts bubbling in the parent process, disregarding the handling done in content

- For keys marked with it, nsXBLWindowKeyHandler.cpp will prevent it from bubbling, generate an id associated with this event, and will wait for content to respond with an async EventHandled message.. Then the proper preventedDefault/propagationStopped flags will be set and the event resumes



If "event bubbling" can't be prevented or if bubbling is not really applicable here (not sure if keyHandler uses the same abstraction), replace it with "will wait the command action from being run until the async response is received"



What does this sound like? Correct? Doable?
Flags: needinfo?(bugs)
I don't quite understand the proposal. 
"nsXBLWindowKeyHandler.cpp will look for this attribute and properly handle it."..
"For keys marked with it, nsXBLWindowKeyHandler.cpp will prevent it from bubbling..."
that all happens in parent process, right? So the bubbling is prevented where?
Note, nsXBLWindowKeyHandler runs pretty much in the top level of event target chain, so there isn't
much to bubble.

"will wait for content to respond with an async EventHandled message." so how long? What if the content process dies or hangs. Well, perhaps we could detect that and cancel such cases.

Hmm, perhaps this is doable. Just need to make sure we don't start spinning event loop or anything
for waiting, but really just add some kind of async event listener in parent process.
Flags: needinfo?(bugs)
One thing to note is that this stuff is already buggy -- see bug 289384 for example.  Actually it might be interesting for somebody to try out attachment 718825 [details] [diff] [review] and see if it affects anything.  That might enable us do this dance in the content process (but see bug 289384 comment 75 first.)
I worked around this on the larch branch with this checkin:
https://hg.mozilla.org/projects/larch/rev/9d86bba13b48
It just turns off the key handler for the backspace key. You won't be able to go back by hitting backspace, but that's disabled by default anyway.

Hopefully we can find a better fix for this that can be checked into inbound, but until then, this should suffice.
I just filed bug 899338 to work around this problem on mozilla-central.
Summary: Backspace doesn't work in electrolysis builds → Need other keyhandling for e10s builds
Blocks: 918376
Summary: Need other keyhandling for e10s builds → Events handled by both content and chrome are not supported well in electrolysis
I'm working on this
Assignee: ehsan → felipc
Posted patch WIP (obsolete) — Splinter Review
From what we talked on IRC yesterday, this turned out to be simpler in two points:

- No need to set up an event listener in the child, because we can just tie this to the code that does the event dispatching (in TabChild::RecvRealKeyEvent), and thus we can just reply to the parent after dispatching has finished.

- It didn't need any new mechanisms to forward the event to the child, because ESM::HandleCrossProcessEvent already does that for every key event targeted to a <browser remote>.
Supposedly, the StopPropagation call (which I haven't added yet) won't prevent this from happening since ESM does that in PostHandleEvent.


Another thing missing from the patch is that nsXBLWindowKeyHandler will need to be a listener both for capturing and bubbling phases, as we want to stop propagation early for the events that we want to wait for a reply. The current handler is listening in the bubbling phase, which works but is not correct (it means that the event is being fully dispatched twice through in the UI)
Attachment #738204 - Attachment is obsolete: true
Attachment #8345529 - Flags: feedback?(bugs)
Posted patch Patch (obsolete) — Splinter Review
Finished version, just left one XX for you to tell me if it's necessary to do a null check.

I just sent it try, let's see how it goes: https://tbpl.mozilla.org/?tree=Try&rev=330b1313349a
Attachment #8345529 - Attachment is obsolete: true
Attachment #8345529 - Flags: feedback?(bugs)
Attachment #8346637 - Flags: review?(bugs)
Comment on attachment 8346637 [details] [diff] [review]
Patch

Review of attachment 8346637 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/xbl/src/nsXBLWindowKeyHandler.cpp
@@ +408,5 @@
>        do_QueryInterface(aEvent->GetInternalNSEvent()->originalTarget);
>      if (nsEventStateManager::IsRemoteTarget(originalTarget)) {
>        return NS_OK;
>      }
>    }

^ note to self: look into the original cset where this was added and see if it can now be removed too
(In reply to :Felipe Gomes from comment #15)
> Comment on attachment 8346637 [details] [diff] [review]
> Patch
> 
> Review of attachment 8346637 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/xbl/src/nsXBLWindowKeyHandler.cpp
> @@ +408,5 @@
> >        do_QueryInterface(aEvent->GetInternalNSEvent()->originalTarget);
> >      if (nsEventStateManager::IsRemoteTarget(originalTarget)) {
> >        return NS_OK;
> >      }
> >    }
> 
> ^ note to self: look into the original cset where this was added and see if
> it can now be removed too

I've checked and we still can't remove that with this patch, because the arrow keys are not listed as part of any <keyset>, so they don't get this special treatment. An alternative that fixes it would be to do the same treatment to all key events, not only the ones with matching handlers.
It's a fair proposal but it does suck a bit for every key event to have to wait on the content before running in the parent. Although this would be only for when focus is in content anyway.
But it also makes it harder to have a list of shortcut keys that cannot be ignored, which would be a nice benefit of the current approach (like disallowing content to interfere with ctrl + w, ctrl + t)
Ehsan, what is the code that makes the caret in an input box to move left or right when an arrow key is pressed? Content is receiving the event but is ignoring it, and I want to figure out why (fwiw it's not preventDefault as I've checked and it's not set)
Flags: needinfo?(ehsan)
(In reply to :Felipe Gomes from comment #17)
> Ehsan, what is the code that makes the caret in an input box to move left or
> right when an arrow key is pressed? Content is receiving the event but is
> ignoring it, and I want to figure out why (fwiw it's not preventDefault as
> I've checked and it's not set)

nsFrameSelection::MoveCaret is the main function.  If you set a breakpoint there and it's not triggered, then that's a sign that one of the callers is busted somehow.  IIRC this is called from three distinct locations:

** <http://dxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditorCommands.cpp#733> coming from places like <http://dxr.mozilla.org/mozilla-central/source/content/xbl/builtin/editor-base.inc#4>
** <http://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindowCommands.cpp#294> for caret browsing (Toggle "Always use the cursor keys to navigate withing pages" in prefs->advanced->general)
** <http://dxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#452> for inputs and textareas.

Isn't this setup absolutely beautiful?  ;-)
Flags: needinfo?(ehsan)
Comment on attachment 8346637 [details] [diff] [review]
Patch


>+  if (isXULKeyset) {
>+    // The capturing listener is only used for XUL keysets to properly handle
>+    // shortcut keys in a multi-process environment.
I don't understand this. Why keyset is special? Why we don't have the same behavior for platformHTMLBingings?


>+    manager->AddEventListenerByType(handler, NS_LITERAL_STRING("keydown"),
>+                                  dom::TrustedEventsAtSystemGroupCapture());
>+    manager->AddEventListenerByType(handler, NS_LITERAL_STRING("keyup"),
>+                                    dom::TrustedEventsAtSystemGroupCapture());
>+    manager->AddEventListenerByType(handler, NS_LITERAL_STRING("keypress"),
>+                                    dom::TrustedEventsAtSystemGroupCapture());
>+  }

Hmm, so listeners in default group would be called.


>+  // Inform the child process that this is a event that we want a reply
>+  // from.
>+  widgetEvent->mFlags.mWantReplyFromContentProcess = 1;
>+  aEvent->StopImmediatePropagation();
I think I'd call StopPropagation() here. It would behave less oddly.

>+bool nsXBLWindowKeyHandler::HasHandlerForEvent(nsIDOMKeyEvent* aEvent) {
bool to its own line and { to the next line

r- because I don't understand why special casing keysets, but not handle platform keybindings the same way.
Attachment #8346637 - Flags: review?(bugs) → review-
No longer blocks: e10s-it1
Whiteboard: [e10s] → [e10s] p=0
Posted patch felipe's patch v2 (obsolete) — Splinter Review
I made the changes that Olli asked for. I also changed HasHandlerForEvent so that it doesn't return false when GetElement() is null. That seems to make the code work for platformHTMLBingings. That allows us to remove the special code added in bug 831421 to handle arrow keys.
Attachment #8346637 - Attachment is obsolete: true
Attachment #8384053 - Flags: review?(bugs)
Comment on attachment 8384053 [details] [diff] [review]
felipe's patch v2

Canceling review for now. I think I'll have to combine this with the patch for bug 972420.
Attachment #8384053 - Flags: review?(bugs)
Comment on attachment 8384053 [details] [diff] [review]
felipe's patch v2

I worked some more on that bug and I think they're independent.
Attachment #8384053 - Flags: review?(bugs)
Comment on attachment 8384053 [details] [diff] [review]
felipe's patch v2

>+bool
>+TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& event)
>+{
>+  WidgetKeyboardEvent localEvent(event);
>+  // Set mNoCrossProcessBoundaryForwarding to avoid this event from
>+  // being infinitely redispatched and forwarded to the child again.
>+  localEvent.mFlags.mNoCrossProcessBoundaryForwarding = 1;
>+
>+  nsCOMPtr<mozilla::dom::EventTarget> target = do_QueryInterface(mFrameElement);
>+  NS_ENSURE_TRUE(target, true);
>+
>+  // 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.
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(mFrameElement);
>+  NS_ENSURE_TRUE(content, true);
You're doing quite some QIs here, and none of them is needed.
mFrameElement is Element*, and Element inherits from nsIContent, and nsIContent
inherits nsINode which inherits EventTarget.


>+  nsIDocument* doc = content->OwnerDoc();
>+  nsPresContext* presContext = doc->GetShell()->GetPresContext(); // XXX null check GetShell?
Yes. In general Get* method may return null.

>+
>+  nsIDOMEvent* domEvent;
>+  nsEventDispatcher::CreateEvent(target, presContext,
>+                                 &localEvent, EmptyString(), &domEvent);
You leak the world here with nsIDOMEvent*. Running debug builds with export XPCOM_MEM_LEAK_LOG=1
is recommended ;)

>+
>+  domEvent->SetOwner(target);
>+
>+  bool dummy;
>+  target->DispatchEvent(domEvent, &dummy);
>+  return true;

You could just
nsEventDispatcher::Dispatch(target, presContext, &localEvent);
Attachment #8384053 - Flags: review?(bugs) → review-
Posted patch events v3 (obsolete) — Splinter Review
Thanks for the help.
Attachment #8384053 - Attachment is obsolete: true
Attachment #8388918 - Flags: review?(bugs)
Duplicate of this bug: 972501
Comment on attachment 8388918 [details] [diff] [review]
events v3

>+TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& event)
>+{
>+  NS_ENSURE_TRUE(mFrameElement, true);
>+
>+  WidgetKeyboardEvent localEvent(event);
>+  // Set mNoCrossProcessBoundaryForwarding to avoid this event from
>+  // being infinitely redispatched and forwarded to the child again.
>+  localEvent.mFlags.mNoCrossProcessBoundaryForwarding = 1;
Uh, does this even compile. Anyhow, true; not 1;

>+
>+  // 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* docShell = doc->GetShell();
s/docShell/presShell/

>+nsXBLWindowKeyHandler::HandleEventOnCapture(nsIDOMKeyEvent* aEvent)
>+{
>+  WidgetKeyboardEvent* widgetEvent =
>+      aEvent->GetInternalNSEvent()->AsKeyboardEvent();
2 space indentation

>+
>+  if (widgetEvent->mFlags.mNoCrossProcessBoundaryForwarding == 1) {
>+    return;
>   }
just 
if (if (widgetEvent->mFlags.mNoCrossProcessBoundaryForwarding) {
  return;
}

>+nsXBLWindowKeyHandler::HasHandlerForEvent(nsIDOMKeyEvent* aEvent)
>+{
>+  bool trustedEvent = false;
>+  // Don't process the event if it was not dispatched from a trusted source
>+  aEvent->GetIsTrusted(&trustedEvent);
>+
>+  if (!trustedEvent) {
>+    return false;
>+  }
These days you can just do
if (!aEvent->InternalDOMEvent()->IsTrusted()) {
  return false;
}
You need to include "mozilla/dom/Event.h"



>+
>+  nsresult rv = EnsureHandlers();
>+  NS_ENSURE_SUCCESS(rv, false);
>+
>+  nsCOMPtr<Element> el = GetElement();
>+  if (el) {
>+    nsCOMPtr<nsIContent> content = do_QueryInterface(el);
Element inherits nsIContent.

>+    // skip keysets that are disabled
>+    if (content && content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::disabled,
>+                                        nsGkAtoms::_true, eCaseMatters)) {
>+      return false;
>+    }
>+  }
I guess you just copied this from elsewhere.
I'd prefer having some helper method to check whether the element is disabled.
Maybe another version of GetElement() which return Element* and has outparam bool* aIsDisabled;
That could be used then also in nsXBLWindowKeyHandler::WalkHandlers


Would like to see those nits fixed.
Attachment #8388918 - Flags: review?(bugs) → review-
Posted patch events v4Splinter Review
Attachment #8388918 - Attachment is obsolete: true
Attachment #8392491 - Flags: review?(bugs)
Attachment #8392491 - Flags: review?(bugs) → review+
Those were a small sampling, btw. Please make sure this has a fully-green Try run before attempting to push again.
I had a green run last night.
https://tbpl.mozilla.org/?tree=Try&rev=0be014f0d5fb
If only that were enough to guarantee my changes would stick! I think we're all going to end up with PTSD from inbound some day.
And when you're running this through Try, don't succumb to the temptation to call the failures below bug 874423. They're perma-fail with your patch.
https://tbpl.mozilla.org/php/getParsedLog.php?id=36344450&tree=Mozilla-Inbound
Sorry I was so grumpy. I guess I did screw up here. I missed one of the tests when making the GetElement change, and it randomly happened to pass all Linux64 tests while failing everything else.

Green try push: https://tbpl.mozilla.org/?tree=Try&rev=fa182ce16474

https://hg.mozilla.org/integration/mozilla-inbound/rev/cce47e059431
https://hg.mozilla.org/mozilla-central/rev/cce47e059431
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee: felipc → nobody
No longer blocks: fxdesktopbacklog
Whiteboard: [e10s] p=0
Assignee: nobody → wmccloskey

Updated

2 years ago
Depends on: 1328068
You need to log in before you can comment on or make changes to this bug.