835643 - Set up the infrastructure to switch EventListener to WebIDL codegen

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Now that bug 822470 has landed.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Give CallbackObject an IID so that random things don't QI to it. now it thinks its IID is the nsISupports IID!

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Store an EventListenerHolder, not an nsIDOMEventListener in nsListenerStruct.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Expose the ability to add WebIDL event listeners to an event listener manager via its API.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Switch EventListener to WebIDL codegen.

I'll push this to try when I'm on a network that douesn't block port 22, but local testing looks promising.  Olli, are there any performance tests worth running here?

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 712287 [details] [diff] [review]
part 2.  Store an EventListenerHolder, not an nsIDOMEventListener in nsListenerStruct.

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

::: content/events/src/nsEventListenerManager.cpp
@@ +924,5 @@
>      // nsIDOMEvent::currentTarget is set in nsEventDispatcher.
> +    if (aListener.HasWebIDLCallback()) {
> +      ErrorResult rv;
> +      aListener.GetWebIDLCallback()->HandleEvent(aCurrentTarget, aDOMEvent,
> +                                                  rv);

Nit: indentation

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Nit: indentation

Fixed.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'm seeing various test failures here.  Still debugging some of them but at the very least:

1)  I think we have tests that depend on bug 503244.  They now end up with actual
    exceptions reported and fail.  I have to go dig through them some more to be sure.

2)  We definitely have tests that depend on bug 822213 (e.g. 
    editor/composer/test/test_bug519928.html ).  I guess I should just go and fix
    bug 765780 before I check this in...  :(

Comment 8

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

http://mozilla.pettay.fi/moztests/events/event_speed_3.html is a totally artificial test.
If you want to emphasize callback handling, decrease the dom tree depth (default 100).

Comment 9

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

Comment on attachment 712286 [details] [diff] [review]
part 1.  Give CallbackObject an IID so that random things don't QI to it.   now it thinks its IID is the nsISupports IID!

I assume this is actually used in some other patch.

Comment 10

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

Comment on attachment 712287 [details] [diff] [review]
part 2.  Store an EventListenerHolder, not an nsIDOMEventListener in nsListenerStruct.

Hmm, so we end up doing double cx push/pop?
One in nsEventListenerManager::HandleEventInternal and one in webidl callback.
That is less than optimal.
Could we perhaps pass a flag to WebIDL callback to bypass cx pushing?

Comment 11

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

Comment on attachment 712287 [details] [diff] [review]
part 2.  Store an EventListenerHolder, not an nsIDOMEventListener in nsListenerStruct.

Or am I wrong... reading the generated code.

Comment 12

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

Comment on attachment 712287 [details] [diff] [review]
part 2.  Store an EventListenerHolder, not an nsIDOMEventListener in nsListenerStruct.

Ah, yes, the normal Callback setup is there.

Comment 13

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

Comment on attachment 712289 [details] [diff] [review]
part 4.  Switch EventListener to WebIDL codegen.

But did you say some tests are still failing?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I assume this is actually used in some other patch.

It's not directly, but it keeps some code that I wrote by accident that was wrong from compiling, iirc.

> Hmm, so we end up doing double cx push/pop?

Hmm.  Yes.  We really need to kill the JSContext stack.  :(

> Could we perhaps pass a flag to WebIDL callback to bypass cx pushing?

We could, but it might have a different cx...  I suppose we could pass in a cx to run on to indicate that no push/pop should happen, but it's not immediately obvious to me whether that gives the right behavior in all cases.

I need to think about this.

> But did you say some tests are still failing?

Yep.  See https://tbpl.mozilla.org/?tree=Try&rev=3169a0c5d034

Still sorting through the failures, but see comment 7.

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK, the M2 failures seem to be due to buggy tests that depend on bug 503244.  I mailed the test authors to find out what the deal with those is.

The M4 failure that's due to these patches is because the test depends on bug 822213.

The webapps test failures in Moth are because those tests depend on bug 503244.

Still looking into the rest of the Moth failures and the bc failure.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> OK, the M2 failures seem to be due to buggy tests that depend on bug 503244. 

Well, some of them are.  Looking into the others now.

A number of the other Moth failures seem to be tests that are depending on bug 503244.  I should fix all those and then see where we stand...

Comment 17

[Boris Zbarsky [:bzbarsky]]

So the remaining browser-chrome orange is due to the test closing a window in the middle of some XBL JS running on that window.  This used to not happen because we invoked the event handler on the cx of the event target (which is in that window), but now we do it on the cx of the handler (which is a different window).  I can fix this in the test by just doing the close async, but need to think a tad about what the right approach here is.

Comment 18

[Boris Zbarsky [:bzbarsky]]

So at this point the one orange I don't have a planned fix for is this:

13649 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/test_sanityException2.xul | expectUncaughtException was called but no uncaught exception was detected!

which triggers some other knock-on failures.  Problem is, I can't reproduce it locally by just running that one test.  :(

I _can_ "fix" it by running callbacks on the context of the object they're attached to (so for event listeners the context of the event target) instead of the context of the function object...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Addendum to part 4 to fix missed overriders of add/removeEventListener

Comment 20

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

Comment on attachment 713902 [details] [diff] [review]
Addendum to part 4 to fix missed overriders of add/removeEventListener

Tiny bit copy-pasting in DeviceStorage code, but I guess it is not too bad.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Yeah.  I considered combining them; I'd have to expose versions of add/remove that take an EventListenerHolder in various places.  Can be done, if desired, but the end result is just as much code, I think.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Looks like the sanityException2 issue was the same as comment 17, but triggered by an earlier test in the suite (js/xpconnect/tests/chrome/test_bug799348.xul to be precise).  I'm hoping bug 841312 will just make all those problems go away.

Comment 23

[Doug Turner (:dougt)]

Comment on attachment 713902 [details] [diff] [review]
Addendum to part 4 to fix missed overriders of add/removeEventListener

Fix up whitespace:

+                                     bool aUseCapture,
+                                     const Nullable<bool>& aWantsUntrusted,
+				     ErrorResult& aRv)

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Fix up whitespace:

Done, and added modelines to keep the tabs out.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Update to part 2 to deal with WebIDLification of Event

Comment 26

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

Comment on attachment 728321 [details] [diff] [review]
Update to part 2 to deal with WebIDLification of Event

>diff --git a/content/events/src/nsEventListenerManager.cpp b/content/events/src/nsEventListenerManager.cpp
>--- a/content/events/src/nsEventListenerManager.cpp
>+++ b/content/events/src/nsEventListenerManager.cpp
>@@ -923,18 +923,18 @@ nsEventListenerManager::HandleEventSubTy
>                                          nullptr);
>   }
> 
>   if (NS_SUCCEEDED(result)) {
>     nsAutoMicroTask mt;
>     // nsIDOMEvent::currentTarget is set in nsEventDispatcher.
>     if (aListener.HasWebIDLCallback()) {
>       ErrorResult rv;
>-      aListener.GetWebIDLCallback()->HandleEvent(aCurrentTarget, aDOMEvent,
>-                                                 rv);
>+      aListener.GetWebIDLCallback()->
>+        HandleEvent(aCurrentTarget, *static_cast<nsDOMEvent*>(aDOMEvent), rv);
*(aDOMEvent->InternalDOMEvent()) for now
to guarantee that you get the canonical pointer.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> *(aDOMEvent->InternalDOMEvent()) for now

Done.

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 712289 [details] [diff] [review]
part 4.  Switch EventListener to WebIDL codegen.

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

Any reason not to implement RemoveEventListener directly on dom::EventTarget?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Hmm.  I guess GetListenerManager is on nsIDOMEventTarget... Then no, no reason.  I'll do that!

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: Addendum to part 4 to do comment 28

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 712287 [details] [diff] [review]
part 2.  Store an EventListenerHolder, not an nsIDOMEventListener in nsListenerStruct.

Given that the performance is ok now, re-requesting review on this bit...

Comment 32

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

Comment on attachment 737695 [details] [diff] [review]
Addendum to part 4 to do comment 28

Could you move EventTarget::DispatchEvent from nsINode.cpp to
the new EventTarget.cpp

>+EventTarget::RemoveEventListener(const nsAString& aType,
>+				 EventListener* aListener,
>+				 bool aUseCapture,
>+				 ErrorResult& aRv)
You have some tabs here. Please use spaces

Comment 33

[Boris Zbarsky [:bzbarsky]]

> You have some tabs here. Please use spaces

Done.  Silly editors not noticing I added a modeline.  ;)

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: Addendum to part 4 needed to silence gcc warnings so we can build with all the -Werror bits

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: Addendum to part 4 to disable the new thing so far, until all the dependencies are fixed

Comment 36

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

Comment on attachment 737986 [details] [diff] [review]
Addendum to part 4 needed to silence gcc warnings so we can build with all the -Werror bits

horrible

Comment 37

[Boris Zbarsky [:bzbarsky]]

In the interests of this stuff not bitrotting more, I landed it disabled for now:

https://hg.mozilla.org/integration/mozilla-inbound/rev/72c3a9deb098
https://hg.mozilla.org/integration/mozilla-inbound/rev/55cad36868d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d918701c596
https://hg.mozilla.org/integration/mozilla-inbound/rev/603143e31531

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72c3a9deb098
https://hg.mozilla.org/mozilla-central/rev/55cad36868d8
https://hg.mozilla.org/mozilla-central/rev/7d918701c596
https://hg.mozilla.org/mozilla-central/rev/603143e31531