1331369 - Switching to Compact Light Theme sometimes wrongly shows message in the add-on manager indicating it requires a restart of Firefox

Status: RESOLVED DUPLICATE of bug 1205612

(Toolkit :: Add-ons Manager, defect)

Description

[Jarda Snajdr [:jsnajdr]]

Attachment: Screenshot of the bug

STR:
1. Open about:addons in Nightly, determined to try out the new compact themes from bug 1314091.
2. Click "Enable" several times on various themes (default, compact-dark, compact-light), somewhat randomly, to see how they look and compare
3. Open devtools to see how the compact themes look there
4. Start switching between the themes again

Actual results:
After performing these somewhat chaotic steps for a while, a notice saying that "Compact Light will be enabled after you restart Nightly" appeared on the compact-light theme, and I cannot enable it again. See attached screenshot.

I assume that things would get back in order after Firefox restart, but the restart shouldn't be required in the first place, should it?

Comment 1

[:Gijs (he/him)]

(In reply to Jarda Snajdr [:jsnajdr] from comment #0)
> I assume that things would get back in order after Firefox restart, but the
> restart shouldn't be required in the first place, should it?

No. Can you get more reliable steps to reproduce ?

Comment 2

[Jarda Snajdr [:jsnajdr]]

(In reply to :Gijs from comment #1)
> No. Can you get more reliable steps to reproduce ?

I recorded a short screencast here:

http://recordit.co/iQSculicwu

I'm switching between the themes with devtools opened. The green "restart needed" label appears almost on every switch for a fraction of a second (not visible on the screencast, as it seems to have a low FPS).

Sometimes, it stays displayed for several seconds - that's what happens near the end of the screencast - the label was there, and then disappeared. I didn't click on anything.

Comment 3

[:Gijs (he/him)]

Does this reproduce on a clean profile? Is this a debug build? I can't reproduce. :-\

Comment 4

[Jarda Snajdr [:jsnajdr]]

I use the official Nightly build. On a clean profile, the bug doesn't reproduce.

This is my list of addons when it happens:
ADB Helper
Valence
RDP Inspector
Test Pilot
Min Vid
No More 404s
AdBlock Plus

I was unable to reduce the list to something shorter.

I did some debugging and found this:
- when a theme is being disabled, two events are dispatched: "onDisabling" and "onDisabled". Between these two events, a "need to restart to disable" warning is displayed.
- when a theme is being enabled, another two events: "onEnabling" and "onEnabled". Between these two events, a "need to restart to enable" is displayed.

Most of the time, these labels are not shown.
Sometimes, the "onEnabling" label is displayed for several seconds before disappearing.
Two or three times (out of hundreds of samples), the label stayed displayed forever.

This happened even with devtools closed, so devtools are probably not needed to reproduce.

This is all I can find, sorry. Feel free to close the bug as worksforme/incomplete if nobody else is able to reproduce.

Comment 5

[:Gijs (he/him)]

(In reply to Jarda Snajdr [:jsnajdr] from comment #4)
> I use the official Nightly build. On a clean profile, the bug doesn't
> reproduce.
> 
> This is my list of addons when it happens:
> ADB Helper
> Valence
> RDP Inspector
> Test Pilot
> Min Vid
> No More 404s
> AdBlock Plus
> 
> I was unable to reduce the list to something shorter.
> 
> I did some debugging and found this:
> - when a theme is being disabled, two events are dispatched: "onDisabling"
> and "onDisabled". Between these two events, a "need to restart to disable"
> warning is displayed.
> - when a theme is being enabled, another two events: "onEnabling" and
> "onEnabled". Between these two events, a "need to restart to enable" is
> displayed.
> 
> Most of the time, these labels are not shown.
> Sometimes, the "onEnabling" label is displayed for several seconds before
> disappearing.
> Two or three times (out of hundreds of samples), the label stayed displayed
> forever.
> 
> This happened even with devtools closed, so devtools are probably not needed
> to reproduce.
> 
> This is all I can find, sorry. Feel free to close the bug as
> worksforme/incomplete if nobody else is able to reproduce.

I think this is fair enough, even if I can't repro, we just shouldn't be showing that notification at all when switching between a theme that supports lwthemes and different lwthemes. The add-on manager has enough information for this.

It might still end up wontfix given that we're going to be messing with themes anyway... but let's ask first. :-)

Andrew, am I right in thinking you're doing stuff with the addons manager these days? Any idea what's going on here?

Comment 6

[Andrew Swan [:aswan]]

Oh boy, themes are a dark corner of the add-ons manager that I don't understand well.  But even with the new theme API work, a bunch of the logic involved in this bug (e.g., deactivating the current theme when enabling a new theme) isn't going anywhere so lets get to the bottom of this.

I think these notifications come from the onDisabling and onEnabling events, based on the "needsRestart" flag.  But my first big question is why we're getting these events in the first place.  "Compact Light" and "Compact Dark" are built-in themes, they are not actually stored in XPIs, so we shouldn't be getting those events.  I'm unable to reproduce this but Jarda, it sounds like you can and like you are proficient with the Browser Toolbox.  Can you capture a full stack trace for an onDisabling or onEnabling event on a non-default theme?

Also cc'ing Mossop to straighten me out in case I'm completely barking up the wrong tree...

Comment 7

[:Gijs (he/him)]

Tweaking the summary to make it clearer that the theme is immediately applied - it's just the messaging that's wrong.

Comment 8

[Jarda Snajdr [:jsnajdr]]

(In reply to Andrew Swan [:aswan] from comment #6)
> I'm unable to reproduce this but Jarda, it sounds like you can and like you are
> proficient with the Browser Toolbox.  Can you capture a full stack trace for an
> onDisabling or onEnabling event on a non-default theme?

This is what I got after setting a breakpoint in the _updateState method in extensions.xml:

_updateState extensions.xml:1180
onDisabling extensions.xml:1609
delegateAddonEvent extensions.js:622
initialize/this[event] extensions.js:509
set_userDisabled extensions.xml:1085
oncommand about:addons:1

_updateState extensions.xml:1180
onDisabled extensions.xml:1615
delegateAddonEvent extensions.js:622
initialize/this[event] extensions.js:509
set_userDisabled extensions.xml:1085
oncommand about:addons:1

_updateState extensions.xml:1180
onEnabling extensions.xml:1597
delegateAddonEvent extensions.js:622
initialize/this[event] extensions.js:509
set_userDisabled extensions.xml:1085
oncommand about:addons:1

_updateState extensions.xml:1180
onEnabled extensions.xml:1603
delegateAddonEvent extensions.js:622
initialize/this[event] extensions.js:509
set_userDisabled extensions.xml:1085
oncommand about:addons:1

The breakpoint hits four times when switching from one theme to another. Between hits 1-2, a "restart to disable" warning is shown. Between hits 3-4, a "restart to enable" is shown.

I'm not sure how complete and useful these stack traces are. It's XBL code, and I don't know how much is the new debugger proficient working with that.

Sorry it took me a few days to respond.

Comment 9

[:Gijs (he/him)]

Paging Andrew for comment #8.

Comment 10

[Andrew Swan [:aswan]]

Hm.  I have to admit that the relationship between LightweightThemeManager.jsm and XPIProvider.jsm and how they are meant to work together is making my head spin.

It looks like the needsRestart flag is set to true if a particular pref has a non-default setting:
http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/toolkit/mozapps/extensions/LightweightThemeManager.jsm#664-665
Can you check the value of general.skins.selectedSkin in about:config?  (it may change as you select new themes...)

But when I try this locally, that preference never seems to change, it seems that LightweightThemeManager uses the preference lightweightThemes.selectedThemeID.  There is code in XPIProvider.jsm that sets the general.skins.selectedSkin preference but as I read it, it should only apply when you switch to a theme that is stored in an XPI, but (I don't think that) that is the case here.  Can you grab the full list of extensions you have installed (from about:support) and include it as well as the answer to the above question?

Comment 11

[Jarda Snajdr [:jsnajdr]]

(In reply to Andrew Swan [:aswan] from comment #10)
> It looks like the needsRestart flag is set to true if a particular pref has
> a non-default setting:
> http://searchfox.org/mozilla-central/rev/
> 30fcf167af036aeddf322de44a2fadd370acfd2f/toolkit/mozapps/extensions/
> LightweightThemeManager.jsm#664-665
> Can you check the value of general.skins.selectedSkin in about:config?  (it
> may change as you select new themes...)

general.skins.selectedSkin has the default value for me ("classic/1.0") and never changes. When setting a breakpoint on the line that evaluates needsRestart, it always evaluates to false.

It's the "lightweightThemes.selectedThemeID" pref that stores the selected theme. Looking at about:config, I also see a pref named:

services.sync.prefs.sync.lightweightThemes.selectedThemeID

set to "true". I'm logged in to Firefox Sync in the profile where this happens. Maybe a clue? Setting this pref to "false" doesn't change anything though - when switching themes, the restart warnings still show up for a moment.

> Can you grab the full list of extensions
> you have installed (from about:support) and include it as well as the answer
> to the above question?

My list of extensions from about:support:

Name: ADB Helper
Version: 0.9.2
Enabled: true
ID: [e-mail]

Name: Adblock Plus
Version: 2.8.2
Enabled: true
ID: {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}

Name: Application Update Service Helper
Version: 1.0
Enabled: true
ID: [e-mail]

Name: FlyWeb
Version: 1.0.0
Enabled: true
ID: [e-mail]

Name: Form Autofill
Version: 1.0
Enabled: true
ID: [e-mail]

Name: Min Vid
Version: 0.3.4
Enabled: true
ID: @min-vid

Name: Multi-process staged rollout
Version: 1.7
Enabled: true
ID: [e-mail]

Name: No More 404s
Version: 1.7.0
Enabled: true
ID: [e-mail]

Name: Pocket
Version: 1.0.5
Enabled: true
ID: [e-mail]

Name: Presentation
Version: 1.0.0
Enabled: true
ID: [e-mail]

Name: RDP Inspector
Version: 1.2.5
Enabled: true
ID: [e-mail]

Name: SHA-1 deprecation staged rollout
Version: 1.0
Enabled: true
ID: [e-mail]

Name: Shield Recipe Client
Version: 1.0.0
Enabled: true
ID: [e-mail]

Name: Test Pilot
Version: 0.9.3-dev-e085559
Enabled: true
ID: @testpilot-addon

Name: Valence
Version: 0.3.7
Enabled: true
ID: [e-mail]

Name: Web Compat
Version: 1.0
Enabled: true
ID: [e-mail]

Name: WebCompat Reporter
Version: 1.0.0
Enabled: true
ID: [e-mail]

Comment 12

[:shell escalante]

so we are having a hard time replicating this, though it would be an issue - can someone from QA help with replication scenario steps

Comment 13

[krupa raj [:krupa--use this to needinfo]]

I have been unsuccessful in reproducing this issue following the steps (and, with all the add-ons installed)

Ica, can you give it a try?

Comment 14

[Vasilica Tamas, QA [:vtamas]]

I was unable to reproduce this issue using a clean profile on Firefox 54.0a1 (2017-02-15) under Windows 10 64-bit. 

But based on the following paragraph mentioned in Comment 11

> set to "true". I'm logged in to Firefox Sync in the profile where this
> happens. Maybe a clue? Setting this pref to "false" doesn't change anything
> though - when switching themes, the restart warnings still show up for a
> moment.

I’m suspecting that this is a dupe of Bug 1205612 because I successfully reproduced this issue while being logged into Firefox Account - see screencast: https://www.screencast.com/t/lyvnCu1c4

So my guess is that this is a sync issue and not a compact theme specific bug.

Jarda, could you please check if you see the same “Restart” message when enabling a different lightweight theme on the same profile? Such in the following screencast: https://www.screencast.com/t/FuV7wvSeH

Comment 15

[Jarda Snajdr [:jsnajdr]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #14)
> Jarda, could you please check if you see the same “Restart” message when
> enabling a different lightweight theme on the same profile? Such in the
> following screencast: https://www.screencast.com/t/FuV7wvSeH

Hi Vasilica, I tried out the "Symphony of Colors" theme and the restart message appears there, too. So it's not specific to the DevEdition themes.

> So my guess is that this is a sync issue and not a compact theme specific
> bug.

I agree that a Sync delay is the most probable cause for this bug.

Comment 16

[Vasilica Tamas, QA [:vtamas]]

Thanks Jarda for your reply. Based on Comment 14 and Comment 15 I am marking this bug as Duplicate.

Comment 17

[Vasilica Tamas, QA [:vtamas]]

Removing the qawanted keyword since this bug was marked as duplicate.