1732676 - Firefox doesn't fetch updated language pack

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Francesco Lodolo [:flod]]

To reproduce, install Firefox 92.0.1, then install this French language pack (version 92.0buildid20210903235534).

Then manually search for updates in about:addons.

AMO correctly provides a link to the updated langpack (version 92.0buildid20210922161155).

But Firefox doesn't install, apparently because code bails out here
https://searchfox.org/mozilla-central/rev/9e2239eb75e16204a6186c26f80aaa9723420919/toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm#582-584

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Adding needinfo assigning to my self, I'm looking into it and will comment with some more details.

In the meantime, the following test task can reproduce the unexpected Services.vc.compare that is making the update check to bail out:

add_task(async function test_vc_compare() {
  equal(
    Services.vc.compare(
      "92.0buildid20210903235534" /* current langpack version */,
      "92.0buildid20210922161155" /* new version got from the update */
    ),
    -1,
    "An update should have been detected"
  );
});

I've recorded this small test case with rr and I'm looking into what is making mozilla::CompareVersions to return 0 with those version strings.

Comment 2

[Rob Wu [:robwu]]

The underlying mechanism (compare of nsIVersionComparator) seems to support numbers up to 2^32-1. The buildID exceeds that limit, and consequently it is parsed as 0. This could be fixed at the Firefox side by supporting larger ranges, or by the langpack generator to cut the large number in two parts (or just truncating it).

• The compare implementation is at https://searchfox.org/mozilla-central/rev/9e2239eb75e16204a6186c26f80aaa9723420919/xpcom/base/nsVersionComparatorImpl.cpp#14
• actual comparison is done here: https://searchfox.org/mozilla-central/rev/9e2239eb75e16204a6186c26f80aaa9723420919/xpcom/base/nsVersionComparator.cpp#383-388
• Comparison relies on the parser at https://searchfox.org/mozilla-central/rev/9e2239eb75e16204a6186c26f80aaa9723420919/xpcom/base/nsVersionComparator.cpp#71,92-93,99,106-107,110,112
• The VersionPart struct uses an int32 for the numeric part of a version number: https://searchfox.org/mozilla-central/rev/9e2239eb75e16204a6186c26f80aaa9723420919/xpcom/base/nsVersionComparator.cpp#25
• The buildID exceeds the maximum supported value for in32_t:

   2147483647     2^31-1 (int32_t max value)
   20210922161155 buildID
  

• An out-of-range value is converted to 0 - https://searchfox.org/mozilla-central/rev/9e2239eb75e16204a6186c26f80aaa9723420919/xpcom/base/nsVersionComparator.cpp#43,55,60

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

S1: because this prevents Firefox 92.0.1 from picking up the updated langpacks.

Comment 4

[Eemeli Aro [:eemeli]]

Wouldn't the right fix here be for VersionComparator to stop trying parse the next VersionPart after the 9th digit? The algorithm is iterative, so the result of the overall comparison would stay the same.

Comment 5

[Rob Wu [:robwu]]

We're currently doing bug triage and may follow up with more details later.

One of the discussed options to fix this without binary changes to Firefox is to re-upload the langpack with a new version part at the end.

Here are examples of versions and comparisons (< means lower-than):

92.0buildid20210903235534   = current langpack version
92.0buildid20210922161155   = new version
92.0buildid0                = above versions are equivalent to this
<
92.0buildid0.1              = higher than 92.0buildid0
92.0buildid20210922161155.1 = same version as 92.0buildid0.1
<
92.0.1buildid0              = minor version bump has same effect

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Following some details from tracking the call from rr:

So, CompareVersions runs the while loop twice, the first time the parsed version ares:

(rr) p va
$33 = {numA = 92, strB = 0x0, strBlen = 0, numC = 0, extraD = 0x0}
(rr) p vb
$34 = {numA = 92, strB = 0x0, strBlen = 0, numC = 0, extraD = 0x0}

the second time it moves to compare the buildid part but it decides to only check the first 7 characters (which is buildid in both cases):

(rr) p va
$35 = {numA = 0, strB = 0x7fa066b4fb84 "buildid20210903235534", strBlen = 7, numC = 0, extraD = 0x0}
(rr) p vb
$36 = {numA = 0, strB = 0x7fa066b4f884 "buildid20210922161155", strBlen = 7, numC = 0, extraD = 0x0}

and then it results 0 as if the two versions are exactly the same.

Comment 8

[Mike Kaply [:mkaply]]

Snap is having the same problem even though it ships the new langpacks because of a version comparator problem here:

https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIInstall.jsm#3910

So the distro langpacks don't get installed.

Comment 9

[Rob Wu [:robwu]]

Luca and I just chatted about this. Summary:

1. The root cause of the issue here is the fact that the version part cannot be longer than 2^31-1 (int32_t), and that such invalid numbers are interpreted as 0 (zero).
2. While it is technically possible to bump this (e.g. by switching to int64_t), we are inclined to not fix this because there are other consumers of the logic, and a 32-bit integer is not an unreasonable limit. For example, the version comparator used by the AMO frontend (for Firefox versions, not add-on versions) relies on JS integers to compare versions, whose threshold is MAX_SAFE_INTEGER.
3. We should try to ensure that the services (AMO/Autograph?) does not sign add-ons with invalid versions, to avoid surprises like this bug.
4. This bug can be closed, or closed after updating documentation to emphasize the version limits.

---

My tentative recommendation for langpacks to resolve this issue is to split the version number in two parts that fit well within a 32-bit integer, like this:

92.0buildid20210922161155  <--BUGGY
92.0buildid20210922.161155 <-- NEW FORMAT, with dot after YYYYmmdd

---

Summarized from a thread in release coordination:

• The current approach to unblock this bug is to release a version that uses the full version number, i.e.

90.0buildid20210922161155   <-- BUGGY
90.0.1buildid20210922161155 <-- "FIXED"
90.0.1buildid0              <-- Equivalent to "FIXED", and newer than 90.0buildidxxxx.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is there a reason we aren't currently using the full version number for these?

Comment 11

[Andreas Wagner [:TheOne] [use NI]]

3. We should try to ensure that the services (AMO/Autograph?) does not sign add-ons with invalid versions, to avoid surprises like this bug.

See also https://github.com/mozilla/addons/issues/1264

Comment 12

[Ryan VanderMeulen [:RyanVM]]

We've uploaded new langpack addons which should resolve this issue. Users updating from 92.0 to 92.0.1 should automatically receive these updates and users currently in the broken state who were already updated to 92.0.1 should be able to manually check for addon updates to receive the fixed langpack.
https://support.mozilla.org/kb/how-update-add-ons

Comment 13

[Julien Cristau [:jcristau]]

Attachment: Bug 1732676 - include the full app version in langpack versions

Bug 1455337 started mangling the app version into both the langpack
version and strict_min_version. It turns out using the mangled version
as langpack version means firefox ignores legitimate updates.

So we keep the mangled major.0 as the "min version", but the
langpack's version itself should be bumped on every minor release so
firefox knows to update to it.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

We have enough ways of working around this issue (now that we're aware of it) that this fix can ride the trains whenever it's ready I think.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b19f06b6337a
include the full app version in langpack versions r=aki,robwu

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/b19f06b6337a

Comment 18

[Alex Cornestean]

We reproduced the issue on the latest Beta Dev Edition (94.0b9/20211021185848) with the old string formats, meaning that no updates were found when trying to update a language pack with the old string format to a newer version with the same string format.
Example: https://addons.allizom.org/en-US/firefox/addon/deutsch-de-language-pack-beta2/versions/

On the latest Nightly (95.0a1/20211026214417), we approached the issue in 2 ways:

1. Old string format update to new string format, with no updates found
   Example: https://addons.allizom.org/en-US/firefox/addon/deutsch-de-language-nightly/versions/

2. New string format to new string format, with updates being successful.
   Example: https://addons.allizom.org/en-US/firefox/addon/deutsch-de-language-pack1/versions/

The second result on Nightly confirms the fix.

Please let us know if we’ve correctly tested the fix or if the first result on Nightly is the expected one. Thank you!

Comment 19

[Alex Cornestean]

Hello,

As Beta 95 is now available I attempted language pack updating on this latest version. However, when checking for browser updates on Beta 94.0, none are available as of yet.

As such, I’ve downloaded Beta 95 (95.0b1/20211101163752) from the repository here https://archive.mozilla.org/pub/firefox/candidates/95.0b1-candidates/ and in conjunction with Beta 94.0 , proceeded with the following to validate the fix:

1. Created a profile on the old 94.0 Beta and installed an available 94 language pack (used DE lang pack – 94.0buildid20211028161635)
2. Installed the latest Beta 95.0b1 and opened the profile created at Step 1 (the lang pack was marked as incompatible with Firefox 95.0 in the Languages section of about:addons, which should be expected)
3. Checked for add-on updates on Beta 95 and an update was found. As a result the DE lang pack 94.0buildid20211028161635 was updated to 95.0buildid20211101.163752

This should confirm the fix. For further details, check the attached screenshots.

However, I’d also like to perform further tests when an official update from Beta 94.0 to Beta 95.0b1 is available before changing the status of the bug. I will perform the following:

• install old string format lang pack on Beta 94, update browser to Beta 95 and check that language pack updates are fetched and performed (from old string format to new string format)
• install older new string lang pack on Beta 95 and check language pack updates are fetched and performed (from new string format to new string format). This I presume will become available later in the week when another iteration of Beta 95 is released

Comment 20

[Alex Cornestean]

Attachment: Lang pack update from old string to new string.png

Comment 21

[Rob Wu [:robwu]]

This bug cannot easily be verified - it happens when a langpack is updated in a dotrelease, which doesn't really happen (except recently when this bug was reported). To confirm the bug, we need to verify that the generated langpack includes the minor version part, which is already verified through a unit test in the patch.