1339754 - Compact themes are not applied after restart if you try to enable them with a complete theme installed

Status: RESOLVED WONTFIX

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

Description

[Ciprian Georgiu, Desktop QA]

[Affected versions]:
- latest Nightly 54.0a1 (2017-02-14)
- latest Aurora 53.0a2 (2017-02-14)

[Affected platforms]:
- Windows 10 x64
- macOS 10.12 (Sierra)
- Ubuntu 16.04 x64 LTS

[Steps to reproduce]:
1. Launch Firefox
2. Go to: https://addons.mozilla.org/En-us/firefox/complete-themes/ and install a complete theme, e.g "LavaFox V2"
3. Restart the browser in order to install the complete theme
4. Go to about:addons and select "Appearance" (if it's not already in focus)
5. Enable Compact Dark theme and click "Restart now" to apply the selected theme 

[Expected result]:
- Compact Dark theme is enabled after restart and the other themes are displayed as Disabled  

[Actual result]:
- Compact Dark theme is not enabled and all themes are displayed as Disabled 

[Regression range]:
- This is not a regression as it is reproducible on Nightly 53.0a1 (20170115030210) where this feature first landed

[Additional notes]:
- both Compact Dark and Compact Light are affected 
- see screenshot with about:addons after browser restart: http://imgur.com/a/MNl2N

Comment 1

[:Gijs (he/him)]

Does this affect other lightweight themes besides the compact ones we're now shipping?

Andrew, do you have time to look at this?

Comment 2

[Ciprian Georgiu, Desktop QA]

(In reply to :Gijs from comment #1)
> Does this affect other lightweight themes besides the compact ones we're now
> shipping?

No, it only affects compact themes.

Comment 4

[Vasilica Tamas, QA [:vtamas]]

This seems to be an old issue which reproduces constantly on Nightly and Dev Edition branches. See Bug 1194638.

Comment 5

[Andrew Swan [:aswan]]

(In reply to :Gijs from comment #1)
> Andrew, do you have time to look at this?

I'll put it in my queue, hopefully can get to it this week...

Comment 6

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Andrew Swan [:aswan] from comment #5)
> (In reply to :Gijs from comment #1)
> > Andrew, do you have time to look at this?
> 
> I'll put it in my queue, hopefully can get to it this week...

That would be great. While I do not think this should block compact themes ( not many people use complete themes ) I would support this fix being uplifted to 53. Please needinfo me if you need help with that.

Comment 7

[Andrew Swan [:aswan]]

I think the basic problem here is that when we switch from a complete them to a lightweight theme, the code that applies the change after a restart is here:
http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/toolkit/mozapps/extensions/LightweightThemeManager.jsm#328

Now, the add-ons manager itself is started indirectly from, of all things, nsXREDirProvider::DoStartup, which gets called from here:
http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/toolkit/xre/nsAppRunner.cpp#4307

But built-in themes don't get added to the LWT manager until the final-ui-startup event which happens a bit later:
http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/toolkit/xre/nsAppRunner.cpp#4307

The simplest fix would be to somehow move the addition of built-in lightweight themes earlier but I'm in over my head on the whole startup sequence.  We could also add code to the LWT manager to handle this sequence (ie, if getUsedTheme() returns null, remember the theme we want to apply and then apply it in addBuiltinTheme() ) but that sounds gross to me.

I could use some guidance here...

Comment 8

[Andrew Swan [:aswan]]

Whoops, copy/paste error, the final-ui-startup event is dispatched here:
http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/toolkit/xre/nsAppRunner.cpp#4401

which eventually leads to the builtin themes getting installed here:
http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/browser/components/nsBrowserGlue.js#644-663

Comment 9

[Dave Townsend [:mossop]]

I think what you want to do is to move the calls to addBuiltinTheme to just before the add-ons manager gets started up. You can add a listener for the profile-do-change topic to browserblue's init function (https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#389). profile-do-change is called immediately before the add-ons manager startup (http://searchfox.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#1150).

At first glance that should all be safe but you might find you have to tinker with the LWT code to make sure all the data structures it needs for those calls to succeed are initialised early enough.

Comment 10

[Andy McKay]

Complete themes are deprecated and will no longer be loaded in Firefox 57. From what I can see so far, this looks like quite a tricky bug for just a few releases. The number of complete theme users is small, is this worth the effort at this stage?

Suggesting moving down to a P5 or won't fixing.

Comment 11

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Andy McKay [:andym] from comment #10)
...
> Suggesting moving down to a P5 or won't fixing.

I ust assumed P3 would relegate this to the ages.

Comment 13

[:Gijs (he/him)]

We no longer support complete themes -> wontfix.