Closed Bug 1732676 Opened 2 months ago Closed 2 months ago

Firefox doesn't fetch updated language pack

Categories

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

defect

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox-esr91 --- wontfix
firefox92 + wontfix
firefox93 - wontfix
firefox94 - wontfix
firefox95 --- fixed

People

(Reporter: flod, Assigned: jcristau)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

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

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.

Flags: needinfo?(lgreco)

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).

See Also: → 1732382

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

Severity: -- → S1
Flags: needinfo?(lgreco)
Priority: -- → P1

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.

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

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.

Duplicate of this bug: 1732382

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.

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.

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

  1. 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

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

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.

Assignee: nobody → jcristau
Status: NEW → ASSIGNED
Regressed by: 1455337

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.

See Also: → 1733396
Pushed by jcristau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b19f06b6337a
include the full app version in langpack versions r=aki,robwu
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch
Duplicate of this bug: 1733664

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!

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

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.

You need to log in before you can comment on or make changes to this bug.