Closed Bug 1450270 Opened 2 years ago Closed 2 years ago

Switch away from general.useragent.locale broke all Acer language specific preloads

Categories

(Firefox :: Distributions, defect, P1, critical)

defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox59 blocking wontfix
firefox60 + verified
firefox61 + verified

People

(Reporter: mkaply, Assigned: mkaply)

Details

Attachments

(1 file)

The switch to intl.locale.requested from general.useragent.locale broke all non-US Acer builds.

The reason is that they set the general.useragent.locale preference as a default preference and the migration code only handles user preferences.

They ship all the locales and then set the default in distribution.ini.

I'm going to add a workaround to distribution.js for this.
[Tracking Requested - why for this release]: If we do a chemspill, we need this. It's a huge impact for our partner.
Come to find out they are setting it in the [Preferences] section of distribution.ini which makes things much easier.
Sounds like this has a pretty broad user impact. Tracking as a blocker for 59 (meaning that we will consider 59 dot release for it).   

How did we (or Acer) miss this issue in QA?  And, how will we know through testing that we've got it right in the fix?
Flags: needinfo?(mozilla)
> How did we (or Acer) miss this issue in QA?

I can't speak for Acer, but I don't think the l10n team expected the general.useragent.locale to be a default pref, only a user set pref. So the patch they did didn't account for it.

The Acer stuff was before I was at Mozilla, so I didn't know about their specific scenario.

> And, how will we know through testing that we've got it right in the fix?

I have sample distribution.ini files from Acer for the failing case so I know exactly what was happening and why.

I'll investigate if I can create an automated test for it.
Flags: needinfo?(mozilla)
Comment on attachment 8963971 [details]
Bug 1450270 - Special case general.useragent.locale in distributions.

https://reviewboard.mozilla.org/r/232786/#review238262

This should work, although for the record - `intl.locale.requested` has different characteristics than `general.useragent.locale` had.

In particular it can take one of three types of values:

https://searchfox.org/mozilla-central/source/intl/locale/LocaleService.cpp#105

which is something the old pref was not designed to handle.

In particular, I'm wondering if setting the pref to an empty string (in order to make us follow the OS locale) may be a better solution since we have a large number of langpacks installed in that system?
Attachment #8963971 - Flags: review?(gandalf) → review+
> In particular, I'm wondering if setting the pref to an empty string (in order to make us follow the OS locale) may be a better solution since we have a large number of langpacks installed in that system?

There must be some reason they chose not to use matchOS. Unfortunately I wasn't around at the time, so I can't speak to that decision.
fair enough.

I'd like to suggest talking to them about testing beta release of a product before they deploy it to their customers. It would give us a chance to work through this issue on much better terms than chemspills.

As I said when we made the change we migrated all the known paths in our systems and this belongs to "dark matter" scope of setups that can only be discovered when the third-party starts being affected. If they don't test beta, it'll hit us on release :(
If we can target a system add-on to just their distribution ID, that might be a quicker way to get a fix to affected users.
> If we can target a system add-on to just their distribution ID, that might be a quicker way to get a fix to affected users.

I'd be more than happy to try to make that happen. How do I even kick that off?
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/74826e993054
Special case general.useragent.locale in distributions. r=gandalf
https://hg.mozilla.org/mozilla-central/rev/74826e993054
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment on attachment 8963971 [details]
Bug 1450270 - Special case general.useragent.locale in distributions.

Approval Request Comment
[Feature/Bug causing the regression]: Switch of general.useragent.locale
[User impact if declined]: Locales don't work for acer
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Non
[Is the change risky?]: No
[Why is the change risky/not risky?]: Distribution only
[String changes made/needed]: None

I'd like to preemptively put this on the release in case we do a dot release.
Attachment #8963971 - Flags: approval-mozilla-release?
Attachment #8963971 - Flags: approval-mozilla-beta?
Comment on attachment 8963971 [details]
Bug 1450270 - Special case general.useragent.locale in distributions.

Let's get this into 60.0b9 anyway.
Attachment #8963971 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8963971 [details]
Bug 1450270 - Special case general.useragent.locale in distributions.

Let's get this onto m-r in case we have another 59 release.
Attachment #8963971 - Flags: approval-mozilla-release? → approval-mozilla-release+
Flags: qe-verify?
Release QA would be interested to work on this sooner than later, if manual testing can be of value here. Mike, could you please help us define a test plan?
Flags: needinfo?(mozilla)
Sure. To test this, you need to have a language pack installed into Firefox other than the default.

Create a directory called distribution in the same directory as the Firefox executable on Windows.

Place the contents of this file in that directory.

[Global]
id=acer-002
version=2.0
about=Mozilla Firefox for Acer

[XRE]
EnableProfileMigrator=false

[Preferences]
app.distributor="acer"
app.distributor.channel=""
mozilla.partner.id="acer"
browser.shell.checkDefaultBrowser=false
browser.usedOnWindows10=true
extensions.partnerdefaults.checkDefaultDelay=0
extensions.partnerdefaults.checkDefaultDays=2
extensions.partnerdefaults.firstRunDone=false
extensions.partnerdefaults.showBookmarksToolbar=false

[LocalizablePreferences]
browser.search.defaultenginename="Bing"
browser.search.order.1="Bing"
browser.search.order.2="Yahoo"
browser.search.order.3="Google"
browser.startup.homepage="http://acer17win10.msn.com/?pc=ACTE"
startup.homepage_welcome_url="about:blank"
startup.homepage_welcome_url.additional="about:blank"
browser.usedOnWindows10.introURL="about:blank"

[Preferences]
general.useragent.locale="zh-CN"
browser.tabs.drawInTitlebar=false



Modify the line

general.useragent.locale="zh-CN"

To be the non default locale you installed.

Verify that when you start the browser, the non default locale is now the default.

You can also check in about:config to verify that

intl.locale.requested 
is set
NOT general.useragent.locale
Flags: needinfo?(mozilla)
Thanks a lot for taking the time to write down this information, Mike! We'll start testing it as soon as possible.
Flags: qe-verify? → qe-verify+
I managed to reproduce the bug using Firefox 59.0.2 on Windows 10 x64. After I did all the steps from comment 19, the language didn't change, the pref intl.locale.requested wasn't set to ro (I installed the ro language pack), only general.useragent.locale was set. 

I retested everything using the same methods on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13. The builds I used were beta 60.0b9 and latest Nightly. The bug is not reproducing anymore. In these builds the language changes (you have to wait a few seconds ~30s wihtout restarting the browser) and the pref intl.locale.requested is changed to ro and not the other one.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Comment on attachment 8963971 [details]
Bug 1450270 - Special case general.useragent.locale in distributions.

m-r is on Fx60 now which we're shipping next week
Attachment #8963971 - Flags: approval-mozilla-release+ → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.