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

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(no longer active)]

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

Comment 1

[(no longer active)]

Attachment: 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.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 3

[(no longer active)]

Attachment: 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...

Comment 4

[(no longer active)]

I'm currently building these patches with and without the official branding to test them, and then I'll push them.

Comment 5

[(no longer active)]

OK, the patches revert the changes to the UA string properly.  On to landing...

Comment 6

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-release/rev/6ceaf38164ce
https://hg.mozilla.org/releases/mozilla-release/rev/0ad43828f188

Comment 7

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9bf0f2cdb1b2
https://hg.mozilla.org/releases/mozilla-beta/rev/06930f8a4972

Comment 8

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aa39d72e59b0
https://hg.mozilla.org/releases/mozilla-aurora/rev/7532ac3be0d8

Comment 9

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-esr17/rev/efbb9bc02a31
https://hg.mozilla.org/releases/mozilla-esr17/rev/17b0a56b0482

Comment 10

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08192df425cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6130f6eeaca

Comment 11

[(no longer active)]

OK, we're done here.  Please ask the QA team to inspect this fix with due diligence.  Thanks!

Comment 12

[(no longer active)]

Backed out because of leaks:

https://hg.mozilla.org/releases/mozilla-beta/rev/ab7e9feac7b8
https://tbpl.mozilla.org/?tree=Mozilla-Beta&rev=06930f8a4972

Comment 13

[(no longer active)]

(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?

Comment 14

[(no longer active)]

Ah, I meant to quote comment 12.

Comment 15

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e52f368c8f6b

Comment 16

[(no longer active)]

Backout from release: https://hg.mozilla.org/releases/mozilla-release/rev/4488e52d1ba8

Comment 17

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-esr17/rev/655a06ceca7c

Comment 18

[Alex Keybl [:akeybl]]

(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!

Comment 19

[(no longer active)]

Backed out from inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/a79e44ef6b0a

Comment 20

[(no longer active)]

(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.  :-)

Comment 21

[(no longer active)]

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.

Comment 22

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-release/rev/691d7b214724
https://hg.mozilla.org/releases/mozilla-release/rev/57899cae65c7

Comment 23

[(no longer active)]

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

Comment 24

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2cc6dc1c0e21
https://hg.mozilla.org/releases/mozilla-beta/rev/8502523b15be

Comment 25

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a2d4840f1bfe
https://hg.mozilla.org/releases/mozilla-aurora/rev/733911b204b1

Comment 26

[(no longer active)]

Seems like the first tests are coming out green on release.

Comment 27

[u279076]

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?

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

(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.

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/03e4b50b8d23
https://hg.mozilla.org/mozilla-central/rev/30cd23db6461

Comment 30

[u279076]

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.

Comment 31

[Ben Bucksch (:BenB)]

> 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.

Comment 32

[(no longer active)]

(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

[Ben Bucksch (:BenB)]

> 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.

Comment 34

[Gervase Markham [:gerv]]

(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

Comment 35

[Simona Badau, Desktop QA]

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.

Comment 36

[j.j.]

(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)

Comment 37

[David E. Ross]

Does this mean there will be no further pruning of the user-agent string?

Comment 38

[Alex Keybl [:akeybl]]

(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.

Comment 39

[Gervase Markham [:gerv]]

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

[Ben Bucksch (:BenB)]

> "touch vs. tablet" question

Seems like a touchy question.