334136 - Software Update fails with language packs

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect, P2)

Description

[Brian King [:kinger]]

I installed Firefox 1.5.0.1 ... the Slovene (sl-SI) version. Yesterday I was prompted to upgrade to 1.5.0.2. So I went for it. The XPI update failed, so it went ahead and installed the whole distro.

However, the version that installed was not sl-SI, but en-US.

The fact that I did the upgrade from a profile with the en-US langpack installed might have something to do with it, but then again may not.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

The update system uses the value of general.useragent.locale to determine the build to update to - if the langpack changes this pref, then you'll get the build corresponding to that locale.

Comment 2

[Benjamin Smedberg]

Gavin: that's the bug...

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Yes, I just thought I'd confirm it, seeing as how no one else had (comment 0 says "might have something to do with it, but then again may not").

Comment 4

[Benjamin Smedberg]

This is not going to make 1.8.0 or 1.8. I'll fix it on trunk with the work for a unified and unique buildid.

Comment 6

[Axel Hecht [:Pike]]

As I mentioned over in bug 370421, we could just add the locale explicitly to browserconfig.properties and use that instead of the currently selected locale.

I'm adjusting the description a bit, I would have expected "Software Update does not handle locales" to be a different bug. Though I confess that I didn't search for dupes, bad me. As I'm off-topic and dupe, my mind somehow marries this problem with getting the language packs up on AMO for updating, see bug 245946, I guess.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Depends on bug 313453 , because updating to another branch without updating the language pack would leave the profile unusable (locale has to be changed in the pref.js, otherwise chrome error on startup). Setting the locale to default would not be userfriendly.

Comment 10

[Benjamin Smedberg]

Why did you reasssign this to nobody?

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: gets the default locale for %LOCALE%

Maybe the blocker should be removed, Pike mentioned en-US fallback for non-compatible language packs is coded into the applications. For Sunbird 0.3 > 0.3.1 I'm pretty sure that the language packs had been disabled and the application did not fall back to en-US (had to change it with the editor). If I am wrong, remove it.

To the patch:
Unfortunately with the trunk source and there is no actual language pack available. An en-US build with a language pack with forced compatibility detects the update, bug pressing the restart button does nothing, maybe because of missing strings (the error console can't be accessed because of this).

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 268248 [details] [diff] [review]
gets the default locale for %LOCALE%

That's not necessarily as robust as I'd like it to be, as an add-on shipping that pref would still overwrite the default value.

And I still want langpacks to do that, and other approaches would do, too, such as bundling langpacks with existing builds. That's something that has been brought up recently, and the fix for this bug should pick that up.

IMHO, we need something keeping track of the built locale at packaging time and read that in.

Thus, r-'ing this, but thanks for taking a stab at it.

Benjamin, could you chime in with what your approach would be? Is comment 4 still up-to-date?

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: checks for install locale of browser instead of active language pack

browser.useragent.installedLocale is used instead of general.useragent.locale and represents the language of the application during the installation. Do you want another name (because there are non-browser apps) like app.useragent.installedLocale? At the momemt, app is nearly only used for update related things.

The patch isn't built or tested because of my slow machine, please excuse this. Expecting a r- to be faster.

Comment 14

[Axel Hecht [:Pike]]

Comment on attachment 270881 [details] [diff] [review]
checks for install locale of browser instead of active language pack

This still misses the interesting part, which is to make nsUpdateService.js.in:getLocale use the browserconfig.properties stringbundle directly.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&mark=4328-4330&rev=1.811#4320 has other code that does that.

I'm personally in favour of calling the entity app.installedLocalization or Language, the term locale is a tad overused, IMHO.

Hitting Benjamin with a design-review request for this.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 270881 [details] [diff] [review]
checks for install locale of browser instead of active language pack

forget a file, coming soon

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: included demanded changes

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 270884 [details] [diff] [review]
included demanded changes

Sorry, hadn't used a unique identifier for sr?

Comment 18

[Axel Hecht [:Pike]]

Comment on attachment 270884 [details] [diff] [review]
included demanded changes

Grr on me, I totally missed that this doesn't work for thunderbird at all, sorry for that.

Comment 19

[Axel Hecht [:Pike]]

Comment on attachment 268248 [details] [diff] [review]
gets the default locale for %LOCALE%

OK, it seems like default branch is at good as it gets and works independent of Firefox-isms.

Benjamin, sr?

Comment 20

[Steffen Wilberg]

Comment on attachment 268248 [details] [diff] [review]
gets the default locale for %LOCALE%

>+               var prefs = Components.classes["@mozilla.org/preferences-service           ;1"].
>+                               getService(Components.interfaces.nsIPrefService)           ;
You should get rid of the whitespace before the semicolons.
And this looks better if you align the dots like this:
  Components.classes[
            .getService(

Comment 21

[Axel Hecht [:Pike]]

Attachment: patch for check-in

Carrying over reviews, this is the patch I'm going to check in. I did the whitespace fix-ups that steffen wanted, one of which is sadly actually relevant.

Tested on my own build with app.update.log.Checker set to true, the update url sent is for en-US both with and without an activated polish language pack.

Comment 22

[Axel Hecht [:Pike]]

Checking in nsUpdateService.js.in;
/cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v  <--  nsUpdateService.js.in
new revision: 1.139; previous revision: 1.138
done

Attachment 272641 [details] [diff] checked in on trunk.

A good verification for this patch is to check that the nightly update channel still works.

Comment 23

[Axel Hecht [:Pike]]

Comment on attachment 268248 [details] [diff] [review]
gets the default locale for %LOCALE%

moving approval request over to the checked-in patch

Comment 24

[Axel Hecht [:Pike]]

Comment on attachment 268248 [details] [diff] [review]
gets the default locale for %LOCALE%

moving approval request over to the checked-in patch

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Verified with Windows XP and en-US Trunk with pl-PL language pack. But the update dialog "Firefox is installing ..." is en-US.

Comment 26

[Axel Hecht [:Pike]]

Baked on trunk for a bit now, VERIFIED. Time to request approval for the branch again.

Comment 27

[Axel Hecht [:Pike]]

Comment on attachment 272641 [details] [diff] [review]
patch for check-in

Requesting approval1.8.1.6. This fix is an important step in being able to more aggressively advocate testing of new localizations via language packs without breaking security updates.

The fix is small and verified with loads of dogfood nightly updates on the trunk.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 272641 [details] [diff] [review]
patch for check-in

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 31

[Axel Hecht [:Pike]]

Fixed on the branch, too.

Thanks to Archaeopteryx for the patch.

Comment 33

[Al Billings [:abillings - ex-MoCo]]

Axel, can you verify this on branch using the 2.0.0.8 RC2 build? If so, please change the "fixed1.8.1.8" keyword to "verified1.8.1.8".