1010577 - Add back window.controllers for site compatibility

Status: VERIFIED FIXED

(Core :: Security: CAPS, defect)

Description

[Kohei Yoshino [:kohei]]

+++ This bug was initially created as a clone of Bug #794943 +++

Looks like window.controllers is widely used to detect Gecko UA, and making [ChromeOnly] in Firefox 29 broke many sites. Let's take it back.

* Bug 1010311
* Bug 1010137
* https://support.mozilla.org/en-US/questions/997702
* https://support.mozilla.org/en-US/questions/997564
* https://www.google.com/search?q=ua+detection+"window.controllers"
* https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility

Comment 1

[Mike Taylor [:miketaylr]]

See also https://github.com/search?q=%22window.controllers%22+Gecko&ref=cmdform&search_target=global&type=Code

Comment 2

[Kohei Yoshino [:kohei]]

A similar case is Bug 791526. The netscape.security object was once removed but added back for compatibility.

https://developer.mozilla.org/en-US/Firefox/Releases/17/Site_compatibility#Security

Comment 3

[Kohei Yoshino [:kohei]]

Another 29.0.2 might be needed because this is breaking many sites.

Comment 4

[Kohei Yoshino [:kohei]]

Tweeted from FxSiteCompat:
https://twitter.com/FxSiteCompat/status/466709247011860480
https://www.facebook.com/FxSiteCompat/posts/641091809315995
https://plus.google.com/115208555324773750121/posts/U771BVJZyBn

Comment 5

[Boris Zbarsky [:bzbarsky]]

Note that at least one of the things from one of those Google searches is recommending:

 gecko = window.controllers && Object.prototype.toString.call(window.controllers) == “[object XULControllers]“

Can we get away with:

1) Making window.controllers === "[object XULControllers]" in content, but only in
   beta/release builds so we can keep finding places that use it.
2) Deprecation warning when it's first gotten/resolved from content.

?  We would really like to get sites that break on this sort of sniffing fixed, because otherwise Servo won't work with them....

Comment 6

[Karl Dubost💡 :karlcow]

Detecting is a good idea. I wish we had more telemetry for this type of features. 

Boris,
1. Do you know if we have this ability to test how much a feature is used through telemetry?
2. What is the process to add telemetry?

(I'm thinking about what blink is doing for some of their features.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Karl, see bug 968923.  I assume that we'll get that landed in the near future, once Cameron gets back.

Comment 8

[Mike Taylor [:miketaylr]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> Can we get away with:
> 
> 1) Making window.controllers === "[object XULControllers]" in content, but
> only in
>    beta/release builds so we can keep finding places that use it.
> 2) Deprecation warning when it's first gotten/resolved from content.

This sounds sensible to me.

I just grepped over my corpus of the top 64,621 domains (according to alexa, as of December 2013), with all the JS inlined:

egrep -r "window\.controllers" webdevdata.org-2013-12-09-064743/ > ~/Desktop/orientation.txt

$ cat ~/Desktop/controllers.txt | wc -l
35

7 of which are international variations of 1and1.com. And a handful of other sites embedding the Ace editor.

(Results can be cloned from https://gist.github.com/miketaylr/9a1306f2e1d8932b0f70)

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Tracking for now even if I am not sure that deserves a 29.0.2. 
Anyway, we will need a patch quickly, at least for 30.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Bobby, do you have the bandwidth to deal with this?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Bobby, do you have the bandwidth to deal with this?

Not really, but I can take it.

(In reply to Boris Zbarsky [:bz] from comment #5)
>  gecko = window.controllers &&
> Object.prototype.toString.call(window.controllers) == “[object
> XULControllers]“
> 
> Can we get away with:
> 
> 1) Making window.controllers === "[object XULControllers]" in content

This won't work, because Object.prototype.toString.call(someString) gives 'object [string]'. I can do something with a proxy or a JSClass hook though.

> , but only in
>    beta/release builds so we can keep finding places that use it.

This makes it pretty impossible to write a test here, but I guess we can just get QA to verify.

Comment 12

[Boris Zbarsky [:bzbarsky]]

We could run the test on RELEASE_BUILD only and have the uplift folks hate us... ;)

Comment 13

[Bobby Holley (:bholley)]

Attachment: Shim window.controllers (with a warning) in RELEASE_BUILDs. v1

Comment 14

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #12)
> We could run the test on RELEASE_BUILD only and have the uplift folks hate
> us... ;)

There's no way to detect channel in the manifest logic AFAIK. So yeah, let's QA this one heavily.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> There's no way to detect channel in the manifest logic AFAIK.

Hmm.  I thought we exposed nightly/aurora/release in the UA string, but I guess we nixed that.  OK, then.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8424182 [details] [diff] [review]
Shim window.controllers (with a warning) in RELEASE_BUILDs. v1

Nice.  r=me

Comment 17

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd8b2f03148b

Comment 18

[Bobby Holley (:bholley)]

Attachment: Shim window.controllers (with a warning) in RELEASE_BUILDs (without string changes). v1 r=bz

Attaching a patch for beta without the string changes.

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8424182 [details] [diff] [review]
Shim window.controllers (with a warning) in RELEASE_BUILDs. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 794943
User impact if declined: broken sites
Testing completed (on m-c, etc.): Just pushed to m-i
Risk to taking this patch (and alternatives if risky): Very low risk
String or IDL/UUID changes made by this patch: 1 string change for the warning

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8424182 [details] [diff] [review]
Shim window.controllers (with a warning) in RELEASE_BUILDs. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 794943
User impact if declined: broken sites
Testing completed (on m-c, etc.): Just pushed to m-i
Risk to taking this patch (and alternatives if risky): Very low risk
String or IDL/UUID changes made by this patch: No string changes (this patch does not include the deprecation warning, since it's intended to be landed on beta).

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 8424182 [details] [diff] [review]
Shim window.controllers (with a warning) in RELEASE_BUILDs. v1

Er. I meant to flag the other patch for beta.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8425030 [details] [diff] [review]
Shim window.controllers (with a warning) in RELEASE_BUILDs (without string changes). v1 r=bz

Checked on inbound and the landing looks good so approving now without waiting for the uplift to central so we can make sure to get this into tomorrow morning's beta for more coverage during the rest of this cycle.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/be4ae11bd65e
https://hg.mozilla.org/releases/mozilla-beta/rev/8104a9e1bfaf

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cd8b2f03148b

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/8104a9e1bfaf

Comment 26

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I've verified the fix in the latest Firefox 30 Beta 6 build (Build ID: 20140520115057).

I tested the following:
1. Variations of 1and1.com from Mike's list in comment 8
- I got the following error when clicking on "My Website" links from the Homepage: "000NaN Unsupported client: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0! Assumed gecko version 1.9.0.0 (Firefox 3.0)".
- the error displayed in the browser console only on 30 Beta 5 and 29.0.1, but did NOT display for 30 Beta 6
2. All other pages from Mike's list in comment 8
- did not get any issues on 30 Beta 5, 29.0.1 or 30 Beta 6 - everything worked as expected during navigation
3. https://forschung.beuth-hochschule.de/bis/beuth from bug 1010137
- does not load on 30 Beta 5 and 29.0.1
- loads fine on 30 Beta 6

The fix seems to have not made it to Nightly and Aurora yet, so marking this as Verified for Beta 32.

If some more in depth testing is needed, please let me know the details and we'll look into it a bit more.

Comment 27

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

> The fix seems to have not made it to Nightly and Aurora yet, so marking this
> as Verified for Beta 32.

That's Beta 30.

Comment 28

[Bobby Holley (:bholley)]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #26)
> The fix seems to have not made it to Nightly and Aurora yet, so marking this
> as Verified for Beta 32.

That's because this patch is behind a build-time flag that makes it only take effect on beta/release.

Comment 29

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Bobby Holley (:bholley) from comment #28)
> (In reply to Florin Mezei, QA (:FlorinMezei) from comment #26)
> > The fix seems to have not made it to Nightly and Aurora yet, so marking this
> > as Verified for Beta 32.
> 
> That's because this patch is behind a build-time flag that makes it only
> take effect on beta/release.

Removing the "verifyme" keyword... thanks Bobby.

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

Ryan - I overlooked the string change on Aurora approval here, can we backout the patch from Aurora with the string change and replace it with the Beta patch that doesn't have it?  

cc Bobby to make sure this still does fix properly on that branch

Comment 31

[Lukas Blakk [:lsblakk] use ?needinfo]

Apologies for the churn, it's now too late - locales have picked up the change.

Comment 32

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I've verified the fix in the latest Firefox 31 Beta 5 build (BuildID: 20140626181429).

I tested the same things as in comment 26:
1. Variations of 1and1.com from Mike's list in comment 8
- "window.controllers is deprecated. Do not use it for UA detection." displays for most of the pages on 31 Beta 5, but NO error as it happened for Firefox 30 beta 4
2. https://forschung.beuth-hochschule.de/bis/beuth from bug 1010137
- loads fine on 31 Beta 5

Comment 33

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I've verified the fix in the latest Firefox 32 Beta 8 build (BuildID: 20140818191513).

I tested the same things as in comment 26:
1. Variations of 1and1.com and other sites from Mike's list in comment 8
- "window.controllers is deprecated. Do not use it for UA detection." displays for most of the pages on 32 Beta 8, but NO error as it happened for Firefox 30 beta 4
2. https://forschung.beuth-hochschule.de/bis/beuth from bug 1010137
- loads fine on 32 Beta 8

Comment 34

[dewi]

Just so it's known: the EFF's "HTTPS Everywhere" add-on v4.0.2 is another possible test-case for this issue, since it gives the "window.controllers is deprecated" warning in FF 32.0.3, and in fact pages about this add-on comprise most of the first page of Google results for that search string.
This is a known issue in HTTPS Everywhere, and they are tracking it here:
https://github.com/EFForg/https-everywhere/issues/510

Comment 35

[moueza]

JS error on Firefox 47.0 console with Extension : En-têtes HTTP en direct 0.17.1-signed.1-signed .  Affiche les en-têtes HTTP des pages pendant votre navigation.
Par Daniel SAVARD http://livehttpheaders.mozdev.org

Comment 36

[moueza]

(In reply to moueza from comment #35)
> JS error on Firefox 47.0 console with Extension : En-têtes HTTP en direct
> 0.17.1-signed.1-signed .  Affiche les en-têtes HTTP des pages pendant votre
> navigation.
> Par Daniel SAVARD http://livehttpheaders.mozdev.org

window.controllers is deprecated. Do not use it for UA detection. nsHeaderInfo.js:412:8
Use of getPreventDefault() is deprecated.  Use defaultPrevented instead. jquery-1.8.3.min.js:2:40351

Comment 37

[Mike Taylor [:miketaylr]]

Hi moueza, it's probably best to report the issue to the add-on developers: http://livehttpheaders.mozdev.org/bugs.html Thanks!