523356 - Remote event listeners for Electrolysis

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

As a first step event.target and similar won't be possible.

Comment 1

[Benjamin Smedberg]

For now, I don't think we should send events unless chrome explicitly registers an event listener on the remote frame (so things that rely on inner events bubbling up through the <xul:browser> will break). Otherwise we're going to generate a lot of IPC traffic for DOM events which probably aren't useful.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #1)
> For now, I don't think we should send events unless chrome explicitly registers
> an event listener on the remote frame
We should never do that, since it would slow down event handling dramatically.
Only send events which we have an listener for.

Comment 3

[Benjamin Smedberg]

Yeah, but currently we bubble events all the way up through the parent (chrome) document, right? I'm suggesting that if there's an event listener on the chrome document, it shouldn't ever receive event notifications for the child documents.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #3)
> Yeah, but currently we bubble events all the way up through the parent (chrome)
> document, right?
Right, and I'd like to change that in E10s

> I'm suggesting that if there's an event listener on the chrome
> document, it shouldn't ever receive event notifications for the child
> documents.
Yes, I agree. Chrome needs to use some something else than .addEventListener
to add cross-process event listeners.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

This is on top of Bug 516727 (which requires bug 522533)

I need to merge these patches to latest e10s.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 409387 [details] [diff] [review]
patch

I'm not entirely sure we even want to take this to e10s repository.
ContentDriver could push drawWindow data automatically to chrome process.

Comment 8

[Benjamin Smedberg]

Comment on attachment 409387 [details] [diff] [review]
patch

>diff --git a/content/base/public/nsIFrameLoader.idl b/content/base/public/nsIFrameLoader.idl

>+  void addCrossProcessEventListener(in AString aType, in boolean capture);

Where will the event be sent? Will it be dispatched to the xul:browser/iframe element in the chrome document? Might it be better if this had an explicit `nsIDOMEventListener handler` argument so that events don't bubble through the XUL document at all?

Also, it would be nice to end up with a shared API for in-process and remote tabs where you just did addFrameEventListener and it didn't bubble ever, but I understand if we want to commit this now and revisit that in a different bug.

>diff --git a/content/events/public/nsIPrivateDOMEvent.h b/content/events/public/nsIPrivateDOMEvent.h

> class nsIPrivateDOMEvent : public nsISupports
> {
>@@ -61,6 +63,16 @@
>   NS_IMETHOD_(PRBool) IsDispatchStopped() = 0;
>   NS_IMETHOD_(nsEvent*) GetInternalNSEvent() = 0;
>   NS_IMETHOD SetTrusted(PRBool aTrusted) = 0;
>+  NS_IMETHOD Serialize(nsIBinaryOutputStream* aOutput,
>+                       PRBool aSerializeInterfaceType)
>+  {
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+  }
>+  // aInput does not contain interface type!
>+  NS_IMETHOD Deserialize(nsIBinaryInputStream* aInput)
>+  {
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+  }

Why are you using nsIBinaryOutputStream here? Wouldn't it be more efficient to just use IPC::Message* here? This gives you the added advantage that you can use the IPC::ParamTraits to serialize nsTArray and specialize structures like nsInvalidateRequestList::Request so that you get the serialization/deserialization almost for free. Also make it `virtual bool` instead of a NS_IMETHOD. And then the IPDL signature for SendEvent can just use a custom struct mozilla::dom::RemoteDOMEvent which just holds a nsIDOMEvent*.

What does the aSerializeInterfaceType param mean? That needs a doccomment.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #8)
> Where will the event be sent? Will it be dispatched to the xul:browser/iframe
> element in the chrome document?
Yes.

> Might it be better if this had an explicit
> `nsIDOMEventListener handler` argument so that events don't bubble through the
> XUL document at all?
Why? Doesn't make sense to me to re-implement EventListenerManager for "remote events".
 
> Also, it would be nice to end up with a shared API for in-process and remote
> tabs where you just did addFrameEventListener and it didn't bubble ever,
I don't understand what has bubbling to do with anything here.
You probably mean propagating. But even then, why shouldn't the event
propagate normally?
 
> Why are you using nsIBinaryOutputStream here? Wouldn't it be more efficient to
> just use IPC::Message* here?
Because there is no documentation how to use IPC::Message in cases like this ;)

> What does the aSerializeInterfaceType param mean? That needs a doccomment.
We need to serialize the type of the event interface, so that we can create
right kind of event on the chrome side.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Use IPC::Message for (de)serialization

I didn't add yet another ParamTraits for nsInvalidateRequestList::Request, but
just do (de)serialization manually, since it is less code that way.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3.1

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3.2

Bah,yet few nits.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: with the missing files

Argh, this time even with the missing files.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Because bug 526990 is now fixed, I can remove the float->int->float casting.

Comment 15

[Benjamin Smedberg]

The fact that event serialization can fail makes this code seem really fragile, and the NS_ENSURE_TRUE calls sprinkled around make it really hard to see early-return exit points. Can we ban NS_ENSURE_ from new code here, as I already have in XPCOM and toolkit?

It seems better to me if we could make the serialization methods infallible, and have a separate method "supportsSerialization" or something like that which we call before attempting to send the message. Once you've actually determined that the concrete type implements the serialize method, is there any reason it would subsequently fail, apart from C++ OOM which we're assuming aborts.

The ownership model of RemoteDOMEvent.mEvent is a little hazy: if there are any errors before the value reaches the handler function, or any early errors within the handler, client code may not be able to release the reference as it should. It seems safer to me to make it a nsCOMPtr.

Is there any reason you make nsIDOMEvent.Serialize/Deserialize use PRBool return type instead of bool? Use bool in new C++ code, especially since the matching Chromium method uses bool also.

Comment 16

[Benjamin Smedberg]

To be clear, if we have to keep fallible serialization of DOM events, the ParamTraits::Write method needs a big comment about why you EmptyString to the buffer, and a matching comment in ReadRemoteEvent about how you deserialize that case.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #15)
> Is there any reason you make nsIDOMEvent.Serialize/Deserialize use PRBool
> return type instead of bool? 
Event handling code uses PRBool. And I'm not aware (at least til now) that
new code should use bool.
Having an interface which uses both bool and PRBool 'looks' bad.
(I wonder when we're going to change all the PRBools to bools.)


> Can we ban NS_ENSURE_ from new code here, as I already have in XPCOM and
> toolkit?
I have no plans to ban NS_ENSURE_ in event handling code (if anyone asks me :) ).
I use NS_ENSURE_ macros when I do want to get the warning to the console but
still return early from the method.

Comment 18

[Benjamin Smedberg]

We are going to replace all the PRBools with bools as soon as I can break XPCOM binary compatibility.

Comment 19

[Benjamin Smedberg]

Comment on attachment 410818 [details] [diff] [review]
with the missing files

r- at least for ownership of RemoteDOMEvent.mEvent and as discussed on IRC, only serializing events which actually support it.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v4

Comment 21

[Benjamin Smedberg]

Comment on attachment 411673 [details] [diff] [review]
v4

I still think that NS_ENSURE_* make things worse here, but I'm not a content peer so I'll merely note it as an external objection. r=bsmedberg

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/projects/electrolysis/rev/1f3d37597416

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Windows didn't want to link nsEventDispatcher::CreateEvent -> reopening

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v5

this compiles on windows, and hopefully even on osx.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/projects/electrolysis/rev/ddf8790d3c4a