Last Comment Bug 815743 - Backout the UA changes in bug 588909 and its follow-ups from all branches
: Backout the UA changes in bug 588909 and its follow-ups from all branches
Status: RESOLVED FIXED
: dev-doc-needed, verifyme
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla20
Assigned To: :Ehsan Akhgari
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Mentors:
Depends on: 845253
Blocks: 588909
  Show dependency treegraph
 
Reported: 2012-11-27 11:56 PST by :Ehsan Akhgari
Modified: 2013-12-27 14:32 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
verified
verified
+
fixed


Attachments
Remove all of the site specific user agent overrides as part of backing out bug 588909 (2.38 KB, patch)
2012-11-27 11:59 PST, :Ehsan Akhgari
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑release+
lukasblakk+bugs: approval‑mozilla‑esr17+
Details | Diff | Splinter Review
Backout bug 588909 (2.28 KB, patch)
2012-11-27 12:11 PST, :Ehsan Akhgari
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑release+
lukasblakk+bugs: approval‑mozilla‑esr17+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2012-11-27 11:56:32 PST
In the engineering meeting today, we made the call to proceed with backing this out.
Comment 1 :Ehsan Akhgari 2012-11-27 11:59:40 PST
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.
Comment 2 Lukas Blakk [:lsblakk] use ?needinfo 2012-11-27 12:02:05 PST
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 :Ehsan Akhgari 2012-11-27 12:11:42 PST
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...
Comment 4 :Ehsan Akhgari 2012-11-27 12:27:53 PST
I'm currently building these patches with and without the official branding to test them, and then I'll push them.
Comment 5 :Ehsan Akhgari 2012-11-27 12:54:04 PST
OK, the patches revert the changes to the UA string properly.  On to landing...
Comment 11 :Ehsan Akhgari 2012-11-27 13:12:43 PST
OK, we're done here.  Please ask the QA team to inspect this fix with due diligence.  Thanks!
Comment 13 :Ehsan Akhgari 2012-11-27 14:05:35 PST
(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 :Ehsan Akhgari 2012-11-27 14:06:06 PST
Ah, I meant to quote comment 12.
Comment 16 :Ehsan Akhgari 2012-11-27 14:10:55 PST
Backout from release: https://hg.mozilla.org/releases/mozilla-release/rev/4488e52d1ba8
Comment 18 Alex Keybl [:akeybl] 2012-11-27 14:30:59 PST
(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 :Ehsan Akhgari 2012-11-27 14:37:17 PST
Backed out from inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/a79e44ef6b0a
Comment 20 :Ehsan Akhgari 2012-11-27 14:37:39 PST
(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 :Ehsan Akhgari 2012-11-27 15:05:18 PST
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 26 :Ehsan Akhgari 2012-11-27 16:06:06 PST
Seems like the first tests are coming out green on release.
Comment 27 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-27 16:19:45 PST
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 2012-11-27 22:38:41 PST
(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 30 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-28 13:40:53 PST
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) 2012-11-28 21:01:11 PST
> 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 :Ehsan Akhgari 2012-11-28 21:18:50 PST
(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) 2012-11-28 21:27:37 PST
> 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] 2012-11-29 01:59:08 PST
(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 B [:simonab] 2012-11-29 05:01:18 PST
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. 2012-11-29 07:39:51 PST
(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 2012-12-01 14:43:42 PST
Does this mean there will be no further pruning of the user-agent string?
Comment 38 Alex Keybl [:akeybl] 2012-12-04 08:23:13 PST
(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] 2012-12-04 17:27:04 PST
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) 2012-12-04 17:45:25 PST
> "touch vs. tablet" question

Seems like a touchy question.

Note You need to log in before you can comment on or make changes to this bug.