428229 - [FIX]Unable to override addEventListener

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's currently not possible to override addEventListener using prototypes due to us having special classinfo hacks. The hacks are there to allow chrome code to by default only get "trusted" events, but also allow chrome to opt in to getting all events.

Unfortunately this makes it impossible for webpages to do stuff like:

Node.addEventListener = function(type, listener, useCap) {
 ...
}

Since in effect addEventListener is defined on every node. Unfortunately overriding addEventListener has been suggested as a fix for many use cases, so it's a bummer that it doesn't work in gecko.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So right now what we have is that addEventListener is a native function in nsDOMClassInfo. We allow content to call it and pass a fourth arg, and we use that fourth arg in that case.

I guess the problem is that if the fourth arg is NOT passed we want to default it to different things for chrome and content.  Which means we need to tell apart "arg not passed" and "arg explicitly passed as false"...

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So in theory we could implement this by removing all the special code we have in classinfo and the extra idl file and instead let the AddEventListener implementation grab the 4th argument using JS-APIs, a'la how nsXMLHttpRequest::Open does it. That way we can detect when the argument isn't there and react appropriately.

The main problem with that though is that we'd have to make sure that the JS call at the top of the stack is actually a call to us, and not a call to some other random function which has called us.

This is something that in general we've wanted to have for a number of things (mostly various security checks).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Make the 3-arg version called by content behave sanely

This doesn't help chrome, or content with UniversalXPConnect privs, but it helps typical web pages.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 341473 [details] [diff] [review]
Make the 3-arg version called by content behave sanely

This goes into an infinite recursion on this script:

    var x = document.createElement("div");
    HTMLDivElement.prototype.addEventListener = x.addEventListener;
    x.addEventListener("click", function() {}, false);

I'll try to think of a way to address that.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: With the recursion issues addressed

Comment 7

[Johnny Stenback (:jst)]

I thought I had commented on this already, but apparently I didn't.

I think the right way to fix this is to fix bug is to fix bug 459452
and get rid of this special casing of addEventListener() (as the one patch in that bug already does). But that's not something we want to take this late for 1.9.1. If this something we must fix for 1.9.1 I'd be ok with the patch in this bug, but it doesn't seem critical to me so I'm wondering if it's worth it...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Well, this _is_ breaking code people are writing.  There's a duplicate in this bug, and it's come up in the newsgroups at least 2-3 times.  I admit it seemed more worth fixing for 1.9.1 a month and a half ago.  ;)

I agree that fixing bug 459452 is a better approach, so if we don't decide to take this for 1.9.1 we should probably just do that approach.

Comment 9

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

(In reply to comment #7)
> I think the right way to fix this is to fix bug is to fix bug 459452
> and get rid of this special casing of addEventListener() 
I still don't understand how bug 459452 helps here.

> (as the one patch in that bug already does).
I can't see such patch.

Comment 10

[ChristopherBalz]

(In reply to comment #9):

Regarding how fixing bug 45942 would help, per comment #1 apparently some flexibility is needed to handle both chrome and content.  Comment #1 says:

  " . . . we want to default it to different things for chrome and content. Which means we need to tell apart "arg not passed" and "arg explicitly passed as false"

Comment 11

[ChristopherBalz]

Correction: I meant "Regarding how fixing bug 459452" . . .

Comment 12

[ChristopherBalz]

For reference, in addition to functionality working on Safari 3.x+, in IE 8 ( in IE 8 Browser Mode and IE 8 Standards Document Mode ), the functionality also works as expected. 

In the IE 8 legacy modes (which are default on IE 8, for backwards compatibility reasons), the functionality does not work as expected.

The functionality is the inheritance of an overridden native 'addEventListener' method on 'Element.prototype' by lower-level DOM elements, such as 'div' elements, so that the method is inherited on the concrete object ('div', 'input', etc.) directly from 'Element.prototype'.   

( In my case, the overriding consists of wrapping the native 'addEventListener' method to produce a new function that remembers the object context of the event handler, among other things ).

Comment 13

[Boris Zbarsky [:bzbarsky]]

> The functionality is the inheritance of an overridden native 'addEventListener'
> method on 'Element.prototype' by lower-level DOM elements

This has never worked in Gecko, for any method.  It has nothing to do with this bug.

Comment 14

[ChristopherBalz]

(In reply to comment #13)
> > The functionality is the inheritance of an overridden native 'addEventListener'
> > method on 'Element.prototype' by lower-level DOM elements
> 
> This has never worked in Gecko, for any method.  It has nothing to do with this
> bug.

On the contrary, bug 456151 was marked as a duplicate of this bug.

Comment 15

[ChristopherBalz]

Detail on the duplicate: comment 4 on bug 456151: https://bugzilla.mozilla.org/show_bug.cgi?id=456151#c4

Comment 16

[Boris Zbarsky [:bzbarsky]]

That comment is flat out wrong.  This bug is about being unable to override even when defining on the most-derived prototype.  This bug forces having to override per-node.  The bug about not being able to override on non-most-derived prototypes is something entirely different.  That bug was resolved based on the HTMLDivElement used in its actual testcases.

Comment 17

[ChristopherBalz]

(In reply to comment #16)
> That comment is flat out wrong.  This bug is about being unable to override
> even when defining on the most-derived prototype.  This bug forces having to
> override per-node.  The bug about not being able to override on
> non-most-derived prototypes is something entirely different.  That bug was
> resolved based on the HTMLDivElement used in its actual testcases.

Okay, so it's not a dupe of this bug then?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Please read all of comment 16, ok? 

That said, if it's not a dup of this bug, it's a dup of the bug on maybe implementing what the HTML5 draft has to say about web prototypes right now.  But that behavior is not pinned down yet, so it's not clear that that bug is even going to end up valid.  In all cases, it's a dup.  Now can you _please_ stop adding noise to this bug?

Comment 19

[ChristopherBalz]

You state in Comment 16 above "That bug was resolved based on the HTMLDivElement used in its actual testcases."  Is there any bug report or comment on that?  I don't see any in bug 456151.  

Like most of your audience, I am just trying to find out what is working, what isn't, and where the direction on these bugs is with this browser.

Comment 20

[Lon Boonen]

Hi, I would like to add (at the risk of sounding superfluous) that this bug also applies to XMLHttpRequest.prototype.
I tried overriding its addEventListener, but that has no effect on instantiated objects.

Comment 21

[Peter Van der Beken [:peterv]]

Attachment: v2

Now that bug 459452 is fixed, do we want to do this?

Comment 22

[Boris Zbarsky [:bzbarsky]]

I think we do.  One question: why are we ignoring aWantsUntrusted == PR_TRUE if optional_argc == 0?  I realize this can never arise from JS and doesn't arise for C++ callers; can we just assert and document that and change logic like this:

+  if (optional_argc == 0 ? !nsContentUtils::IsChromeDoc(mNode->GetOwnerDoc()) :
+                           aWantsUntrusted) {

to:

  if (aWantsUntrusted ||
      (optional_argc == 0 && !nsContentUtils::IsChromeDoc(mNode->GetOwnerDoc()))

or something?  It seems easier to read to me than the ?: construct...  Or is there a good reason to keep the :? setup?

Comment 23

[Peter Van der Beken [:peterv]]

No particular reason, I originally checked aWantsUntrusted first but I changed it because I think it removes one test in the common case. I'll change it back. I guess you want the assertion to check that if aWantsUntrusted is PR_TRUE then optional_argc > 0?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Yes, exactly.

Comment 25

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 411554 [details] [diff] [review]
v2.1

Looks good.

Can we now make the 3-arg version noscript and quickstub the 4-arg version?

Comment 27

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

Hmm, that would be binary compatible, right?
I could do that in bug 481652.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> Hmm, that would be binary compatible, right?

Yep.

Comment 29

[Peter Van der Beken [:peterv]]

I tried landing this, but had to back out. Several problems:

1) needed to update nsRootAccessible::AddEventListeners to add optional_argc
2) needed to make nsIDOMNSEventTarget a CI-exposed interface in
   test_bug389797.html
3) nsIXMLHttpRequestEventTarget inherits from nsIDOMEventTarget, so
   nsIDOMNSEventTarget needs to be before nsIXMLHttpRequestEventTarget in the
   list of interfaces. Unfortunately, that seems impossible for
   XMLHttpRequestUpload (its primary interface inherits from
   nsIXMLHttpRequestEventTarget)
4) A bunch of SVG elements exposed addEventListener, but don't have
   nsIDOMEventTarget in their CI interfaces list. I think we should just add
   nsIDOMEventTarget (and nsIDOMNSEventTarget) to their interfaces list. Smaug,
   that should be ok?

(In reply to comment #26)
> Can we now make the 3-arg version noscript and quickstub the 4-arg version?

I'd really like to do this (it would help avoid/fix problem 3 above). One issue is Worker, it currently only inherits from nsIDOMEventTarget. I think it should be ok to make that inherit from nsIDOMNSEventTarget too. The other thing I think this would break is EventTarget.prototype.addEventListener = foo, script would have to do NSEventTarget.prototype.addEventListener = foo?

Comment 30

[Boris Zbarsky [:bzbarsky]]

> The other thing I think this would break is
> EventTarget.prototype.addEventListener = foo

I'd be _very_ surprised if that works right now.

Comment 31

[Peter Van der Beken [:peterv]]

Yeah, I keep forgetting that's broken anyway, so I'll just quickstub and make Worker inherit from nsIDOMNSEventTarget.

Comment 32

[Peter Van der Beken [:peterv]]

Attachment: v2.1

This just adds bustage and test fixes.

Comment 33

[Peter Van der Beken [:peterv]]

Attachment: Add nsIDOMNSEventTarget to workers v1

This adds nsIDOMNSEventTarget to workers, so we can make the 3-argument addEventListener noscript. Not sure what to do if someone passes in aWantsUntrusted for workers, smaug suggested throwing NS_ERROR_NOT_IMPLEMENTED.

Comment 34

[Peter Van der Beken [:peterv]]

Attachment: Add nsIDOM(NS)EventTarget to all SVG elements v1

They already got an addEventListener before from the resolve hook.

Comment 35

[Peter Van der Beken [:peterv]]

Attachment: Quickstub addEventListener v1

Comment 36

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 414330 [details] [diff] [review]
Add nsIDOMNSEventTarget to workers v1

Looks good to me!

Comment 37

[Peter Van der Beken [:peterv]]

We also need to make sure to not lose the security checks on the inner window from AddEventListenerHelper (pointed out by moz_bug_r_a4 in bug 531364).

Comment 38

[Peter Van der Beken [:peterv]]

Attachment: Add security check to nsGlobalWindow::AddEventListener

Note that GetListenerManager does FORWARD_TO_INNER_CREATE, so no need to forward in AddEventListener.

Comment 39

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/c554301399ee
http://hg.mozilla.org/mozilla-central/rev/abfac14c4041
http://hg.mozilla.org/mozilla-central/rev/0177f1bb7197
http://hg.mozilla.org/mozilla-central/rev/fe544e3dfb7c
http://hg.mozilla.org/mozilla-central/rev/a64d7bdf282d