644933 - [ACR Extension] Blacklist incompatible language packs, they prevent firefox from starting

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Compatibility Tools, defect, P1)

Description

[Gummi]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0) Gecko/20100101 Firefox/4.0

Language packs for different firefox versions are never compatible, and they tend to prevent firefox from starting with XUL XML parser errors. The addon compatibility reporter shouldn't override compatibility for these, or if it does, it should disable them first.

Reproducible: Always

Steps to Reproduce:
Steps (these are the upgrade steps I followed; it can probably be done with vanilla firefox tarballs as well): Install the addon compatibility reporter. Install language-pack-en-base and enable the language pack. Install firefox from ppa:mozillateam/firefox-stable .
Actual Results:  
On start up, Firefox displays a red xml parser error instead of the UI.

Expected Results:  
Firefox starts normally.

Launching `firefox -uilocale en`, and disabling the language pack from the addon manager's language pane, allows firefox to start again.

Comment 1

[Brian King [:kinger]]

Can't turn compat prefs on/off for any particular type. This one could be tricky.

Blair / Dave, any ideas?

Comment 2

[Dave Townsend [:mossop]]

Yeah, disabling the locale packs is the only thing I can think of, maybe if you do that early enough in startup it'll work, not sure though.

Comment 3

[David McNamara [:mackers]]

r94029 has a potential fix for this.

When ACR detects that a major application upgrade has occurred, it will *uninstall* all locales, and then restart the browser. This will mean that Firefox can at least start with a working UI.

For some reason, merely disabling the locales does not have any effect. I suspect this is because the addon manager is not yet watching for addon disables when we need to disable it (when the broken UI comes up), but uninstalling the addon will trigger the uninstall immediately.

I'm not sure it is desireable that we uninstall all locales in this instance, but this behaviour must be better than seeing a broken UI.

I am also looking for some clarification on locale vs langpack in this regard, as the AddonManager returns "locale" as the addon type, regardless.

Comment 4

[Brian King [:kinger]]

cc'ing l10n FYI.

Marking as fixed as we have a solution. If anyone is not happy with it, please reopen.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is a kinda rough method to get rid of the problem. Especially for users who switch between different versions of Firefox, i.e testing purposes. Those would always have to install the language pack again and again when using the same profile.

DaveT, do you have some ideas re comment 3?

Comment 6

[Axel Hecht [:Pike]]

The mobile folks work on infrastructure to keep language packs compatible on nightlies. Mark, my bug foo is weak today, which is the best bug to point to right now?

Comment 7

[David McNamara [:mackers]]

For a definition of what ACR currently defines 'application upgrade', see bug 527249.

Comment 8

[Dave Townsend [:mossop]]

(In reply to David McNamara [:mackers] from comment #3)
> r94029 has a potential fix for this.
> 
> When ACR detects that a major application upgrade has occurred, it will
> *uninstall* all locales, and then restart the browser. This will mean that
> Firefox can at least start with a working UI.
> 
> For some reason, merely disabling the locales does not have any effect. I
> suspect this is because the addon manager is not yet watching for addon
> disables when we need to disable it (when the broken UI comes up), but
> uninstalling the addon will trigger the uninstall immediately.

This is a little surprising, not sure why that'd be the case. 

> I'm not sure it is desireable that we uninstall all locales in this
> instance, but this behaviour must be better than seeing a broken UI.

As I understand things the UI by default uses the system locale and attempts to find chrome packages registered for that, however my understanding is that there are prefs you can set to override this and use a specific locale. Perhaps you can just set these prefs on startup to force it to use the app-shipped locale for that startup while you handle disabling the other locale packs.

> I am also looking for some clarification on locale vs langpack in this
> regard, as the AddonManager returns "locale" as the addon type, regardless.

As far as the add-on manager is concerned we have an extension type "locale" which ships localized strings. Langpacks are all of this type. Technically it is possible to ship localizations for only a part of the UI in a "locale" xpi, but I don't think this ever happens in a reality so there is basically a 1:1 mapping here.

Comment 9

[David McNamara [:mackers]]

This feature has been disabled for ACR 0.9.

Re-opening to readdress another day.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Axel Hecht [:Pike] from comment #6)
> The mobile folks work on infrastructure to keep language packs compatible on
> nightlies. Mark, my bug foo is weak today, which is the best bug to point to
> right now?

We have a few bugs for Fennec. The original bug was bug 63467, which basicially states the problem - using langpacks XPIs for nightlies means the ngihtly could be busted (yellow screen of death) if a langpack is not up to date.

Bug 678111 outlines some work RelEng is doing to keep langpacks separated by buildid.Fennec will check for langpacks compatible with the new app buildid. If it finds some, they are downloaded with the app update. If we don't find any, we uninstall those langpacks.

Comment 11

[Brian King [:kinger]]

(In reply to Dave Townsend (:Mossop) from comment #8)
> > For some reason, merely disabling the locales does not have any effect. I
> > suspect this is because the addon manager is not yet watching for addon
> > disables when we need to disable it (when the broken UI comes up), but
> > uninstalling the addon will trigger the uninstall immediately.
> 
> This is a little surprising, not sure why that'd be the case. 

David, could you post the code you tried. Maybe there is a minor error or oversight.

Comment 12

[Brian King [:kinger]]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> If it finds some, they are downloaded with the app update. If we don't find
> any, we uninstall those langpacks.

The update check and download is probably something we could do as well, but there is still that uninstall part that noone seems to like. I like Mossop's idea of reverting to the app-shipped locale. The pref is general.useragent.locale

Comment 13

[Axel Hecht [:Pike]]

You want to set general.useragent.locale to default, and, if it exists, intl.locale.matchOS, too. The latter exists today, but there are plans to phase that out, so you want to be prepared for that just not being there.

Comment 14

[David McNamara [:mackers]]

(In reply to Brian King (Briks) [:kinger] from comment #11)
> David, could you post the code you tried. Maybe there is a minor error or
> oversight.

In essence the code is the following:

ACR.Util.getInstalledExtensions(function(installedExtensions) {
    for (var i=0; i<installedExtensions.length; i++) {
        if (installedExtensions[i].type == "locale") {
            //installedExtensions[i].userDisabled = true;
            installedExtensions[i].uninstall();
        }                                          
    }
});

In the above, 'uninstall()' works as expected while 'userDisabled = true' has no effect.

Note, ACR restarts the application as soon as it finishes uninstalling/disabling extensions. This may be why the disable does not complete.

A possible solution is to set up extension disable listeners and only to restart the app once all those listeners have been notified.

Comment 15

[Dave Townsend [:mossop]]

(In reply to David McNamara [:mackers] from comment #14)
> (In reply to Brian King (Briks) [:kinger] from comment #11)
> > David, could you post the code you tried. Maybe there is a minor error or
> > oversight.
> 
> In essence the code is the following:
> 
> ACR.Util.getInstalledExtensions(function(installedExtensions) {
>     for (var i=0; i<installedExtensions.length; i++) {
>         if (installedExtensions[i].type == "locale") {
>             //installedExtensions[i].userDisabled = true;
>             installedExtensions[i].uninstall();
>         }                                          
>     }
> });
> 
> In the above, 'uninstall()' works as expected while 'userDisabled = true'
> has no effect.
> 
> Note, ACR restarts the application as soon as it finishes
> uninstalling/disabling extensions. This may be why the disable does not
> complete.

When you say that, what do you mean? The code above doesn't show the restart call inside the anonymous function which it is where I would expect it to have to be in order to only happen after the async add-on retrieval has completed.

> A possible solution is to set up extension disable listeners and only to
> restart the app once all those listeners have been notified.

Setting userDisabled should (currently) behave synchronously, so the disable events should be sent before that line even completes.

Comment 16

[David McNamara [:mackers]]

(In reply to Dave Townsend (:Mossop) from comment #15)
> Setting userDisabled should (currently) behave synchronously, so the disable
> events should be sent before that line even completes.

I'm seeing the correct behaviour now and I can't reproduce the previous erroneous behaviour.

r96097 of ACR has this feature enabled. Any major application upgrade will cause the ACR to:

1. Disable all locales.
2. Reset the 'general.useragent.locale' and 'intl.locale.matchOS' prefs
3. Restart the application.

Tested with Breton langpack on a Firefox 3.6 -> 7.0 upgrade.

Comment 17

[Reşat SABIQ (Reshat)]

I ran into a related issue and logged bug 701631.

I'd appreciate your feedback on the following:
i. Has the fix from comment 16 been included in Firefox 8?
ii. Is the following statement correct based on fix referred to in comment 16?
Even if there's a remote version of the lang pack that is compatible with the upgraded app (say 7.0 upgraded to 8.0), then it will NOT be downloaded, and steps 1., 2., 3. from comment 16 will still be performed.
iii. (In reply to Mark Finkle (:mfinkle) from comment #10)
> Bug 678111 outlines some work RelEng is doing to keep langpacks separated by
> buildid.Fennec will check for langpacks compatible with the new app buildid.
> If it finds some, they are downloaded with the app update. If we don't find
> any, we uninstall those langpacks.
Is buildid idea something that's happening or not? From the install.rdf viewpoint i'm only aware of minVersion and maxVersion. Will they be sufficient in the foreseeable future? Just curious how this buildid idea could affect a lang pack like the following, for instance:
https://addons.mozilla.org/en-US/firefox/addon/QIRIMTATARCA-til-destesi/

P.S. If the answer to question ii. above is yes, i think it's really not user-friendly. In the best case scenario, the user'd have to get another addon update prompt some time later and re-start. I hope the answer is no, cause non-lang-pack addons are updated during an app upgrade, and it should work the same for langpacks IMHO.

Comment 18

[Brian King [:kinger]]

(In reply to Reşat SABIQ (Reshat) from comment #17)
> i. Has the fix from comment 16 been included in Firefox 8?

That fix is in the ACR extension, not Firefox. ACR 1.0 with the fix will be released very soon.