973702 - Unable to override `window.addEventListener` via `Window.prototype`

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[mozilla.persona]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.107 Safari/537.36

Steps to reproduce:

I am unable to override `addEventListener` via `Window.prototype` on firefox nightly (30.0a1 2014-02-17).  This is a regression similar to #612267 and #428229 although I notice it does not apply to all functions (eg the `testMethod` test in #612267 actually passes).

It *does* work to override `addEventListener` on `window` directly.

Window.prototype.addEventListener = function myFun() { };
window.addEventListener + "";


Actual results:


"function addEventListener() {
    [native code]
}"


Expected results:

"function myFun() { }"

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That's because the properties are on window itself, instead of on the prototype.  This change was made in bug 932322.

Comment 2

[mozilla.persona]

Thanks for the prompt response and ref to the explanation Kyle.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I do wonder how web-compatible the spec is, though.  Reporter, did you run into this in the wild on an already-deployed site?

Comment 4

[mozilla.persona]

Boris, I'm one of the maintainers of a (near) [polyfill][1] for MessageChannel.  A user [reported][2] that it's broken on FF nightly due to this issue; they've had to work around it in their own library, [Firetext][3].


[1]: https://github.com/tildeio/MessageChannel.js
[2]: https://github.com/tildeio/MessageChannel.js/issues/18
[3]: https://github.com/Codexa/Firetext

Comment 5

[mozilla.persona]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #3)
> I do wonder how web-compatible the spec is, though.  Reporter, did you run
> into this in the wild on an already-deployed site?

One annoying thing to note is that in IE8 `window.attachEvent.apply` is undefined whereas `Window.prototype.attachEvent.apply` is callable (although not a function).

For this reason, I expect there may be other web-compatible problems for libraries (or sites) that need to support IE8.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah...  The problem is that we want to move the methods to the global object itself to solve a common set of breakage in websites.  The question is which behavior breaks _more_ sites.  :(  Especially in the long run.  It doesn't help that not everyone agrees that this spec decision is right, either.

I'm going to reopen this for now and ask for tracking, but time for 28 is _really_ short.  :(

Comment 7

[Boris Zbarsky [:bzbarsky]]

And the point is, it's not clear so far whether we should go ahead with shipping this in 28 or not.  Checking with some people what we want to do here.

Comment 8

[mozilla.persona]

Boris, just to be clear, we've fixed our MessageChannel polyfill so this isn't a problem for us any longer.  I only mentioned the above in the hope that it would be useful to you.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Right, I got that.  

Do you have any estimates how many people out there might be using the old version of your polyfill, by the way?

Comment 10

[mozilla.persona]

I don't, sorry.  Part of the difficulty is that I doubt many people are using the polyfill directly, but rather using libraries that depend on it.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Do you know which libraries those are?

Comment 12

[mozilla.persona]

There's [Oasis.js][1] (and indirectly [Conductor.js][2]), which I know about because I maintain Oasis and have worked on both libraries.  There's also the app whose contributor logged the initial issue for us, [Firetext][3], but I only know about that because of the issue.

There are a few other apps that I don't know, whose authors have contacted me for help on some issues (which why I know they exist at all).

I don't *believe* the polyfill is very widely used, but it's tough to say anything with confidence when I only know about users incidentally.


[1]: http://oasisjs.com/
[2]: https://github.com/tildeio/conductor.js
[3]: https://github.com/Codexa/Firetext

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

bz - this has been in beta now for several weeks and we're not hearing reports of a lot of breakage which seems to confirm that this is not widely used.  Anything else we can do to confirm that would be great, but as you know, we are a few weeks away from release and this Thursday's beta is the last for speculative fixes to land in with time to evaluate and backout if need be.

What is the actual experience for the user if this remains as-is for 28 while a fix is investigated more fully for 29?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> which seems to confirm that this is not widely used

We've had two reports of breakage so far: this bug and bug 943065.

> What is the actual experience for the user if this remains as-is for 28 while a fix is
> investigated more fully for 29?

I'm not sure, but the experience for web developers is terrible if we keep changing the behavior back and forth.

I'm currently testing whether I can just back out bug 932322 on beta, giving us more time to figure out what's going on with the spec and such.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I filed bug 976920 on backing bug 932322 out of beta for now.

Comment 16

[mozilla.persona]

Lukas Blakk, if you're asking about the near-polyfill for MessageChannel, the user experience is that the polyfill simply doesn't work: when a user adds a message event handler to receive the transferred ports, she will have strings (the pseudoports encoded) rather than objects to which she can `postMessage` and attach listeners &c.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking unaffected on 28 as per the backout in bug 976920

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Just like in comment #17, marking 29 as unaffected.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Ditto comment 18, and updating tracking flags so as to keep this on our radar for 31 & 32

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Boris, do you know if there is anything new here or we should also backout the change for 31 (and maybe in m-c/32 too to have the same behavior everywhere)? Thanks

Comment 21

[Boris Zbarsky [:bzbarsky]]

I don't think we should back this out for 31 or later.  At this point we have agreement in principle from Apple and Google on also implementing this spec, and barring stop-ship compat issues I think we should just ship it.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Boris, if I understand correctly, the status tracking flags should be wontfix? and invalid for the bug itself?
Thanks

Comment 23

[Boris Zbarsky [:bzbarsky]]

Yes.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Thanks