Closed Bug 815743 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox17 + verified
firefox18 + verified
firefox19 + verified
firefox20 --- verified
firefox-esr17 + fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: dev-doc-needed, verifyme)

Attachments

(2 files)

In the engineering meeting today, we made the call to proceed with backing this out.
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+
Keywords: dev-doc-needed
Attached patch Backout bug 588909 — — Splinter Review
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...
OK, we're done here.  Please ask the QA team to inspect this fix with due diligence.  Thanks!
Keywords: qawanted
(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.
(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!
(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.
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
Closed: 12 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.
Keywords: qawantedverifyme
OS: Mac OS X → All
Hardware: x86 → All
> 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!
> 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.
(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)
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
> "touch vs. tablet" question

Seems like a touchy question.
Depends on: 845253
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: