Closed Bug 973702 Opened 8 years ago Closed 7 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED INVALID
Tracking Status
firefox27 --- unaffected
firefox28 + unaffected
firefox29 + unaffected
firefox30 + unaffected
firefox31 + wontfix
firefox32 + wontfix

People

(Reporter: mozilla.persona, Unassigned)

References

Details

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() { }"
Component: General → DOM
That's because the properties are on window itself, instead of on the prototype.  This change was made in bug 932322.
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
Thanks for the prompt response and ref to the explanation Kyle.
I do wonder how web-compatible the spec is, though.  Reporter, did you run into this in the wild on an already-deployed site?
Flags: needinfo?(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
Flags: needinfo?(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.
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.  :(
Blocks: 932322
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INVALID → ---
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.
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.
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?
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.
Do you know which libraries those are?
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
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?
> 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.
Flags: needinfo?(bzbarsky)
I filed bug 976920 on backing bug 932322 out of beta for now.
Depends on: 976920
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.
Marking unaffected on 28 as per the backout in bug 976920
Just like in comment #17, marking 29 as unaffected.
Ditto comment 18, and updating tracking flags so as to keep this on our radar for 31 & 32
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
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Boris, if I understand correctly, the status tracking flags should be wontfix? and invalid for the bug itself?
Thanks
Flags: needinfo?(bzbarsky)
Yes.
Status: REOPENED → RESOLVED
Closed: 8 years ago7 years ago
Flags: needinfo?(bzbarsky)
Resolution: --- → INVALID
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.