Built-in LWT recommendations break LWT updates

VERIFIED FIXED in Firefox 64

Status

()

defect
P1
normal
VERIFIED FIXED
5 months ago
2 months ago

People

(Reporter: mstriemer, Assigned: aswan)

Tracking

(Blocks 2 bugs)

Trunk
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64+ verified, firefox65 verified, firefox66 verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 months ago
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
(Reporter)

Updated

5 months ago
Priority: -- → P1
Depends on: 1513316
Blocks: 1513316
No longer depends on: 1513316

Comment 1

4 months ago
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.
(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

4 months ago
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?
(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

4 months ago
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".
(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

4 months ago
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?
(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

4 months ago
And you're the one to figure this out, right? :-D
Do you know how many people are in this boat?
I don't like boats.
Assignee: nobody → aswan
(Assignee)

Comment 11

4 months ago
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.
(Assignee)

Comment 12

4 months ago
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.
(Assignee)

Comment 13

4 months ago
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 15

4 months ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f69fb4fba54b
Skip updates for LWTs with no updateURL r=kmag
So far we don't have a dot release planned before January 7, though we may have one that week.

Comment 17

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f69fb4fba54b
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
(Assignee)

Comment 18

4 months ago
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.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 19

4 months ago
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
Attachment #9032320 - Flags: approval-mozilla-release?
Attachment #9032320 - Flags: approval-mozilla-beta?
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.
Attachment #9032320 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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?
Flags: needinfo?(aswan)
(Assignee)

Comment 23

4 months ago
(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.
Flags: needinfo?(kraj)
(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.
(Assignee)

Comment 25

4 months ago
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.
Flags: needinfo?(aswan)

Comment 26

4 months ago
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?
Flags: needinfo?(aswan)
(Assignee)

Comment 27

4 months ago
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.
Flags: needinfo?(aswan)

Comment 28

4 months ago
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.
Flags: needinfo?(kraj)

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

Attachment #9032320 - Flags: approval-mozilla-release? → approval-mozilla-release+

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

Flags: qe-verify+

Comment 32

3 months ago

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

Flags: qe-verify+

Updated

2 months ago
Blocks: rm-lwthemes

Updated

2 months ago
No longer blocks: 1513316
You need to log in before you can comment on or make changes to this bug.