Backout the UA changes in bug 588909 and its follow-ups from all branches

RESOLVED FIXED in Firefox 17

Status

()

Core
General
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Depends on: 1 bug, {dev-doc-needed, verifyme})

Trunk
mozilla20
dev-doc-needed, verifyme
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17+ verified, firefox18+ verified, firefox19+ verified, firefox20 verified, firefox-esr17+ fixed)

Details

Attachments

(2 attachments)

In the engineering meeting today, we made the call to proceed with backing this out.
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
tracking-firefox17: --- → +
tracking-firefox18: --- → +
tracking-firefox19: --- → +
Created attachment 685761 [details] [diff] [review]
Remove all of the site specific user agent overrides as part of backing out bug 588909

This patch backs out all of the overrides landed as follow-ups of bug 588909.
Attachment #685761 - Flags: approval-mozilla-release?
Attachment #685761 - Flags: approval-mozilla-esr17?
Attachment #685761 - Flags: approval-mozilla-beta?
Attachment #685761 - Flags: approval-mozilla-aurora?
Comment on attachment 685761 [details] [diff] [review]
Remove all of the site specific user agent overrides as part of backing out bug 588909

We'll be taking this for 17.0.1 - approving for all branches.
Attachment #685761 - Flags: approval-mozilla-release?
Attachment #685761 - Flags: approval-mozilla-release+
Attachment #685761 - Flags: approval-mozilla-esr17?
Attachment #685761 - Flags: approval-mozilla-esr17+
Attachment #685761 - Flags: approval-mozilla-beta?
Attachment #685761 - Flags: approval-mozilla-beta+
Attachment #685761 - Flags: approval-mozilla-aurora?
Attachment #685761 - Flags: approval-mozilla-aurora+

Updated

5 years ago
Keywords: dev-doc-needed
Created attachment 685767 [details] [diff] [review]
Backout bug 588909

The changes to config/autoconf.mk.in in the original patch did not apply cleanly in this patch.  I'm building locally to see if that's ok...
Attachment #685767 - Flags: approval-mozilla-release?
Attachment #685767 - Flags: approval-mozilla-esr17?
Attachment #685767 - Flags: approval-mozilla-beta?
Attachment #685767 - Flags: approval-mozilla-aurora?
Attachment #685767 - Flags: approval-mozilla-release?
Attachment #685767 - Flags: approval-mozilla-release+
Attachment #685767 - Flags: approval-mozilla-esr17?
Attachment #685767 - Flags: approval-mozilla-esr17+
Attachment #685767 - Flags: approval-mozilla-beta?
Attachment #685767 - Flags: approval-mozilla-beta+
Attachment #685767 - Flags: approval-mozilla-aurora?
Attachment #685767 - Flags: approval-mozilla-aurora+
I'm currently building these patches with and without the official branding to test them, and then I'll push them.
OK, the patches revert the changes to the UA string properly.  On to landing...
https://hg.mozilla.org/releases/mozilla-release/rev/6ceaf38164ce
https://hg.mozilla.org/releases/mozilla-release/rev/0ad43828f188
status-firefox17: affected → fixed
status-firefox-esr17: --- → affected
tracking-firefox-esr17: --- → ?
https://hg.mozilla.org/releases/mozilla-beta/rev/9bf0f2cdb1b2
https://hg.mozilla.org/releases/mozilla-beta/rev/06930f8a4972
status-firefox18: affected → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/aa39d72e59b0
https://hg.mozilla.org/releases/mozilla-aurora/rev/7532ac3be0d8
status-firefox19: affected → fixed
https://hg.mozilla.org/releases/mozilla-esr17/rev/efbb9bc02a31
https://hg.mozilla.org/releases/mozilla-esr17/rev/17b0a56b0482
status-firefox-esr17: affected → fixed
https://hg.mozilla.org/integration/mozilla-inbound/rev/08192df425cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6130f6eeaca
status-firefox20: affected → fixed
tracking-firefox20: --- → ?
OK, we're done here.  Please ask the QA team to inspect this fix with due diligence.  Thanks!
Keywords: qawanted
Backed out because of leaks:

https://hg.mozilla.org/releases/mozilla-beta/rev/ab7e9feac7b8
https://tbpl.mozilla.org/?tree=Mozilla-Beta&rev=06930f8a4972
status-firefox18: fixed → affected
(In reply to Ehsan Akhgari [:ehsan] from comment #11)
> OK, we're done here.  Please ask the QA team to inspect this fix with due
> diligence.  Thanks!

We might need to remove/rework the whitelisting code, since that is the only thing that I can see causing these leaks here.  Lukas, can you find another owner for that, please?
Ah, I meant to quote comment 12.
https://hg.mozilla.org/releases/mozilla-aurora/rev/e52f368c8f6b
status-firefox17: fixed → affected
Backout from release: https://hg.mozilla.org/releases/mozilla-release/rev/4488e52d1ba8
status-firefox19: fixed → affected
https://hg.mozilla.org/releases/mozilla-esr17/rev/655a06ceca7c
status-firefox-esr17: fixed → affected
(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> https://hg.mozilla.org/releases/mozilla-esr17/rev/655a06ceca7c

Can we also land on GECKO170_2012111914_RELBRANCH for mozilla-esr17? I see that we've had other security landings since 17.0. Thanks!
Backed out from inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/a79e44ef6b0a
(In reply to Alex Keybl [:akeybl] from comment #18)
> (In reply to Ehsan Akhgari [:ehsan] from comment #17)
> > https://hg.mozilla.org/releases/mozilla-esr17/rev/655a06ceca7c
> 
> Can we also land on GECKO170_2012111914_RELBRANCH for mozilla-esr17? I see
> that we've had other security landings since 17.0. Thanks!

Sure, once we have a fix that doesn't leak.  :-)
Setting the general.useragent.complexOverride.moodle pref to false instead of removing it seems to fix the leaks and test failures.  This code <http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#382> reads the pref unconditionally and it will throw if the pref does not exist.
https://hg.mozilla.org/releases/mozilla-release/rev/691d7b214724
https://hg.mozilla.org/releases/mozilla-release/rev/57899cae65c7
status-firefox17: affected → fixed
default:

https://hg.mozilla.org/releases/mozilla-esr17/rev/228ed4f15621
https://hg.mozilla.org/releases/mozilla-esr17/rev/e770ee07f032

GECKO170_2012111914_RELBRANCH relbranch:

https://hg.mozilla.org/releases/mozilla-esr17/rev/06f0e2b9f4f9
https://hg.mozilla.org/releases/mozilla-esr17/rev/ba1984563174
status-firefox-esr17: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/2cc6dc1c0e21
https://hg.mozilla.org/releases/mozilla-beta/rev/8502523b15be
status-firefox18: affected → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/a2d4840f1bfe
https://hg.mozilla.org/releases/mozilla-aurora/rev/733911b204b1
status-firefox19: affected → fixed
Seems like the first tests are coming out green on release.
Taking QA ownership to coordinate testing once we have builds to test. Apart from doing a broad sweep of web compatibility testing, is there anything we should pay particular attention to?
QA Contact: anthony.s.hughes
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #27)
> Taking QA ownership to coordinate testing once we have builds to test. Apart
> from doing a broad sweep of web compatibility testing, is there anything we
> should pay particular attention to?

Please check on the STR in bug 797703 as well as checking as many sites listed in attachment 685761 [details] [diff] [review] as you can.
https://hg.mozilla.org/mozilla-central/rev/03e4b50b8d23
https://hg.mozilla.org/mozilla-central/rev/30cd23db6461
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Using the following test from MattN:

Firefox
* UA string now contains Gecko/20100101 (instead of Gecko/17.0)
* general.useragent.complexOverride.moodle defaults to false

http://www.opensourcecms.com/scripts/details.php?name=Moodle&scriptid=137
1) Click Demo Admin Page button
2) Log in with admin/Demo123!
3) Select Edit Settings
> Full page description textarea should be a rich-text editor

http://dev.moodle.org
1) Clear all cookies
2) Login at https://moodle.org/login/index.php (mozqatest/Mozilla123!)
3) Visit https://moodle.org/network/ which will list the other Moodle installs that are connected with Moodle.org via MNet
4) Click on "Moodle Developer Courses" (http://dev.moodle.org)
> You should still be logged in, not receive a "connection failed" message

Using all these steps I confirm that Firefox 17.0 reproduces the bug. Firefox 20.0a1, 19.0a2, and 18.0b2#1 do not.

Flagging for verification with 17.0.1 and 17.0.1esr once we have candidate builds.
status-firefox18: fixed → verified
status-firefox19: fixed → verified
status-firefox20: fixed → verified
Keywords: qawanted → verifyme
OS: Mac OS X → All
Hardware: x86 → All

Comment 31

5 years ago
> In the engineering meeting today, we made the call to proceed with backing this out.

Given that this is an open source project, please:
* Give reasons why you think that must be backed out
* Then allow this to be discussed here, including alternative solutions how the stated problems can be fixed (evang, etc.)
* Then let the person in charge (Gerv, as far as I know) make the decision, based on these rational arguments.

This change was prepared for months, and a lot of effort went into it to make it work. We knew we'd have some small problems, and we do. Just backing it out within a day without even a disucssion is not fair to the efforts.
(In reply to comment #31)
> > In the engineering meeting today, we made the call to proceed with backing this out.
> 
> Given that this is an open source project, please:
> * Give reasons why you think that must be backed out
> * Then allow this to be discussed here, including alternative solutions how the
> stated problems can be fixed (evang, etc.)
> * Then let the person in charge (Gerv, as far as I know) make the decision,
> based on these rational arguments.

Sorry that you got the wrong impression about this.  This decision was not made on a secret meeting, it happened during out public weekly engineeting call <https://wiki.mozilla.org/Platform#Meetings>.  You can see the minutes here <https://wiki.mozilla.org/Platform/2012-11-27>.

> This change was prepared for months, and a lot of effort went into it to make
> it work. We knew we'd have some small problems, and we do. Just backing it out
> within a day without even a disucssion is not fair to the efforts.

I understand that we put every effort into making sure that this change makes it into the release channel, but we found out from Firefox 17 feedback that this breaks a number of banking websites and also a number of small web applications using FCVEditor to provide rich text editing capabilities.  There wasn't really anything that we can do to work around this (since this did not affect a small number of websites, we could not add whitelist prefs for them) and we had a very limited time (Firefox 17.0.1 is building as we speak) so we had to make a tough call and give up trying this time around.  :/

(That all said, I was mostly the person doing the backout mechanics, I'm sure others can fill you with better information on the details.)

Sorry that this might have came off the wrong way!

Comment 33

5 years ago
> You can see the minutes here <https://wiki.mozilla.org/Platform/2012-11-27>.

Thank you. That's valuable.

> give up trying this time around.  :/

OK. I take it that this we can try it again later on, in, say, a year.
(In reply to Ben Bucksch (:BenB) from comment #31)
> Given that this is an open source project, please:
> * Give reasons why you think that must be backed out
> * Then allow this to be discussed here, including alternative solutions how
> the stated problems can be fixed (evang, etc.)
> * Then let the person in charge (Gerv, as far as I know) make the decision,
> based on these rational arguments.

It could be that the process here is not quite as transparent as it could be, although Ehsan has linked to minutes, so that's good. But I should say that I submitted a report on the incompatibilities found so far to that meeting (I wasn't able to attend myself) and do not oppose the decision to back the change out. I think it's disappointing, but we did hit more problems than we expected, and they are of a nature that means in-product mitigation is not possible. (We can't detect instances of FCKeditor and other rich text systems with any sort of decent reliability.)

For a partial overview of the problems encountered, look at the dependency tree of bug 588909.

As for trying again later on, I think a year is probably too soon.

Gerv
Verified as fixed on Firefox 17.0.1 - verified using the test from Comment 30 on Windows 7, Ubuntu 12.10 and Mac OS X 10.8.
status-firefox17: fixed → verified

Comment 36

5 years ago
(In reply to Gervase Markham [:gerv] from comment #34)
> (We can't detect instances of FCKeditor and
> other rich text systems with any sort of decent reliability.)

Opera's "browser.js" file does a lot of such things with success
https://github.com/operasoftware/browserjs/blob/master/desktop/browserjs-12.10.js
(but for various reasons such a thing is not an option for Mozilla imho)

> As for trying again later on, I think a year is probably too soon.

seems way to soon. But shouldn't there be at least a strategy or plan to prevent people writing *new* code relying on the frozen build date? (a pious hope)
tracking-firefox20: ? → ---
tracking-firefox-esr17: ? → +

Comment 37

5 years ago
Does this mean there will be no further pruning of the user-agent string?
(In reply to David E. Ross from comment #37)
> Does this mean there will be no further pruning of the user-agent string?

None planned for the next 3 releases at least.
That's not totally true; there's a non-zero chance we may do something about the "touch vs. tablet" question. But that won't have any effect on desktop (or most mobile).

Gerv

Comment 40

5 years ago
> "touch vs. tablet" question

Seems like a touchy question.

Updated

4 years ago
Depends on: 845253
You need to log in before you can comment on or make changes to this bug.