1810231 - Fix expired retained colorway theme still uninstalled by BuiltInThemes.ensureBuiltInThemes as expired after being migrated from built-in to AMO installed

Status: VERIFIED FIXED

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

Description

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

This bugzilla issue is tracking a followup to Bug 1806701 to fix the scenario where an expired retained colorway theme is updated while being not set as the active theme, in that case the migrated theme is getting uninstalled when BuiltInThemes.ensureBuiltInThemes will be called again after the migration has been completed (which can either happen when about:addons themes list view is opened for the first time or refreshed, e.g. as by switching from the detail view to the list view as described in the STR right below) because the retained theme is being removed from the retainedExpiredTheme pref and BuiltInThemes._uninstallExpiredThemes (called internally by BuiltInThemes.ensureBuiltInThemes) does only check the id and not if they are installed in a builtin XPIProvider location or not when considering a colorway theme expired and to be removed.

This issue has been caught as part of the ongoing QA verification related to the colorway migration logic introduced by Bug 1806701, and reported to me promptly by Alex Cornestean:

• setup nightly profile for pointing to the AMO dev instance for the addon updates checks
• added 2 themes via browser.theme.retainedExpiredThemes (the 2022-red-colorway and 2022-orange-colorway).
• after a browser restart both the themes are listed in about:addons as expected, and the red one is set as active
• finally performed the manual update and the following happened:

Actual Behavior:

• the red theme, the active one, got properly updated to 2.0 and stays installed and set as the active theme
• the inactive one (the orange theme) disappeared from about:addons, after being updated to 2.0, when the about:addons tab is switched to the red theme details view and then back to the themes list view
• the retained theme was also removed from the retainedExpiredThemes pref and the related xpi file is not listed anymore in the profile extensions subdirectory

Expected Behavior:

• the red theme, the active one, got properly updated to 2.0 and stays installed and set as the active theme (same as actual behavior)
• the inactive one (the orange theme) should have been removed from the retainedExpiredThemes pref (same as actual behavior
• the inactive one (the orange theme) should stay listed in about:addons, its related xpi file still listed in the profile extensions subdirectory

Comment 1

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

I looked into this issue today, there were two underlying issues:

• the first issue is described in the preamble of comment 1 (related to BuiltInThemes._uninstallExpiredThemes only checking the theme id and not if they are still installed in a builtin XPIProvider location or not when considering a colorway theme expired and to be removed)
• the second issue was instead just a rendering issue: the themes list sections are still filtering out the migrated colorway theme as if it is expired (also in this case because that code is not yet checking if the theme is also installed as builtin before considering it as expired)

The xpcshell test included in Bug 1806701 was not catching this issue and so this bug should also take care of making sure we are covering this scenario properly and a test would be failing if these two issues are still hit.

Comment 2

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

Attachment: Bug 1810231 - Prevent BuiltInThemes._uninstallExpiredThemes to uninstall non-builtin colorways themes as expired and not retained. r?dao!

Comment 3

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

Attachment: Bug 1810231 - Ensure that non-builtin colorways themes are not hidden in about:addons as if they are expired builtin themes. r?dao!,willdurand

Depends on D166834

Comment 4

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

Attachment: Bug 1810231 - Do not add back into retained expired theme pref the active theme if not a colorway builtin. r?dao!

Depends on D166835

Comment 5

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

Attachment: Bug 1810231 - Add an additional mochitest-browser test to cover the colorway builtin themes migration. r?willdurand!,dao

Depends on D166917

Comment 6

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

Push to try: https://treeherder.mozilla.org/jobs?repo=try&revision=122e21e96c1e4a3d986e0932adac114036ba306a

Comment 7

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

Attachment: Bug 1810231 - Remove builtin colorway theme from builtin location when the migrated colorway theme is uninstalled. r?willdurand!

This patch also fixes the TV job failure caught by running the new mochitest-browser test case from D166918,
where the test was failing on the second run because installing the colorway theme builtin in the same
profile where is was previously installed by the same test and then left into the builtin location as
disabled and not visible is basically a no-op.

By removing the instance installed in the builtin location we are still preventing the colorway builtin
to be made enabled again when the migrated theme is installed, but the TV job will be able to pass as
expected.

Depends on D166917

Comment 8

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

Last push to try confirmed that with the addition of the patch attached in comment 7 the TV job is now passing as expected:

• https://treeherder.mozilla.org/jobs?repo=try&revision=3174986e5b123a2b046febeffcd028c70b96f02f

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bef2fcf4c3a2
Prevent BuiltInThemes._uninstallExpiredThemes to uninstall non-builtin colorways themes as expired and not retained. r=dao
https://hg.mozilla.org/integration/autoland/rev/b92cb2804063
Ensure that non-builtin colorways themes are not hidden in about:addons as if they are expired builtin themes. r=willdurand,dao
https://hg.mozilla.org/integration/autoland/rev/5fd77b37a5d0
Do not add back into retained expired theme pref the active theme if not a colorway builtin. r=dao
https://hg.mozilla.org/integration/autoland/rev/e973db0b1eff
Remove builtin colorway theme from builtin location when the migrated colorway theme is uninstalled. r=willdurand
https://hg.mozilla.org/integration/autoland/rev/834df4e872e2
Add an additional mochitest-browser test to cover the colorway builtin themes migration. r=dao,willdurand

Comment 10

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/bef2fcf4c3a2
https://hg.mozilla.org/mozilla-central/rev/b92cb2804063
https://hg.mozilla.org/mozilla-central/rev/5fd77b37a5d0
https://hg.mozilla.org/mozilla-central/rev/e973db0b1eff
https://hg.mozilla.org/mozilla-central/rev/834df4e872e2

Comment 11

[Alex Cornestean]

Verified as Fixed on the latest Nightly (111.0a1/20230119093127). Tested on Windows 10 x64 and Ubuntu 16.04 LTS.

Checked the fix as per the below STR:

1. Made the necessary pref changes to point the browser updates to -Stage and made 3 expired themes visible again: [“[e-mail]”, “[e-mail]”, ”[e-mail]”]. The generated themes have been uploaded to AMO -Stage beforehand.
2. Set “activist-balanced” as active while the other 2 are left as is, disabled.
3. Performed a manual update and all 3 themes were updated properly as per the Network tab in the browser toolbox showing the requests. Checking the theme versions in about:addons confirms that they were updated to 2.0. The .XPIs also showed up in the extensions folder from the profile, as expected.
4. Refreshed about:addons a few times and none of the themes disappeared.
5. Set “activist-bold” as active and refreshed about:addons a few times. The theme was set as active without issues and none of them disappeared as a result of refreshing.
6. Set “activist-balanced” as active again and then removed it. The theme was removed and it did not come back after a page refresh or browser restart. In the extensions folder from the profile folder, the removed theme .xpi was gone, while the other 2 .xpis were still there as expected.
7. Removed an inactive theme (“activist-soft”). The theme did not come back after page refresh or browser restart. The .xpi was gone from the extensions folder in the profile. Only “activist-bold” is left as expected both in about:addons and the extensions folder from the profile.
8. Checked "browser.theme.retainedExpiredThemes" pref which is now empty, as expected.

As per the above, the fix is confirmed.