1508777 - Built-in LWT recommendations break LWT updates

Status: VERIFIED FIXED

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

Description

[Mark Striemer [:mstriemer]]

The LWT that are bundled and installable through Customize do not include an updateURL. It is assumed that each LWT will have an updateURL though, and the update code will throw an error preventing all LWT updates [1].

I haven't tested the upgrade to static themes but it looks like the upgrade should happen regardless. However since the LWT update was prevented, you will likely end up with a static theme and a now duplicate LWT.

We also need to figure out what to do to deal with the recommended LWTs. Including an updateURL may allow them to be upgraded normally, if there's a reasonable way to do so.

[1] https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/toolkit/mozapps/extensions/LightweightThemeManager.jsm#286

Comment 1

[Amy Tsay [:amyt]]

Is it an option to remove the Recommended section entirely as a fix for this bug? We have plans to add recommendations to about:addons, and between that and the "Get More Themes" button on the bottom of the menu, we can probably drop this section with minimal impact to discovery.

Comment 2

[Kris Maglione [:kmag]]

(In reply to Amy Tsay [:amyt] from comment #1)
> Is it an option to remove the Recommended section entirely as a fix for this
> bug?

We can remove the UI easily enough, but users who already have the themes installed still have no way to get upgrades when we remove support for LWTs.

Comment 3

[Amy Tsay [:amyt]]

For the bundled default themes, since a duplicate will be created, can we forcibly remove the LWT version once the update happens?

Is the question for Recommended themes whether we can retroactively include an update URL to the ones already installed? And then remove the section altogether so people can't install those anymore?

Comment 4

[Kris Maglione [:kmag]]

(In reply to Amy Tsay [:amyt] from comment #3)
> For the bundled default themes, since a duplicate will be created, can we
> forcibly remove the LWT version once the update happens?

I'm not sure what you mean by "duplicates will be created". Those themes don't have any updateURLs.

Comment 5

[Amy Tsay [:amyt]]

I was referring to what Mark said, "since the LWT update was prevented, you will likely end up with a static theme and a now duplicate LWT".

Comment 6

[Kris Maglione [:kmag]]

(In reply to Amy Tsay [:amyt] from comment #5)
> I was referring to what Mark said, "since the LWT update was prevented, you
> will likely end up with a static theme and a now duplicate LWT".

Don't think so. We're completely relying on the AMO update mechanism to replace LWTs with static themes. These themes don't have update URLs at all, so they'll just die.

Comment 7

[Amy Tsay [:amyt]]

Ok, does this mean that for anyone who has installed a default or recommended theme from this menu, we either have to figure out how to give them an update URL, or they'll just revert back to the default theme when we update to static themes?

Comment 8

[Kris Maglione [:kmag]]

(In reply to Amy Tsay [:amyt] from comment #7)
> Ok, does this mean that for anyone who has installed a default or
> recommended theme from this menu, we either have to figure out how to give
> them an update URL, or they'll just revert back to the default theme when we
> update to static themes?

Yes.

Comment 9

[Amy Tsay [:amyt]]

And you're the one to figure this out, right? :-D
Do you know how many people are in this boat?

Comment 10

[Kris Maglione [:kmag]]

I don't like boats.

Comment 11

[Andrew Swan [:aswan]]

Putting this on relman's radar.  This is going to affect users who have installed one of the recommended themes from the customize menu at the point when AMO starts converting lightweight themes.  Specifically, those users will end up with two copies of every lightweight theme (they'll get the converted version, but the old version won't be cleaned up due to this bug).

The AMO theme conversion is currently scheduled for the week of January 7, so if there's a dot release before then, we'd like to consider this for a ride-along.

Comment 12

[Andrew Swan [:aswan]]

In addition to fixing the update bug here, we would also like to provide update urls for the recommended themes.

Two of them are:

A Web Browser Renaissance
updateURL: https://versioncheck.addons.mozilla.org/en-US/themes/update-check/31658

Pastel Gradient:
updateURL: https://versioncheck.addons.mozilla.org/en-US/themes/update-check/520346

Unfortunately, the third theme, "Space Fantasy" lists this page which is 404 as its homepage:
https://addons.mozilla.org/en-US/firefox/addon/space-fantasy/

The page https://addons.mozilla.org/en-US/firefox/addon/space_fantasy/ does exist, but that appears to be a different theme.

Comment 13

[Andrew Swan [:aswan]]

Upon further consideration, we'll defer the work of fixing any users of the recommended themes (ie the stuff from comment 12) to a separate bug, this one is now quite simple, patch coming.

Comment 14

[Andrew Swan [:aswan]]

Attachment: Bug 1508777 Skip updates for LWTs with no updateURL r=kmag

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f69fb4fba54b
Skip updates for LWTs with no updateURL r=kmag

Comment 16

[Julien Cristau [:jcristau]]

So far we don't have a dot release planned before January 7, though we may have one that week.

Comment 17

[Eliza Balazs [:ebalazs_]]

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

Comment 18

[Andrew Swan [:aswan]]

I did the manual verification here myself but installing an LWT from the AMO dev environment then converting it to a static theme.  Without this patch, the user is left with two copies of the converted theme (the converted theme is installed but the old LWT is not removed).  With this patch, the user is left with one copy as expected.

Comment 19

[Andrew Swan [:aswan]]

Comment on attachment 9032320 [details]
Bug 1508777 Skip updates for LWTs with no updateURL r=kmag

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1430276

User impact if declined: This bug only affects users who have installed a theme from the "Recommended" list in the Themes section of the Customize menu.  When AMO begins converting lightweight themes to packaged themes, users with recommended themes will end up with two copies of every converted lightweight theme.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The change error is small, it consists entirely of improved error handling, and it is limited to the code that handles theme updates.

String changes made/needed: none

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9032320 [details]
Bug 1508777 Skip updates for LWTs with no updateURL r=kmag

Let's uplift for beta 6 since this is verified in nightly. 
Though, normally it's best to have someone who didn't write the patch do the verifying.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Actually, this looks like a nightmare for QA to verify given comment 18. Andrew, can you please also test 65.0b6  once it's released on Friday?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fb3197004db5

Comment 23

[Andrew Swan [:aswan]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)
> Actually, this looks like a nightmare for QA to verify given comment 18.
> Andrew, can you please also test 65.0b6  once it's released on Friday?

I did it today in the interest of getting it done quickly.  I'm happy to do it again for Beta, but I'm going to be traveling on Friday so I won't be able to do it right away.  Krupa, could somebody from QA who did the original migration testing help with this?  The steps are basically what's described in comment 18 with the prerequisite of having one of the recommended themes installed from the customize page.

Comment 24

[Bogdan Maris, Desktop QA]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)
> Actually, this looks like a nightmare for QA to verify given comment 18.
> Andrew, can you please also test 65.0b6  once it's released on Friday?

Will drop qe-verify+ based on this.

Comment 25

[Andrew Swan [:aswan]]

My understanding is the task of verifying this in beta went into the QA queue during the holiday break, and should hopefully happen in the next few days.

Comment 26

[Vlad Jiman]

Andrew, we've started validating this but we have a few questions: we've noticed that the recommended themes that appear on the customize page are hardcoded and do not have an AMO id, I know that in order for the migration to be performed AMO devs need a valid theme id. I'd like to know the steps you followed in order to test this fix in Nightly, to make sure that we are on the same page. Please note that we are testing this on AMO stage. 
Also for the expected results, is the duplicate LWT not supposed to show in about:addons - themes tab?

Comment 27

[Andrew Swan [:aswan]]

This bug is about the updates to *other* LWTs being broken if one of the recommended themes is installed.  So the steps are:

1. Install an LWT from AMO (not a recommended theme obviously)
2. Install one of the recommended themes
3. Convert the LWT from step 1 into a static theme (and be sure theme migration is enabled on the AMO side)
4. Trigger a browser update check (simplest way to do this is to open the browser console and paste in this line:
ChromeUtils.import("resource://gre/modules/AddonManager.jsm", {}).AddonManagerPrivate.backgroundUpdateCheck();

To rule out other about:addons front-end glitches, close and re-open about:addons if you had it open during the above.  The recommended theme will not be converted of course, but the LWT from step 1 should only appear once in the list of themes in about:addons.

Comment 28

[Vlad Jiman]

Thanks for the clarification, we managed to validate this successfully using Fx65 and Nightly, however we needed to trigger the browser update check as described at comment #27 step 4. Also since we were on the about:addons page while the update check was performed, we had to do a refresh to get rid of the duplicate themes.

Comment 29

[Julien Cristau [:jcristau]]

Comment on attachment 9032320 [details]
Bug 1508777 Skip updates for LWTs with no updateURL r=kmag

fix an issue with LWT updates, approved for 64.0.2

Comment 30

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/e4c03449c5ec

Comment 31

[Cornel Ionce [:noni] [Hubs QA]]

Setting the qe-verify+ flag for the verification on 64.0.2.

Comment 32

[Vlad Jiman]

Verified using the steps from comment #27 on 64.0.2, everything works as expected and no other issues were encountered.