976920 - Back out bug 932322 on beta until we're sure the spec will stay as it is

Status: RESOLVED WONTFIX

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

Description

[Boris Zbarsky [:bzbarsky]]

Or something?  In any case, for now it's breaking sites and we don't know what final behavior we'll want.

I have no idea how to properly track this so we keep remembering to do it on every train.  :(

Comment 1

[Masatoshi Kimura [:emk]]

I propose to just backout everywhere until the spec is saner. It introduced more problems than it fixed.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The problems if fixes are the ones that will come up when we start removing prefixed crud from window.  So of course it's not fixing any right this second.

If you have a concrete proposal for how to fix the spec, do feel free to post to public-script-coord.

In the meantime, I think there is value in leaving this on nightly/aurora so we can find out whether there are other compat issues.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Mostly back out bug 932322 for now; only define the unforgeable properties on the window object itself.

Comment 4

[Boris Zbarsky [:bzbarsky]]

And to be clear, the only concrete counterproposals to the current spec are:

1)  Standardize some of the prefixed props.
2)  arv proposal of "let's just break sites if we ever remove prefixed props and then
    evangelize them to fix themselves".

In practice, #2 will act like #1, since no one will remove prefixed stuff if that means breaking sites.

Comment 5

[Masatoshi Kimura [:emk]]

Why we can't do more similar things to Chrome and IE? They aren't breaking either sites.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Mostly back out bug 932322 for now; only define the unforgeable properties on the window object itself.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Why we can't do more similar things to Chrome and IE?

Chrome has some combination of never removing prefixed properties and putting some properties (but not all!) on the object itself.

IE has a behavior that requires never removing prefixed properties.

Comment 8

[Boris Zbarsky [:bzbarsky]]

And the point is, if we want to spec the "never remove prefixed properties" behavior, the spec also needs to spec a prefixed property that everyone should implement.  Otherwise it's useless for new UAs.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8382233 [details] [diff] [review]
Mostly back out bug 932322 for now; only define the unforgeable properties on the window object itself.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 932322
User impact if declined: Some sites are likely broken
Testing completed (on m-c, etc.): Passes tests
Risk to taking this patch (and alternatives if risky): Some risk, but this is a
   pretty straight backout, so should get us back more or less to where we used
   to be in Firefox 27...
String or IDL/UUID changes made by this patch:  None.

Note that I want to only back this out on beta for now...

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Boris, before approving the uplift, can you tell me what you are planning to do with regard to 29 & 30? Thanks

Comment 11

[Boris Zbarsky [:bzbarsky]]

Keep gathering compat info and seeing whether we can get Chrome to agree on what the spec should be.  We may end up landing this patch on 29 and 30 as well in the future, if needed...

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8382233 [details] [diff] [review]
Mostly back out bug 932322 for now; only define the unforgeable properties on the window object itself.

Good plan, approving for today's beta.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dc567976528c

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/dc567976528c

Comment 15

[Kohei Yoshino]

Updated the site compat doc:
https://developer.mozilla.org/en-US/Firefox/Releases/28/Site_Compatibility

:jorgev can update https://blog.mozilla.org/addons/2014/02/21/compatibility-for-firefox-28/

Comment 16

[Kohei Yoshino]

What's the plan for 29 Beta and 30 Aurora?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Unclear so far.  Depends on what happens with the spec.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Boris, beta 4 should be released today. Do you think we should consider a backout also in 29? Thanks

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yes, we should.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Version of the patch for beta 29

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8400045 [details] [diff] [review]
Version of the patch for beta 29

Try run at https://tbpl.mozilla.org/?tree=Try&rev=8f2d8576af48

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aecbb562466a

Comment 23

[Boris Zbarsky [:bzbarsky]]

Note that we've discussed this with the Apple and Google folks and they agree the spec should stay as is.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8400045 [details] [diff] [review]
Version of the patch for beta 29

Time to do this again, for 30, until we get the opener issues in bug 989584 sorted out.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d5fb33c4194e

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out from beta for mochitest-bc permafail.
https://hg.mozilla.org/releases/mozilla-beta/rev/8b801635d8ce

https://tbpl.mozilla.org/php/getParsedLog.php?id=40022732&tree=Mozilla-Beta

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Beta patch updated to work on Beta 30

Comment 28

[Boris Zbarsky [:bzbarsky]]

Yeah, browser_bug982298.js is busted.  It's importing Timer.jsm into the browser window!

I removed that gunk, and did a try push at https://tbpl.mozilla.org/?tree=Try&rev=f901a087c1ab

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1c26f6798184

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/1c26f6798184

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Lawrence, could you help here? (webcompat perspective)

Comment 32

[Boris Zbarsky [:bzbarsky]]

Note that there are also sites that depend on us NOT landing this patch.  See bug 1024806.

I think we're done here.  We have no plans to check this in again for 31.

Comment 33

[Kohei Yoshino]

Okay, then I'll document Bug 932322 in the Firefox 31 site compat doc. The original note that was in the 28 compat doc is below. Boris, can you please refine this?

--

<h3>Global variables with the same name as <code>window</code> properties are no longer accessible</h3>

<p>A typical impact of this change has been reported as <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=943958" title="Bug 943958 - Global variables with the same name as window properties can't accessed">Bug&nbsp;943958</a>. Code like <code>var name = 1</code> no longer works as expected, instead <code>name</code> will return a string <code>'1'</code> because of the {{ domxref("window.name") }} property. Web developers should avoid using global variables with the same name as {{ domxref("window") }} properties.</p>

Comment 34

[Boris Zbarsky [:bzbarsky]]

I would phrase it like so:

 <h3>Global variables with the same name as <code>window</code> properties now call the property setter when they are set.</h3>

The paragraph after that point is OK, though maybe it's worth pointing out that this is also the behavior WebKit and Blink have for window properties.

Comment 35

[Kohei Yoshino]

Awesome, thank you!

https://developer.mozilla.org/en-US/Firefox/Releases/31/Site_Compatibility#DOM
https://twitter.com/FxSiteCompat/status/477510724810334208

Comment 36

[Lawrence Mandel [:lmandel] (use needinfo)]

I take it comment 32 means that we're going ahead with this spec based change in 31. Do we know the expected breakage that we expect on the Web due to this change? Hallvord and Seif should be able to run their crawler to detect usage to give us some idea of what this change will mean for Web content when it hits release.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> I take it comment 32 means that we're going ahead with this spec based change in 31.

Yes, correct.

> Do we know the expected breakage that we expect on the Web due to this change?

We don't expect much, for what it's worth.  We've been shipping this in nightlies and aurora and partway through beta in 28, 29, 30, and 31, and in nightly and partway through aurora in 32.  We've found and fixed a few issues along the way, so it's not like we're just trying this with no testing at all...

Of course replacing guesswork with data is always appreciated.  ;)

Comment 38

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Boris Zbarsky [:bz] from comment #37)
> Of course replacing guesswork with data is always appreciated.  ;)

Hallvord - Given the description in comment 33 and comment 34 I don't know how easy it will be to scrape the Web for usage and possible breakage. I suppose that we can scan for global declaration of all window properties. Thoughts?

Comment 39

[Hallvord R. M. Steen [:hallvors]]

(In reply to Lawrence Mandel [:lmandel] from comment #38)
> (In reply to Boris Zbarsky [:bz] from comment #37)
> > Of course replacing guesswork with data is always appreciated.  ;)
> 
> Hallvord - Given the description in comment 33 and comment 34 I don't know
> how easy it will be to scrape the Web for usage and possible breakage. I
> suppose that we can scan for global declaration of all window properties.

In this case, gathering the data from JS might be possible for many (not all) properties - it might be more robust and cover more properties if we put some logging in the ES engine itself I guess - but the real challenge would lie in the interpretation of the data once collected. If we've found 10 000 sites setting window.name, how many of those set it on purpose, and how many set it by mistake? I don't think that can be automated.