Closed Bug 1010577 Opened 6 years ago Closed 6 years ago

Add back window.controllers for site compatibility

Categories

(Core :: Security: CAPS, defect, major)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox29 + wontfix
firefox30 + verified
firefox31 + verified
firefox32 + verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: kohei, Assigned: bholley)

References

Details

(Keywords: dev-doc-needed, site-compat)

Attachments

(2 files)

+++ 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
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
Another 29.0.2 might be needed because this is breaking many sites.
Severity: normal → major
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....
Summary: Take back window.controllers for site compatibility → Add back window.controllers for site compatibility
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.)
Flags: needinfo?(bzbarsky)
Karl, see bug 968923.  I assume that we'll get that landed in the near future, once Cameron gets back.
Flags: needinfo?(bzbarsky)
(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)
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.
Bobby, do you have the bandwidth to deal with this?
Flags: needinfo?(jst)
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
We could run the test on RELEASE_BUILD only and have the uplift folks hate us... ;)
(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.
Assignee: nobody → bobbyholley
Keywords: verifyme
> 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 on attachment 8424182 [details] [diff] [review]
Shim window.controllers (with a warning) in RELEASE_BUILDs. v1

Nice.  r=me
Attachment #8424182 - Flags: review?(bzbarsky) → review+
Attaching a patch for beta without the string changes.
Attachment #8425030 - Flags: review+
Flags: needinfo?(jst)
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
Attachment #8424182 - Flags: approval-mozilla-aurora?
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).
Attachment #8424182 - Flags: approval-mozilla-beta?
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.
Attachment #8424182 - Flags: approval-mozilla-beta?
Attachment #8425030 - Flags: approval-mozilla-beta?
Attachment #8424182 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8425030 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/cd8b2f03148b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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.
> 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.
(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.
(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.
Keywords: verifyme
Flags: needinfo?(ryanvm)
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
Flags: needinfo?(bobbyholley)
Apologies for the churn, it's now too late - locales have picked up the change.
Flags: needinfo?(ryanvm)
Flags: needinfo?(bobbyholley)
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
Depends on: 1031988
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
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
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
(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
Hi moueza, it's probably best to report the issue to the add-on developers: http://livehttpheaders.mozdev.org/bugs.html Thanks!
You need to log in before you can comment on or make changes to this bug.