Closed Bug 1646016 Opened 4 years ago Closed 4 years ago

Version compatibility is not checked when installing language packs.

Categories

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

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
82 Branch
Tracking Flags:
Tracking Status
firefox77 --- wontfix
firefox78 --- wontfix
firefox79 --- wontfix
firefox80 --- wontfix
firefox81 --- fixed
firefox82 --- fixed

People

(Reporter: flod, Assigned: mixedpuppy)

References

Details

Attachments

(2 files)

extensions.json
90.63 KB, application/json
Details
Bug 1646016 fix app compat data for locales if they are not version specific
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
Details | Review

Not sure if this is the right component, in case feel free to move.

With Nightly (79) or Beta (78), try installing the language pack from AMO
https://addons.mozilla.org/firefox/addon/fran%C3%A7ais-language-pack/

It will install without issues, even if the manifest explicitly has

  "applications": {
    "gecko": {
      "id": "langpack-fr@firefox.mozilla.org",
      "strict_max_version": "77.*",
      "strict_min_version": "77.0"
    }
  },

[Tracking Requested - why for this release]: I believe both bug 1643067 and bug 1645470 were caused by this.

I still need to check if 77 is affected. If it is, on the next update an existing language pack will be considered compatible, and things will break very badly.

status-firefox78: --- → affected
status-firefox79: --- → affected
tracking-firefox78: --- → ?
Component: Extension Compatibility → Add-ons Manager
Product: Firefox → Toolkit

Tried installing a language pack on release (77.0.1) from FTP, and the version check is correctly enforced
http://archive.mozilla.org/pub/firefox/releases/76.0/linux-x86_64/xpi/

status-firefox77: --- → unaffected
Keywords: regressionwindow-wanted

I have no clue how much we rely on AMO for the compatibility checks.

A side effect of this, is that folks can flip these preferences, and install an invalid language pack directly from preferences (downloaded from AMO)

intl.multilingual.downloadEnabled
intl.multilingual.enabled

The API returns language packs, when it shouldn't (asking for appversion 90)
https://addons.mozilla.org/api/v4/addons/language-tools/?app=firefox&type=language&appversion=90.0&author=mozilla

Keywords: regression

I'm starting to be really confused. Noticed while trying to figure out a regression window with mozregression.

Using Nightly (79):

So, maybe 77 is affected, but I can't really tell. For sure, I can't install language packs compatible with 76 or 78 on the current release.

Rob, any idea?

tracking-firefox78: ? → +
Flags: needinfo?(rob)

This is a bug in AMO, Mat will follow up with a comment.

From Firefox's perspective, this is working as intended:

Note: When I visit the langpack from the bug report on Beta, I am served 78.0buildid20200612174529 with range 78.0 - 78.*. This is the correct version and will result in the expected langpack installation.

Flags: needinfo?(rob)

On the AMO side, the problem is that we silently "rewrite" (not actually rewrite, but just store in our database) invalid/unknown strict_max_version in the manifest as *, and that sometimes, langpacks are submitted on AMO before the corresponding Firefox version object is created in AMO database. So, some langpacks ended up with * as the max version in our database, and that's why the API is returning them.

Our plan to fix this on AMO is:

  1. Add API to create new AppVersion instances https://github.com/mozilla/addons-server/issues/14649
    • Once the API exists, have Releng call that API in their release scripts, before submitting langpacks
  2. Raise an error when an invalid strict_max_version is used https://github.com/mozilla/addons-server/issues/14654
  3. Fix existing language packs incorrectly pointing to * as their max app version https://github.com/mozilla/addons-server/issues/14653

(In reply to Francesco Lodolo [:flod] from comment #1)

I still need to check if 77 is affected. If it is, on the next update an existing language pack will be considered compatible, and things will break very badly.

This bug is not tied to a Firefox version, but to whether AMO has a known match for the requested version.

I just tried to simulate an update for updating from 77 to 78, by replacing "79.0a1" with "78.0" (and "78.0.112") in the URL from comment 10, and am served a langpack for the correct target (78.0).

https://versioncheck.addons.mozilla.org/update/VersionCheck.php?reqVersion=2&id=langpack-fr@firefox.mozilla.org&version=77.0buildid20200504222419&maxAppVersion=77.*&status=userEnabled,incompatible&appID=%7Bec8030f7-c20a-464f-9b0e-13a3a9e97384%7D&appVersion=78.0.112&appOS=Linux&appABI=x86_64-gcc3&locale=en-US&currentAppVersion=78.0.112&updateType=49&compatMode=normal

PS. Update check is triggered here and the min/max versions are overwritten during the update check.

QA Whiteboard: [qa-regression-triage]

Hello,

Based on Comment 8 and Comment 10, since it’s an AMO issue, can the regressionwindow-wanted flag be removed? Thank you!

Flags: needinfo?(rob)
Flags: needinfo?(rob)

Removing tracking since this isn't tied to a change on the client side.

tracking-firefox78: + → ---

Looks like the immediate issue was fixed. Should this bug remains open to track the work that needs to avoid it happening again?

The severity field is not set for this bug.
:mixedpuppy, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(mixedpuppy)
Assignee: nobody → mixedpuppy
Severity: -- → S3
Flags: needinfo?(mixedpuppy)
Priority: -- → P2

(In reply to Francesco Lodolo [:flod] from comment #13)
Looks like the immediate issue was fixed. Should this bug remains open to track the work that needs to avoid it happening again?

Yes, let's keep it open for that purpose. For robustness we should not blindly trust the compatibility information from the server.
In the last line of comment 10 I pointed to the code that does overwrite the compat info. That overwrite should not be respected for langpacks; for langpacks we should verify the compatibility when the xpi is read/installed.

See Also: → 1648214

bug 1655040 just got filed - did this problem recur? The API calls appear to be returning the right info. Can we uninstall/disable these langpacks if they are not compatible anymore? It's causing yellow-screen-of-death issues.

Flags: needinfo?(rob)
Flags: needinfo?(mixedpuppy)
See Also: → 1655040

I think that the issues are unrelated. This bug is about refusing to install incompatible langpacks.

The other bug could happen when a profile contains an old langpack, and the application updates.
Fixing this bug would not fix the other bug, but fixing the other bug can fix this bug.

Shane is looking into multiple langpack-related issues, I would wait until he returns.

Flags: needinfo?(rob)

Here's what I don't understand:

  • It was possible to install that language pack because the compatibility was incorrectly set to "strict_max_version": "*" on AMO for 77 language packs.
  • Why was the language pack still active? The local files has 77.*, compatibility on AMO is supposedly fixed, and the fix should have happened before 80 started (so, there was an update from 79 to 80).

When are compatibility checks performed on Nightly?

(In reply to Francesco Lodolo [:flod] from comment #18)

When are compatibility checks performed on Nightly?

They are checked during install but I believe they are also supposed to be checked periodically as part of regular update checks.
Does the affected user have background addon updates disabled? Can we get a copy of extensions.json from their profile?

(In reply to Andrew Swan [:aswan] from comment #19)

(In reply to Francesco Lodolo [:flod] from comment #18)

When are compatibility checks performed on Nightly?

They are checked during install but I believe they are also supposed to be checked periodically as part of regular update checks.
Does the affected user have background addon updates disabled? Can we get a copy of extensions.json from their profile?

--> Dexter.

Flags: needinfo?(alessio.placitelli)
Attached file extensions.json —

(In reply to :Gijs (he/him) from comment #20)

(In reply to Andrew Swan [:aswan] from comment #19)

(In reply to Francesco Lodolo [:flod] from comment #18)

When are compatibility checks performed on Nightly?

They are checked during install but I believe they are also supposed to be checked periodically as part of regular update checks.
Does the affected user have background addon updates disabled? Can we get a copy of extensions.json from their profile?

--> Dexter.

Nope, I have background addon updates enabled :) I attached my extensions file.

Flags: needinfo?(alessio.placitelli)

(In reply to Andrew Swan [:aswan] from comment #19)

(In reply to Francesco Lodolo [:flod] from comment #18)

When are compatibility checks performed on Nightly?

They are checked during install but I believe they are also supposed to be checked periodically as part of regular update checks.
Does the affected user have background addon updates disabled? Can we get a copy of extensions.json from their profile?

I've also asked him to do a manual search for updated in about:addons before removing the add-on, and it didn't do anything.

An update is not available, since we only have language packs compatible with Beta and Release on AMO, but I was expecting it to disable the old add-on.

Pushed by scaraveo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/77ff17c3906c fix app compat data for locales if they are not version specific r=robwu

https://hg.mozilla.org/mozilla-central/rev/77ff17c3906c

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox82: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

Comment on attachment 9170821 [details]
Bug 1646016 fix app compat data for locales if they are not version specific

Beta/Release Uplift Approval Request

  • User impact if declined: Most probable cause of YSOD happening on beta 81
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Unsure of manual steps to produce this, as it was caused initially by addon compat override data from addons.mozilla.org. It is a complicated chain to reproduce. While the tests should cover this, the severity warrants the extra effort to verify that the YSOD is dealt with.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The patch is pretty simple and has been on nightly for a few days now, and is covered by automated tests.
  • String changes made/needed: none
Flags: needinfo?(mixedpuppy)
Attachment #9170821 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9170821 [details]
Bug 1646016 fix app compat data for locales if they are not version specific

Approved for 81.0b5.

Attachment #9170821 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-regression-triage] → [qa-regression-triage][qa-triaged]

Hello, Here are my testing results:

  • I went to the list of lang packs listed https://addons.mozilla.org/en-US/firefox/language-tools/, then to the detail page of a lang pack and installed FF77, then FF76. With both versions of FF I was able to install the lang packs but if I checked on the all version page https://addons.mozilla.org/en-US/firefox/addon/fran%C3%A7ais-language-pack/versions/, the compatible versions for FF76 or FF77 are no longer listed. I should have a warning on the detail page and the install button to be disabled. Strange fact, I was not able to reproduce this scenario on AMO dev.
  • with FF82.0a1 the lang packs cannot be installed. A compatible version has not been uploaded yet. I have the warning message displayed on the detail page and the button is inactive.
  • with FF81.0b5 I could install the lang packs without problems because versions with min:81.0 and max:81.* have been uploaded.

So maybe the fact that I could not install lang packs with FF82 (nightly) is proving that the issue has been fixed.

Please let me know if I can mark the issue verified.

Thank you!

Shane, is comment 31 expected?

Flags: needinfo?(mixedpuppy)

That all looks like reasonable behavior.

The primary issue is to verify that a language pack installed into any given version of firefox does not remain enabled when upgrading to a newer version of firefox, or that with fx81 upgrading to fx82, that the language pack was updated (though that is tested in a separate bug, so don't spend extra time verifying that).

So the test should probably look like:

  • install fx76
  • install a langpack
  • wait*
  • install/upgrade to fx77
  • verify langpack is now disabled
  • the issue here is that at some point, some api calls are made to AMO to update compatibility data. This is where we received incorrect data before, and it is that incorrect data that is necessary to replicate the problem. This is where I don't see a clean way to verify this fix, we should probably rely on the automated test in the patch.
Flags: needinfo?(mixedpuppy)

(In reply to Shane Caraveo (:mixedpuppy) (on PTO till 9/14) from comment #33)

That all looks like reasonable behavior.

The primary issue is to verify that a language pack installed into any given version of firefox does not remain enabled when upgrading to a newer version of firefox, or that with fx81 upgrading to fx82, that the language pack was updated (though that is tested in a separate bug, so don't spend extra time verifying that).

So the test should probably look like:

  • install fx76
  • install a langpack
  • wait*
  • install/upgrade to fx77
  • verify langpack is now disabled
  • the issue here is that at some point, some api calls are made to AMO to update compatibility data. This is where we received incorrect data before, and it is that incorrect data that is necessary to replicate the problem. This is where I don't see a clean way to verify this fix, we should probably rely on the automated test in the patch.

I verified this again with different FF versions, FF76 -> FF82.
This time after installing lang packs, I upgraded/opened the profile with a newer FF version. The about:addons showed the lang packs as being disabled. Meanwhile the +Add to Firefox button was active on AMO prod (as expected).
An auto-update runs and will update them to be compatible with the new FF (in this phase AMO displays the "Remove" state on the add-on detail page), except for FF82 that keeps them disabled as the last uploaded version is now FF81 ( the manifest shows: "strict_min_version": "81.0", "strict_max_version": "81.*").

I think this is the expected behavior, so I'm going to mark it verified by qa.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: