563256 - Re-enabling the active theme doesn't clear the pending switch preference

Status: VERIFIED FIXED

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

Description

[[e-mail]]

Note: extensions.dss.enabled is false for the purposes of this bug.

I only have two themes to test this with, so I am not sure how this might generalise to multiple themes.

After enabling an alternate theme, if the active (as defined by isActive is true) theme is re-enabled before restarting then the Classic theme is displayed.

Additionally, the next theme switch causes the Addon Manager to believe that the new theme is active, so that you would then need to switch the theme twice more before restarting to duplicate the bug.

Initial conditions:
Classic theme: isActive is true, userDisabled is false
Modern theme: isActive is false, userDisabled is true

Steps to reproduce problem:
1. Set Modern theme userDisabled to false.
This sets the Classic theme userDisabled to true.
2. Set Classic theme userDisabled to false.
This sets the Modern theme userDisabled to true.
3. Restart. (This doesn't change the properties.)
4. Set Modern theme userDisabled to false.
This sets the Classic theme userDisabled to true, but it strangely also sets the Classic theme isActive to false and the Modern theme isActive to true!
5. Set Classic theme userDisabled to false.
This sets the Modern theme userDisabled to true.
6. Set Modern theme userDisabled to false.
This sets the Classic theme userDisabled to true.
7. Restart.
At this point, the Classic theme is displayed!
8. Set Classic theme userDisabled to false.
This sets the Modern theme userDisabled to true, but the Classic theme isActive changes to true and the Modern theme isActive changes to false...

The fix for this would seem to be to unconditionally set the PREF_DSS_SWITCHPENDING preference to aPendingRestart.

Comment 1

[Dave Townsend [:mossop]]

Thanks for the excellent STR, I converted them directly to an automated test.

Fixed in http://hg.mozilla.org/projects/addonsmgr/rev/6d179ca894db

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This keeps track of the currently displaying skin and if the switch is back to that then it clears the switch prefs.

Comment 3

[[e-mail]]

Comment on attachment 443375 [details] [diff] [review]
patch rev 1

Just a general thought, it occurred to me that you only track the value of the current/selected skin preference that you think you set it to; this might not match the value in about:config as you don't appear to have a listener.

>+      Services.prefs.clearUserPref(PREF_DSS_SWITCHPENDING);
>+      Services.prefs.clearUserPref(PREF_DSS_SKIN_TO_SELECT);
Is this safe or could it throw, or does it not matter if it throws?

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 443375 [details] [diff] [review]
patch rev 1

Yeah I think you're right and it can throw, good catch, will update.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, we catch everything with the automated test or do we also need a manual test?

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This handles any problems clearing the prefs. For watching the prefs for external changes I'd like to take care of that separately, it is a pre-existing problem that needs some thought and I don't want to block this fix on it.

Comment 7

[Dave Townsend [:mossop]]

Filed bug 563953 for that.

Comment 8

[Dave Townsend [:mossop]]

I think the automated test covers this one.

Comment 9

[Dave Townsend [:mossop]]

Follow-up was landed at http://hg.mozilla.org/projects/addonsmgr/rev/e402f4069631

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 443636 [details] [diff] [review]
patch rev 2

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -779,16 +779,18 @@ var XPIProvider = {
>   // An array of known install locations
>   installLocations: null,
>   // A dictionary of known install locations by name
>   installLocationsByName: null,
>   // An array of currently active AddonInstalls
>   installs: null,
>   // The default skin for the application
>   defaultSkin: "classic/1.0",
>+  // The currently selected skin for the application
The current skin used by the application

>+  currentSkin: null,
>   // The currently selected skin or the skin that will be switched to after a
>   // restart
The selected skin to be used by the application when it is restarted. This
will be the same as currentSkin when it is the skin to be used when the
application is restarted.

Though I think this could be refactored so selectedSkin is null except when there is a change I don't think doing so would be of much value

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 443636 [details] [diff] [review]
patch rev 2

r=me with the comment fixes

Comment 12

[Dave Townsend [:mossop]]

http://hg.mozilla.org/mozilla-central/rev/7e481c7f64c6

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marking as verified fixed based on check-in and passing automated tests.