Closed Bug 523356 Opened 10 years ago Closed 10 years ago

Remote event listeners for Electrolysis

Categories

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

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(3 files, 5 obsolete files)

As a first step event.target and similar won't be possible.
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.
(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.
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.
(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.
Attached patch wip (obsolete) — Splinter Review
This is on top of Bug 516727 (which requires bug 522533)

I need to merge these patches to latest e10s.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #409387 - Flags: review?(benjamin)
Attachment #409387 - Flags: review?(benjamin) → review-
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.
(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.
Depends on: 526990
I didn't add yet another ParamTraits for nsInvalidateRequestList::Request, but
just do (de)serialization manually, since it is less code that way.
Attachment #408392 - Attachment is obsolete: true
Attachment #409387 - Attachment is obsolete: true
Attachment #410814 - Flags: review?(benjamin)
Attached patch v3.1 (obsolete) — Splinter Review
Attachment #410814 - Attachment is obsolete: true
Attachment #410815 - Flags: review?(benjamin)
Attachment #410814 - Flags: review?(benjamin)
Attached patch v3.2 (obsolete) — Splinter Review
Bah,yet few nits.
Attachment #410815 - Attachment is obsolete: true
Attachment #410816 - Flags: review?(benjamin)
Attachment #410815 - Flags: review?(benjamin)
Argh, this time even with the missing files.
Attachment #410816 - Attachment is obsolete: true
Attachment #410818 - Flags: review?(benjamin)
Attachment #410816 - Flags: review?(benjamin)
Because bug 526990 is now fixed, I can remove the float->int->float casting.
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.
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.
(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.
We are going to replace all the PRBools with bools as soon as I can break XPCOM binary compatibility.
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.
Attachment #410818 - Flags: review?(benjamin) → review-
Attached patch v4Splinter Review
Attachment #411673 - Flags: review?(benjamin)
Attachment #411673 - Flags: review?(benjamin) → review+
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
http://hg.mozilla.org/projects/electrolysis/rev/1f3d37597416
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Windows didn't want to link nsEventDispatcher::CreateEvent -> reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch v5Splinter Review
this compiles on windows, and hopefully even on osx.
http://hg.mozilla.org/projects/electrolysis/rev/ddf8790d3c4a
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.