1450975 - Somehow Lightweight themes lose their accentcolor after update, resulting in the lwt footer and header images failing to apply

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Artem S. Tashkinov]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180322140748

Steps to reproduce:

Install https://addons.mozilla.org/firefox/addon/flag-of-co-385587-2/

It works. Exit the browser. Run it ~24 hours later.


Actual results:

This theme doesn't get applied - Firefox starts with the default theme.

Trying to select this theme in the customizations and add-ons does _not_ work either - you select it and nothing happens.


Expected results:

A custom theme should keep on working.

The only workaround is to open add-ons, uninstall it completely, go to its home page and install it again. It will work then for a little while: launching the browser less than an hour after installing the theme doesn't break it. Run the browser several hours later  (> 8 in my case) and the theme will fail to apply.

Info: Firefox for Linux 60.0b8 (64-bit), the official build from ftp.mozilla.org.

This has never been an issue with previous Firefox releases. Firefox 59.0.2 works just fine here. Firefox 60 beta has always had this problem.

Comment 1

[Artem S. Tashkinov]

This bug is most likely a dupe of bug 1446183.

Comment 2

[:Gijs (out for now; he/him)]

Do you have sync set up? Can you reproduce in a clean profile?

Comment 4

[:Gijs (out for now; he/him)]

Also, can you reproduce on nightly? If so, any chance you can find a regression window?

Comment 5

[Artem S. Tashkinov]

Sync is indeed set up - all the checkboxes are selected.

However that doesn't explain why ***I cannot apply a custom theme after a certain period unless I reinstall it***.

Before I try nightly - will it break my profile? I just want to be sure 'cause I've heard that some versions of Firefox make it impossible to downgrade.

Also, instead of trying Nightly, are there any command line flags I could try to see any errors related to the handling of custom themes?

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Artem S. Tashkinov from comment #5)
> Sync is indeed set up - all the checkboxes are selected.
> 
> However that doesn't explain why ***I cannot apply a custom theme after a
> certain period unless I reinstall it***.

I'm not saying there's not a bug, I'm wondering if sync be where the bug comes from. We don't have code that re-sets preferences willy-nilly...

> Before I try nightly - will it break my profile? I just want to be sure
> 'cause I've heard that some versions of Firefox make it impossible to
> downgrade.

Yeah, you should try nightly with a new profile. More generally, it'd be interesting if you could reproduce with a clean profile.

> Also, instead of trying Nightly, are there any command line flags I could
> try to see any errors related to the handling of custom themes?

I don't think so, but maybe sync logs would help. Kit or Thom, can you help diagnose the issue further?

Comment 7

[Artem S. Tashkinov]

Another interesting observation: even when the issue arises I still can apply all the built-in themes: default, light, dark and space fantasy (it's not exactly built-in but it comes preinstalled with Firefox).

However when I try to choose my custom theme the default theme gets applied instead.

Again, reinstallation temporarily fixes the issue.

That's all extremely weird ;-)

Comment 8

[Artem S. Tashkinov]

This issue is reproducible with  	firefox-61.0a1.en-US.linux-x86_64.tar.bz2 	59M 	03-Apr-2018 23:31

Comment 9

[:Gijs (out for now; he/him)]

(In reply to Artem S. Tashkinov from comment #8)
> This issue is reproducible with  	firefox-61.0a1.en-US.linux-x86_64.tar.bz2 
> 59M 	03-Apr-2018 23:31

On a clean profile, without signing into sync?

Comment 10

[:Gijs (out for now; he/him)]

:ntim, are you aware of any of these issues?

Comment 11

[Artem S. Tashkinov]

An old profile with sync enabled. With a new profile I have to wait for at least a couple of hours to be sure the issue is reproducible.

Comment 12

[Artem S. Tashkinov]

Actually I reluctant to try to reproduce this issue in nightly, because the issue is

**I cannot apply a custom theme after a certain period of time after applying it for the first time**.

This must have nothing to do with sync.

Lightweight themes handling in Firefox 60 beta for Linux is just broken - with or without sync.

If you insist I will try to reproduce it in nightly - I just don't think it makes any sense.

Comment 13

[:Gijs (out for now; he/him)]

(In reply to Artem S. Tashkinov from comment #12)
> Actually I reluctant to try to reproduce this issue in nightly, because the
> issue is
> 
> **I cannot apply a custom theme after a certain period of time after
> applying it for the first time**.
> 
> This must have nothing to do with sync.
> 
> Lightweight themes handling in Firefox 60 beta for Linux is just broken -
> with or without sync.

You've only tested profiles with sync, so how do you know that?

FWIW, I can't reproduce with a clean profile - I install a theme, it works, I close the browser, I reopen the browser, it's still there. This is the point of asking you all these questions - without being able to reproduce, I can't answer them myself.

If there is some requirement for a time-delay between exit + restart for it to break, it'd be good to narrow down what that time-delay is (and noting it on the bug), and then just trying a clean profile with the same build (without signing into sync) to see if it reproduces without sync. But nothing will be running after exiting Firefox, so the time delay having an impact here doesn't make a lot of sense to me.

> If you insist I will try to reproduce it in nightly - I just don't think it
> makes any sense.

It's not so much about a specific build as figuring out when stuff broke, as you said you think this is a regression, which must mean it broke sometime. Finding out when it broke helps narrow down what broke it. There are literally 1000s of changesets that landed between 59 beta and 60 beta, so just knowing it broke between those 2 points doesn't help much. Normally we find a regression window with nightly, hence asking whether it reproduces there (or is somehow beta-specific). mozregression would be the best way of finding a regression window, but without knowing how long it takes between exiting and restarting to reproduce reliably would make it hard to run a regression hunt.

Comment 14

[:Gijs (out for now; he/him)]

When the default theme shows up when another is supposed to be selected, are there any errors in the browser console? Do (more) errors appear if you try to select another theme?

Comment 15

[Tim Nguyen :ntim]

I'm not aware of such issues with LWTs.

Comment 16

[Artem S. Tashkinov]

No errors are logged in the browser console or terminal.

If you're interested I can email you my entire firefox profile sans passwords, cookies and history - so that you could reproduce it easily.

Comment 17

[:Gijs (out for now; he/him)]

(In reply to Artem S. Tashkinov from comment #16)
> No errors are logged in the browser console or terminal.
> 
> If you're interested I can email you my entire firefox profile sans
> passwords, cookies and history - so that you could reproduce it easily.

Sure, though you'd need to remove sync credentials as well (otherwise passwords and history will just start syncing), and then it might not reproduce anymore...

Comment 18

[Artem S. Tashkinov]

I've just emailed you my profile.

Check it out.

Comment 19

[:Gijs (out for now; he/him)]

(In reply to Artem S. Tashkinov from comment #18)
> I've just emailed you my profile.
> 
> Check it out.

So this profile started up with the "flag of colombia" theme selected when checking in about:addons. Switching to other themes worked, so I couldn't reproduce the issue you filed with it.

The only issue I saw was that none of the background images loaded for the theme, which I assume is because normally those are cached and the profile you sent me was very minimal, and presumably lost track of where those cached files were or whatever.

Is it possible you have any other add-ons installed that might be interfering with the theme that weren't included in the profile you sent? (x-ref bug 1451012)

Comment 20

[Artem S. Tashkinov]

Could you switch to this theme? I presume not. Now I don't understand why you're saying that you cannot reproduce this bug. All the pertinent files are in it:

.:
total 188
drwx------.  3 birdie birdie    140 Apr  4 12:26 .
drwxrwxrwt. 19 root   root      540 Apr  4 13:17 ..
-rw-rw-r--.  1 birdie birdie  58744 Apr  4 12:09 lightweighttheme-footer
-rw-rw-r--.  1 birdie birdie 101323 Apr  4 12:09 lightweighttheme-header
drwxr-xr-x.  2 birdie birdie     80 Apr  4 12:09 lwtheme
-rw-------.  1 birdie birdie  19702 Apr  4 12:26 prefs.js
-rw-------.  1 birdie birdie   6026 Apr  4 12:10 xulstore.json

./lwtheme:
total 132
drwxr-xr-x. 2 birdie birdie    80 Apr  4 12:09 .
drwx------. 3 birdie birdie   140 Apr  4 12:26 ..
-rw-r--r--. 1 birdie birdie 44423 Apr  4 12:09 lightweighttheme-footer-1920x1080
-rw-r--r--. 1 birdie birdie 87290 Apr  4 12:09 lightweighttheme-header-1920x1080

The only extra addons that I have installed are:

* LastPass
* uBlock Origin

If I disable them the bug doesn't go away.

Comment 21

[Artem S. Tashkinov]

The four graphical files included in this profile are _exactly_ this theme.

Firefox is unable to load them for the reasons I'm unable to comprehend.

Comment 22

[Artem S. Tashkinov]

You're free to share this profile with anyone - there's barely any private info left in it (I may have left pieces in prefs.js but I guess nothing serious).

Comment 23

[:Gijs (out for now; he/him)]

(In reply to Artem S. Tashkinov from comment #20)
> Could you switch to this theme? I presume not.

It's already marked as selected. I can switch to another theme and then back to it without issues (using about:addons). Your issue is that (a) it's not selected and (b) you can't switch to another theme (per comment #0).

Comment 24

[vbnm123c]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1446183

1) Whether you're using Firefox Sync or not
   
   Yes, always active.

2) What's your Firefox version and where you installed it from

   https://ftp.mozilla.org/pub/firefox/releases/<58.*>/linux-x86_64/zh-TW/
   and update automaticlly.
   
   ![about:buildconfig](https://i.imgur.com/ui5Ldv8.png)

3) What other add-ons you have installed and active
   
   I just use the default profile instead of a clean profile to test this bug.
   So, a lot of extensions are installed and active.

   ![1st page](https://i.imgur.com/g5nEbg7.png)
   ![2nd page](https://i.imgur.com/v4wF1g4.png)

4) Whether you have these files in your profile

   ~/.mozilla/firefox/mwad0hks.default $ ll **/lightweighttheme* lightweighttheme*
   -rw-r--r-- 1 flandre flandre  22963  4月  4 21:34 lightweighttheme-footer
   -rw-r--r-- 1 flandre flandre 180979  4月  4 21:34 lightweighttheme-header
   -rw-r--r-- 1 flandre flandre  26090  4月  4 21:34 lwtheme/lightweighttheme-footer-1920x1080
   -rw-r--r-- 1 flandre flandre 190473  4月  4 21:34 lwtheme/lightweighttheme-header-1920x1080

   they belong to the theme you have selected ✔️

5) Specify a link to the theme you're using

   https://addons.mozilla.org/zh-TW/firefox/addon/flandre-free/

Comment 25

[Artem S. Tashkinov]

(In reply to :Gijs (he/him) from comment #23)
> (In reply to Artem S. Tashkinov from comment #20)
> > Could you switch to this theme? I presume not.
> 
> It's already marked as selected. I can switch to another theme and then back
> to it without issues (using about:addons). Your issue is that (a) it's not
> selected and (b) you can't switch to another theme (per comment #0).

You can actually select my custom theme and it gets applied? It's not obvious from your comment.

In my case I select it and nothing happens - I see the default theme being applied instead.

Comment 26

[Artem S. Tashkinov]

(In reply to vbnm123c from comment #24)

> 2) What's your Firefox version and where you installed it from
> 
>    https://ftp.mozilla.org/pub/firefox/releases/<58.*>/linux-x86_64/zh-TW/
>    and update automaticlly.

Are you saying you're using Firefox 58? Originally you made it sound like you were on a beta release.

Comment 27

[vbnm123c]

(In reply to Artem S. Tashkinov from comment #26)
> (In reply to vbnm123c from comment #24)
> 
> > 2) What's your Firefox version and where you installed it from
> > 
> >    https://ftp.mozilla.org/pub/firefox/releases/<58.*>/linux-x86_64/zh-TW/
> >    and update automaticlly.
> 
> Are you saying you're using Firefox 58? Originally you made it sound like
> you were on a beta release.

Now is 60b9 but the binary I installed that time is 58.
It updates automatically. :D

Comment 28

[:Gijs (out for now; he/him)]

(In reply to Artem S. Tashkinov from comment #25)
> (In reply to :Gijs (he/him) from comment #23)
> > (In reply to Artem S. Tashkinov from comment #20)
> > > Could you switch to this theme? I presume not.
> > 
> > It's already marked as selected. I can switch to another theme and then back
> > to it without issues (using about:addons). Your issue is that (a) it's not
> > selected and (b) you can't switch to another theme (per comment #0).
> 
> You can actually select my custom theme and it gets applied? It's not
> obvious from your comment.

It becomes the selected theme, yes. There are issues with applying the header/footer images, as I already said, but otherwise things work fine. I can tell the difference with the default theme because of the selected tab foreground colour (though that may depend on your GTK/window-manager theme).

What does the add-on manager look like you for you after a restart where you see the default theme visually, but you expect your theme to be selected - can you post a screenshot?

Comment 29

[vbnm123c]

(In reply to :Gijs (he/him) from comment #28)
> It becomes the selected theme, yes. There are issues with applying the
> header/footer images, as I already said, but otherwise things work fine. I
> can tell the difference with the default theme because of the selected tab
> foreground colour (though that may depend on your GTK/window-manager theme).
> 
> What does the add-on manager look like you for you after a restart where you
> see the default theme visually, but you expect your theme to be selected -
> can you post a screenshot?

If the bug works, the theme description will disappear.
https://i.imgur.com/vV4dzqp.png

If the theme works fine, the theme description will show.
https://i.imgur.com/gOkeevx.png

Comment 30

[:Gijs (out for now; he/him)]

OK, I think my confusion here is that comment #0 implies it's impossible for the reporter to select *any* theme, including (for instance) the builtin light/dark theme after startup. This is a sentiment that is repeated in comment #5. That's not what I'm seeing. I can change themes fine. The issue seems to be with the header/footer not applying correctly, which then gives the impression that selecting the theme "just" applies the default theme, despite there being some differences anyway.

It seems this is because 'active' is false, which is because the theme lacks an accentcolor property, which changed with bug 1404688 (before, it'd check for there being a headerURL), which matches this being a regression in 60 beta. So my current suspicion is that while, when initially installed, the theme has an accentcolor, it gets lost somehow (it's not in the prefs.js version that is in the profile Artem sent me), and that breaks applying the theme. :ntim, can you have a look?

:kit/:thom, is it possible the accent color gets dropped by sync? How do we sync lwthemes?

vbnm123c, can you confirm that if you copy the value of lightweightThemes.usedThemes in about:config, look for the object that has the ID that's in lightweightThemes.selectedThemeID , and check that it has an 'accentcolor' property? (If you're not sure how to do that, copy-pasting the entire value of `lightweightThemes.usedThemes` and noting what is the value of `selectedThemeID` would work.)

Comment 31

[vbnm123c]

(In reply to :Gijs (he/him) from comment #30)
> vbnm123c, can you confirm that if you copy the value of
> lightweightThemes.usedThemes in about:config, look for the object that has
> the ID that's in lightweightThemes.selectedThemeID , and check that it has
> an 'accentcolor' property? (If you're not sure how to do that, copy-pasting
> the entire value of `lightweightThemes.usedThemes` and noting what is the
> value of `selectedThemeID` would work.)

lightweightThemes.usedThemes (formatted):
[{
  "id": "716739",
  "name": "Flandre:Free",
  "headerURL": "https://addons.cdn.mozilla.net/user-media/addons/716739/header.png?modified=af683c38",
  "footerURL": "https://addons.cdn.mozilla.net/user-media/addons/716739/footer.png?modified=af683c38",
  "textcolor": "#000000",
  "accentcolor": "#",
  "iconURL": "https://addons.cdn.mozilla.net/user-media/addons/716739/icon.png?modified=af683c38",
  "previewURL": "https://addons.cdn.mozilla.net/user-media/addons/716739/preview.png?modified=af683c38",
  "author": "フラン♥",
  "description": "感謝 6U☆ 大大漂亮的作品",
  "updateURL": "https://versioncheck.addons.mozilla.org/zh-TW/themes/update-check/716739",
  "version": "1.0"
}]
lightweightThemes.selectedThemeID: 716739

After I change selectedThemeID to empty string, the theme works fine.
I can wait until the bug appears and check these config properties.

Comment 32

[:Gijs (out for now; he/him)]

(In reply to vbnm123c from comment #31)
> After I change selectedThemeID to empty string, the theme works fine.
> I can wait until the bug appears and check these config properties.

Yes, sorry, I meant what are the values once this bug appears, please. :-)

Comment 33

[Thom Chiovoloni [:tcsc] (ex-moco)]

Markh would be better to ask about this since he wrote the code for syncing themes. My understanding of what happens, which may be incorrect in parts is:

1. The theme is an addon (Apparently this may be untrue, and that the real place themes live is in the lightweightThemes.usedThemes pref? I don't know), and so it's synced as normal with addons.

The code for addon sync is somewhat complex so it's completely possible that there's a bug in it, but for the most part we just ask the addonmanager to install the latest version of the given addon that works on your Firefox.

2. Your current theme is synced using the prefs engine. The two prefs for themes that are synced are `lightweightThemes.selectedThemeID` and `lightweightThemes.usedThemes`. We don't try an interpret the data in a pref at all, and they aren't synced incrementally (e.g. we sync all your prefs every time, not only the ones that changed).

3. When we see that the `lightweightThemes.selectedThemeID` preference is changed after a pref sync, we change your theme using the LightweightThemeManager with something that amounts to `LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme(themeID)` [0].

I feel like there is a possible issue here if themes need to be installed by installing an addon, in that we will try to sync prefs before addons (since the prefs engine has the lower syncPriority value).

But OTOH if themes actually live in the lightweightThemes.usedThemes pref, then there seems to be an issue in that they have a version number that (AFAICT) might different between two different versions of and Firefox (I seem to get different versions from installing https://addons.mozilla.org/en-US/firefox/addon/quantum-launch/ on release vs nightly).

[0]: https://searchfox.org/mozilla-central/source/services/sync/modules/engines/prefs.js#129-135

Comment 34

[Artem S. Tashkinov]

Attachment: A custom theme selected

Comment 35

[Artem S. Tashkinov]

[
	{
		"id": "385587",
		"name": "Flag of Colombia",
		"headerURL": "https://addons.cdn.mozilla.net/user-media/addons/385587/Persona_columbia.jpg?modified=31f490b6",
		"footerURL": "https://addons.cdn.mozilla.net/user-media/addons/385587/footer_persona_columbia.jpg?modified=31f490b6",
		"iconURL": "https://addons.cdn.mozilla.net/user-media/addons/385587/preview_small.jpg?modified=31f490b6",
		"previewURL": "https://addons.cdn.mozilla.net/user-media/addons/385587/preview.jpg?modified=31f490b6",
		"author": "MozThemes",
		"updateURL": "https://versioncheck.addons.mozilla.org/en-US/themes/update-check/385587",
		"version": "0"
	},
	{
		"id": "recommended-2",
		"name": "Space Fantasy",
		"headerURL": "resource:///chrome/browser/content/browser/defaultthemes/2.header.jpg",
		"textcolor": "#ffffff",
		"accentcolor": "#d9d9d9",
		"iconURL": "resource:///chrome/browser/content/browser/defaultthemes/2.icon.jpg",
		"previewURL": "resource:///chrome/browser/content/browser/defaultthemes/2.preview.jpg",
		"author": "fx5800p",
		"description": "Space Fantasy is (C) fx5800p. Available under CC-BY-SA. No warranty.",
		"homepageURL": "https://addons.mozilla.org/firefox/addon/space-fantasy/",
		"version": "1.0"
	}
]

Comment 36

[Artem S. Tashkinov]

I've found an interesting preference:

services.sync.prefs.sync.lightweightThemes.usedThemes

I will try to set it to true and report whether it makes any difference.

Comment 37

[:Gijs (out for now; he/him)]

(In reply to Artem S. Tashkinov from comment #36)
> I've found an interesting preference:
> 
> services.sync.prefs.sync.lightweightThemes.usedThemes
> 
> I will try to set it to true and report whether it makes any difference.

It's set to true by default...

Comment 38

[Artem S. Tashkinov]

(In reply to :Gijs (he/him) from comment #37)
> (In reply to Artem S. Tashkinov from comment #36)
> > I've found an interesting preference:
> > 
> > services.sync.prefs.sync.lightweightThemes.usedThemes
> > 
> > I will try to set it to true and report whether it makes any difference.
> 
> It's set to true by default...

My bad - I meant "false".

Comment 39

[vbnm123c]

(In reply to :Gijs (he/him) from comment #32)
> (In reply to vbnm123c from comment #31)
> > After I change selectedThemeID to empty string, the theme works fine.
> > I can wait until the bug appears and check these config properties.
> 
> Yes, sorry, I meant what are the values once this bug appears, please. :-)

Here is a video when the bug appeared.
https://youtu.be/CxqIH2D4Jkk

Comment 40

[Thom Chiovoloni [:tcsc] (ex-moco)]

(In reply to Artem S. Tashkinov from comment #36)
> I've found an interesting preference:
> 
> services.sync.prefs.sync.lightweightThemes.usedThemes
> 
> I will try to set it to true and report whether it makes any difference.

This pref just controls whether or not `lightweightThemes.usedThemes` is synced. That's the case for all `services.sync.prefs.sync.*` prefs, they control if the pref that comes after is synced.

Comment 41

[Mark Hammond [:markh] [:mhammond]]

(In reply to Thom Chiovoloni [:tcsc] from comment #33)
> Markh would be better to ask about this since he wrote the code for syncing
> themes.

Not really - I did however land the change that took extra action when the lightweightThemes.selectedThemeID preference was synced in bug 1287748.

I also can't reproduce the issue with that specific theme on 2 Windows devices.

Artem, how many other desktop devices do you have connected to Sync, and do those devices see the same basic behaviour? (ie, do they see the theme as being installed and active before reverting)?

Comment 42

[Artem S. Tashkinov]

(In reply to Mark Hammond [:markh] from comment #41)
> 
> Artem, how many other desktop devices do you have connected to Sync, and do
> those devices see the same basic behaviour? (ie, do they see the theme as
> being installed and active before reverting)?

I have just two devices syncing: my laptop and desktop. Both exhibit the same behaviour.

services.sync.prefs.sync.lightweightThemes.usedThemes = FALSE has seemingly "fixed" the issue for me.

Comment 43

[:Gijs (out for now; he/him)]

(In reply to vbnm123c from comment #39)
> (In reply to :Gijs (he/him) from comment #32)
> > (In reply to vbnm123c from comment #31)
> > > After I change selectedThemeID to empty string, the theme works fine.
> > > I can wait until the bug appears and check these config properties.
> > 
> > Yes, sorry, I meant what are the values once this bug appears, please. :-)
> 
> Here is a video when the bug appeared.
> https://youtu.be/CxqIH2D4Jkk

Yeah, the accentcolor is missing there too. So somehow, that property is disappearing out of the json that we sync... It's still not clear to me when/how, as none of us have managed to reproduce the issue starting with a working profile, so it's not clear what sequence of events is causing this to happen.

Comment 44

[Tim Nguyen :ntim]

Could be related to the "#" character, that might not be properly escaped and might cause issues when thinking. Not sure.

Comment 45

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs (he/him) from comment #43)
> So somehow, that property is
> disappearing out of the json that we sync...

FWIW, There's no addon/theme specific state synced between devices. I wonder if the theme is somehow "failing" on one of the devices causing a sync-war between them as the preference is reset?

Comment 46

[Artem S. Tashkinov]

(In reply to :Gijs (he/him) from comment #43)
> 
> Yeah, the accentcolor is missing there too. So somehow, that property is
> disappearing out of the json that we sync... It's still not clear to me
> when/how, as none of us have managed to reproduce the issue starting with a
> working profile, so it's not clear what sequence of events is causing this
> to happen.

Speaking of reproducibility: you have my profile which fails to load a lightweight theme for no obvious reasons.

Comment 47

[:Gijs (out for now; he/him)]

(In reply to Artem S. Tashkinov from comment #46)
> (In reply to :Gijs (he/him) from comment #43)
> > 
> > Yeah, the accentcolor is missing there too. So somehow, that property is
> > disappearing out of the json that we sync... It's still not clear to me
> > when/how, as none of us have managed to reproduce the issue starting with a
> > working profile, so it's not clear what sequence of events is causing this
> > to happen.
> 
> Speaking of reproducibility: you have my profile which fails to load a
> lightweight theme for no obvious reasons.

The reason is that the accentcolor property is missing.

The problem is we don't understand *how* it came to be that the accentcolor property is missing in that profile. It's there when the theme is installed from AMO. Somehow, it disappears at some point. We don't know why/how, and have no steps to go from a situation where it's working to a situation where it's not. The copy of your profile where things are broken doesn't help in that regard - it's how the profile got into that state that we need to figure out.

(In reply to Mark Hammond [:markh] from comment #45)
> (In reply to :Gijs (he/him) from comment #43)
> > So somehow, that property is
> > disappearing out of the json that we sync...
> 
> FWIW, There's no addon/theme specific state synced between devices. I wonder
> if the theme is somehow "failing" on one of the devices causing a sync-war
> between them as the preference is reset?

I mean, if bogus data gets saved in one location, it'll inevitably get synced to the others, right? I still don't understand why/how bogus data gets saved, though.

Comment 48

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs (he/him) from comment #47)
> I mean, if bogus data gets saved in one location,

Saved where?

> it'll inevitably get
> synced to the others, right? I still don't understand why/how bogus data
> gets saved, though.

Only if that data is saved in a very specific set of places.

Comment 49

[:Gijs (out for now; he/him)]

(In reply to Mark Hammond [:markh] from comment #48)
> (In reply to :Gijs (he/him) from comment #47)
> > I mean, if bogus data gets saved in one location,
> 
> Saved where?

To the synced pref, on one of the machines signed into sync.

Comment 50

[Artem S. Tashkinov]

This is getting crazy. I'm now talking only about my desktop PC (I last ran my laptop several days ago, so it's not syncing or polluting sync).

Several hours ago I reenabled services.sync.prefs.sync.lightweightThemes.usedThemes and then exited the web browser.

Now I've just run it again and the issue has re-arisen.

It surely looks like sync is messing with theming.

This is the value of lightweightThemes.usedThemes (with the problem):

[
	{
		"id": "385587",
		"name": "Flag of Colombia",
		"headerURL": "https://addons.cdn.mozilla.net/user-media/addons/385587/Persona_columbia.jpg?modified=31f490b6",
		"footerURL": "https://addons.cdn.mozilla.net/user-media/addons/385587/footer_persona_columbia.jpg?modified=31f490b6",
		"iconURL": "https://addons.cdn.mozilla.net/user-media/addons/385587/preview_small.jpg?modified=31f490b6",
		"previewURL": "https://addons.cdn.mozilla.net/user-media/addons/385587/preview.jpg?modified=31f490b6",
		"author": "MozThemes",
		"updateURL": "https://versioncheck.addons.mozilla.org/en-US/themes/update-check/385587",
		"version": "0",
		"updateDate": 1523026921593,
		"installDate": 1523026921593
	},
	{
		"id": "recommended-2",
		"name": "Space Fantasy",
		"headerURL": "resource:///chrome/browser/content/browser/defaultthemes/2.header.jpg",
		"textcolor": "#ffffff",
		"accentcolor": "#d9d9d9",
		"iconURL": "resource:///chrome/browser/content/browser/defaultthemes/2.icon.jpg",
		"previewURL": "resource:///chrome/browser/content/browser/defaultthemes/2.preview.jpg",
		"author": "fx5800p",
		"description": "Space Fantasy is (C) fx5800p. Available under CC-BY-SA. No warranty.",
		"homepageURL": "https://addons.mozilla.org/firefox/addon/space-fantasy/",
		"version": "1.0"
	}
]

Comment 51

[Onno Witvliet]

I also have this problem with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:60.0) Gecko/20100101 Firefox/60.0
20180412172954 (60.0b12)

The problem seems to be triggered by an extension update check

The steps to reproduce the problem are:

- Create new profile
- Install theme from https://addons.mozilla.org/en-US/firefox/addon/iris-leaf (not all lightweight themes trigger this problem)
- Save lightweightThemes.usedThemes value
- Set extensions.update.interval to 120 (about:config)
- Wait at least 2 minutes until an extension background update check has been performed 
- Exit Firefox
- Start Firefox
- Problem appears
- Save lightweightThemes.usedThemes value

lightweightThemes.usedThemes after install:
[
  {
    'id': '46666',
    'name': 'Iris Leaf',
    'headerURL': 'https://addons.cdn.mozilla.net/user-media/addons/46666/irispersona.jpg?modified=1257075470',
    'footerURL': 'https://addons.cdn.mozilla.net/user-media/addons/46666/irisfooter.jpg?modified=1257075470',
    'textcolor': '#000000',
    'accentcolor': '#',
    'iconURL': 'https://addons.cdn.mozilla.net/user-media/addons/46666/preview_small.jpg?modified=1257075470',
    'previewURL': 'https://addons.cdn.mozilla.net/user-media/addons/46666/preview_large.jpg?modified=1257075470',
    'author': 'silfiriel',
    'description': 'Yellow and green soundly blended.',
    'updateURL': 'https://versioncheck.addons.mozilla.org/en-US/themes/update-check/46666',
    'version': '1.0',
    'updateDate': 1523822394340,
    'installDate': 1523822394340
  }
]

lightweightThemes.usedThemes after extension update check:
[
  {
    'id': '46666',
    'name': 'Iris Leaf',
    'headerURL': 'https://addons.cdn.mozilla.net/user-media/addons/46666/irispersona.jpg?modified=1257075470',
    'footerURL': 'https://addons.cdn.mozilla.net/user-media/addons/46666/irisfooter.jpg?modified=1257075470',
    'textcolor': '#000000',
    'iconURL': 'https://addons.cdn.mozilla.net/user-media/addons/46666/preview_small.jpg?modified=1257075470',
    'previewURL': 'https://addons.cdn.mozilla.net/user-media/addons/46666/preview.jpg?modified=1257075470',
    'author': 'silfiriel',
    'updateURL': 'https://versioncheck.addons.mozilla.org/en-US/themes/update-check/46666',
    'version': '0',
    'updateDate': 1523822514129,
    'installDate': 1523822394340
  }
]


The following differences exist after the update check:
- updateDate: differs from installDate
- version: changes from '1.0' to '0'
- accentcolor: disappears

Comment 52

[Mark Hammond [:markh] [:mhammond]]

(In reply to Onno Witvliet from comment #51)
> The problem seems to be triggered by an extension update check
> 
> The steps to reproduce the problem are:

Thanks! FWIW, I could reproduce this exactly as described, with a new profile. I could not reproduce it with one of my existing dev profiles, nor with an explicit "check for updates"

> - Wait at least 2 minutes until an extension background update check has
> been performed 
> - Exit Firefox
> - Start Firefox
> - Problem appears

FWIW, the "bad" lightweightThemes.usedThemes value appeared before the restart - the restart was only necessary to visually see the effect.

Comment 53

[Julien Cristau [:jcristau]]

Sounds like a bad experience, tracking for 60.  Hopefully with STR from comment 51 we can get to the bottom of this.

Comment 54

[Tim Nguyen :ntim]

Attachment: Bug 1450975 - Load themes without an accentcolor.

Review commit: https://reviewboard.mozilla.org/r/236808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236808/

Comment 55

[Tim Nguyen :ntim]

The patch I just pushed removes the accentcolor requirement from bug 1404688 if a headerURL is there. 

I'm not convinced this is the right fix however, and I think it would be much better to fix the underlying update/sync issue :)

Comment 56

[:Gijs (out for now; he/him)]

This is an issue with the server... when asking for an update (which you can simulate manually by running this in the browser console:

Cc["@mozilla.org/addons/integration;1"].getService(Ci.nsITimerCallback).notify(null)

The response for the "Flag of Colombia" theme is:

{
   "username":"MozThemes",
   "description":null,
   "detailURL":"https://addons.mozilla.org/en-US/addon/flag-of-co-385587-2/",
   "accentcolor":null,
   "iconURL":"https://addons.cdn.mozilla.net/user-media/addons/385587/preview_small.jpg?modified=31f490b6",
   "previewURL":"https://addons.cdn.mozilla.net/user-media/addons/385587/preview.jpg?modified=31f490b6",
   "textcolor":null,
   "id":"385587",
   "headerURL":"https://addons.cdn.mozilla.net/user-media/addons/385587/Persona_columbia.jpg?modified=31f490b6",
   "dataurl":"",
   "name":"Flag of Colombia",
   "author":"MozThemes",
   "updateURL":"https://versioncheck.addons.mozilla.org/en-US/themes/update-check/385587",
   "version":"0",
   "footerURL":"https://addons.cdn.mozilla.net/user-media/addons/385587/footer_persona_columbia.jpg?modified=31f490b6"
}


Which indeed shows accentcolor, textcolor and version being missing/broken when they shouldn't be. Grebs / :eviljeff, can you take a look?

I would expect this to also be broken for 59, but I haven't tested.

Comment 57

[:Gijs (out for now; he/him)]

(In reply to :Gijs (he/him) from comment #56)
> I would expect this to also be broken for 59, but I haven't tested.

Err, no, it won't be broken, because I guess those builds didn't require the accentcolor to be present...

Comment 58

[Artem S. Tashkinov]

I now experience this issue with a profile which does _not_ have sync enabled.

Firefox starts but only loads colors - it doesn't try to load header and footer images.

When I go to Customize and select the theme, it gets loaded.

The theme is https://addons.mozilla.org/firefox/addon/sunshine-on-bills/

Comment 59

[:Gijs (out for now; he/him)]

(In reply to Artem S. Tashkinov from comment #58)
> I now experience this issue with a profile which does _not_ have sync
> enabled.

Yes, per comment 51 (thanks very much for the clear steps, Onno!) I think it's clear that this is an issue with the add-on update server.

Comment 60

[Andrew Williamson [:eviljeff]]

(In reply to :Gijs (he/him) from comment #56)
> This is an issue with the server... when asking for an update (which you can
> simulate manually by running this in the browser console:
...
> "updateURL":"https://versioncheck.addons.mozilla.org/en-US/themes/update-
> check/385587",

This LWT doesn't have accentcolor or textcolor set, according to what I can see.  Is there an example of a theme that definately does/did have accentcolor/textcolor set and the update service is saying otherwise?

(I checked against the api response https://addons.mozilla.org/api/v3/addons/addon/385587/ -theme_data section- and then the edit theme page on AMO)

Comment 61

[:Gijs (out for now; he/him)]

(In reply to Andrew Williamson [:eviljeff] from comment #60)
> (In reply to :Gijs (he/him) from comment #56)
> > This is an issue with the server... when asking for an update (which you can
> > simulate manually by running this in the browser console:
> ...
> > "updateURL":"https://versioncheck.addons.mozilla.org/en-US/themes/update-
> > check/385587",
> 
> This LWT doesn't have accentcolor or textcolor set, according to what I can
> see.  Is there an example of a theme that definately does/did have
> accentcolor/textcolor set and the update service is saying otherwise?
> 
> (I checked against the api response
> https://addons.mozilla.org/api/v3/addons/addon/385587/ -theme_data section-
> and then the edit theme page on AMO)

Yes, but the theme page itself provides `accentcolor: "#"` on the AMO install page ( in the markup on https://addons.mozilla.org/en-US/firefox/addon/flag-of-co-385587-2/ ) which is treated differently on the client. So the theme installs fine and works fine initially, which is why nobody noticed anything was broken. But then any update breaks everything. The update should be serving the same JSON as the AMO page where you can install the theme.

Comment 62

[Andrew Williamson [:eviljeff]]

That part of the theme update hasn't been changed since it landed in AMO 5 years ago (https://github.com/mozilla/addons-server/blame/master/services/theme_update.py#L152).

(Arguably we should change frontend to match the reality that that textcolor and accentcolor are not defined by the theme developer/artist and fix the JSON there.)  When did Firefox start treating an invalid color value ("#") any differently than null/None?

Comment 63

[:Gijs (out for now; he/him)]

(In reply to Andrew Williamson [:eviljeff] from comment #62)
> That part of the theme update hasn't been changed since it landed in AMO 5
> years ago
> (https://github.com/mozilla/addons-server/blame/master/services/theme_update.
> py#L152).

So looks like we could fix this bug by just replacing

            'accentcolor': '#%s' % accent if accent else None,

with

            'accentcolor': '#%s' % (accent if accent else ""),


How quickly could such a change be rolled out?

> (Arguably we should change frontend to match the reality that that textcolor
> and accentcolor are not defined by the theme developer/artist and fix the
> JSON there.)  When did Firefox start treating an invalid color value ("#")
> any differently than null/None?

In bug 1404688 - it started implicitly requiring the accentcolor for the theme to work. See the attached patch for a Firefox-based fix...

(In reply to Tim Nguyen :ntim from comment #55)
> The patch I just pushed removes the accentcolor requirement from bug 1404688
> if a headerURL is there. 

How is this different from just reverting 1404688 ?

I don't really understand the fix in 1404688 anyway. Can you provide some more textual context about why the change to the "active" variable was made to require accentcolors? Why require anything? What's the point of a non-active theme ?

> I'm not convinced this is the right fix however, and I think it would be
> much better to fix the underlying update/sync issue :)

Per what Jeff is saying, I think the real issue is with desktop now requiring an accentcolor/textcolor when lots of themes don't really have one, even if the as-installed-from-the-webpage version has an invalid CSS string ("#").

If you don't think this fix is right, can you elaborate on what you *do* think is right?

Comment 64

[Tim Nguyen :ntim]

(In reply to :Gijs (he/him) from comment #63)
> (In reply to Andrew Williamson [:eviljeff] from comment #62)
> > That part of the theme update hasn't been changed since it landed in AMO 5
> > years ago
> > (https://github.com/mozilla/addons-server/blame/master/services/theme_update.
> > py#L152).
> 
> So looks like we could fix this bug by just replacing
> 
>             'accentcolor': '#%s' % accent if accent else None,
> 
> with
> 
>             'accentcolor': '#%s' % (accent if accent else ""),
> 
> 
> How quickly could such a change be rolled out?
> 
> > (Arguably we should change frontend to match the reality that that textcolor
> > and accentcolor are not defined by the theme developer/artist and fix the
> > JSON there.)  When did Firefox start treating an invalid color value ("#")
> > any differently than null/None?
> 
> In bug 1404688 - it started implicitly requiring the accentcolor for the
> theme to work. See the attached patch for a Firefox-based fix...
> 
> (In reply to Tim Nguyen :ntim from comment #55)
> > The patch I just pushed removes the accentcolor requirement from bug 1404688
> > if a headerURL is there. 
> 
> How is this different from just reverting 1404688 ?
> 
> I don't really understand the fix in 1404688 anyway. Can you provide some
> more textual context about why the change to the "active" variable was made
> to require accentcolors? Why require anything? What's the point of a
> non-active theme ?

Previously, we required "headerURL" to be set and it was used as a way to detect whether some theme data is active (applies a theme) or not (resets the theme). This headerURL requirement does not make sense at all for WE themes, which is why bug 1404688 dropped it and started using accentcolor as "active" check, and it makes sense because not having an accentcolor or having a broken one results in bugs like bug 1404386. Ideally, the accentcolor requirement should be dropped (see bug 1413144), but it wouldn't be an easy fix (at least not one that can be uplifted to beta).

Reverting bug 1404688 is different, because it would make headerURL mandatory again. The workaround proposed in this bug checks for the headerURL or the accentcolor existance to detect if a theme is applied. It's not a great fix (because the headerURL requirement is arbitrary), but it works.

> If you don't think this fix is right, can you elaborate on what you *do* think is right?

What I would prefer is that the add-ons server reflects the reality within Firefox: an incorrect accentcolor falls back to a white accent color, it would be nice if the add-ons server could just set "white" by default.

Comment 65

[Tim Nguyen :ntim]

So it would be:

            'accentcolor': '#%s' % (accent if accent else "#ffffff"),

Comment 66

[Andrew Williamson [:eviljeff]]

Changing the update service on AMO is trivial, but we're pretty cautious about making changes in a service that's pinged A LOT - we don't want to break anything.  My concern would be old versions of Firefox (or Thunderbird or Seamonkey) that would be getting served a subtly different response to their theme update request, and how we'd be able to verify it wasn't going to break those clients.

To answer the timing question - April 26 is the next scheduled push any changes to the update service would be possible for.

I'm not really clear on why changes in Firefox made to allow static themes to not have to provide a headerURL should affect LWT at all.  They are two different types of add-on - why not just only apply the changes in bug 1404688 to just static themes?

Comment 67

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen :ntim from comment #64)
> Previously, we required "headerURL" to be set and it was used as a way to
> detect whether some theme data is active (applies a theme) or not (resets
> the theme).

I don't understand. If we're resetting the theme, we pass null, don't we? Why not just check for that? :-\

> This headerURL requirement does not make sense at all for WE
> themes, which is why bug 1404688 dropped it and started using accentcolor as
> "active" check, and it makes sense because not having an accentcolor or
> having a broken one results in bugs like bug 1404386. Ideally, the
> accentcolor requirement should be dropped (see bug 1413144), but it wouldn't
> be an easy fix (at least not one that can be uplifted to beta).
> 
> Reverting bug 1404688 is different, because it would make headerURL
> mandatory again. The workaround proposed in this bug checks for the
> headerURL or the accentcolor existance to detect if a theme is applied. It's
> not a great fix (because the headerURL requirement is arbitrary), but it
> works.

Can we get this landed and uplifted to 60, then? Please take ownership of the regression, or if you don't have time to do so, please find someone else who does. We can't ship this in the current state, and continuing not to do anything with this bug isn't really an option.

FWIW,

(In reply to Andrew Williamson [:eviljeff] from comment #66)
> I'm not really clear on why changes in Firefox made to allow static themes
> to not have to provide a headerURL should affect LWT at all.  They are two
> different types of add-on - why not just only apply the changes in bug
> 1404688 to just static themes?

I agree with this, but I think it's too late to rewrite this for 60. :-\



(In reply to Tim Nguyen :ntim from comment #64)
> > If you don't think this fix is right, can you elaborate on what you *do* think is right?
> 
> What I would prefer is that the add-ons server reflects the reality within
> Firefox: an incorrect accentcolor falls back to a white accent color, it
> would be nice if the add-ons server could just set "white" by default.

I thought the fallback was black, but either way, encoding what gecko does in some python server code for AMO seems like a recipe for disaster. Making the install and update pages do the same thing with the same input seems sane, though. I'll file an issue on github, I guess.

Comment 68

[:Gijs (out for now; he/him)]

I wrote a naive PR for the addons-server ( https://github.com/mozilla/addons-server/pull/8067 ), but I would prefer we fix this in Firefox (too) as that's where we're regressing something.

Comment 69

[Tim Nguyen :ntim]

(In reply to :Gijs (he/him) from comment #67)
> (In reply to Tim Nguyen :ntim from comment #64)
> > Previously, we required "headerURL" to be set and it was used as a way to
> > detect whether some theme data is active (applies a theme) or not (resets
> > the theme).
> 
> I don't understand. If we're resetting the theme, we pass null, don't we?
> Why not just check for that? :-\

The state before bug 1404688 required headerURL to be in the theme data, otherwise it would reset to the default theme (regardless of what else is in the theme data).

> > This headerURL requirement does not make sense at all for WE
> > themes, which is why bug 1404688 dropped it and started using accentcolor as
> > "active" check, and it makes sense because not having an accentcolor or
> > having a broken one results in bugs like bug 1404386. Ideally, the
> > accentcolor requirement should be dropped (see bug 1413144), but it wouldn't
> > be an easy fix (at least not one that can be uplifted to beta).
> > 
> > Reverting bug 1404688 is different, because it would make headerURL
> > mandatory again. The workaround proposed in this bug checks for the
> > headerURL or the accentcolor existance to detect if a theme is applied. It's
> > not a great fix (because the headerURL requirement is arbitrary), but it
> > works.
> 
> Can we get this landed and uplifted to 60, then? Please take ownership of
> the regression, or if you don't have time to do so, please find someone else
> who does. We can't ship this in the current state, and continuing not to do
> anything with this bug isn't really an option.

I don't understand the necessity of shipping a fix in Firefox, when https://github.com/mozilla/addons-server/pull/8067 should fix this problem fine? I'll let you decide however.

> (In reply to Andrew Williamson [:eviljeff] from comment #66)
> > I'm not really clear on why changes in Firefox made to allow static themes
> > to not have to provide a headerURL should affect LWT at all.  They are two
> > different types of add-on - why not just only apply the changes in bug
> > 1404688 to just static themes?

Lightweight themes and static themes use the same back-end.

Comment 70

[Tim Nguyen :ntim]

>  we pass null, don't we?

And no, we don't just pass null, we send an theme object with just empty strings.

Comment 71

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen :ntim from comment #70)
> >  we pass null, don't we?
> 
> And no, we don't just pass null, we send an theme object with just empty
> strings.

We pass null to the observers:

https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/LightweightThemeManager.jsm#318

and pass null to `currentTheme`, e.g.:

https://dxr.mozilla.org/mozilla-central/rev/6276ec7ebbf33e3484997b189f20fc1511534187/browser/components/customizableui/CustomizableUI.jsm#2646

Where do we pass a theme object with just empty strings? That sounds like a bug to me...

Comment 72

[:Gijs (out for now; he/him)]

Comment on attachment 8968127 [details]
Bug 1450975 - Load themes without an accentcolor.

https://reviewboard.mozilla.org/r/236808/#review242972

::: toolkit/modules/LightweightThemeConsumer.jsm:136
(Diff revision 1)
>        root.setAttribute("lwtheme-image", "true");
>      } else {
>        root.removeAttribute("lwtheme-image");
>      }
>  
> -    let active = !!aData.accentcolor;
> +    let active = aData.accentcolor || aData.headerURL;

So in particular, should we instead just save `aData` to a temp and check if it was null here (before assigning all the default `""` values to it)?


I'm going to let Jared review this because I'm not convinced I understand what is going on in the original bug still, so I'd like a second opinion.

Comment 73

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen :ntim from comment #69)
> I don't understand the necessity of shipping a fix in Firefox, when
> https://github.com/mozilla/addons-server/pull/8067 should fix this problem
> fine? I'll let you decide however.

1) Firefox broke something, the add-ons server didn't change, so it's reasonable that we pursue an in-product fix if possible.
2) The add-ons update bit is one bit that exposes the brokenness. I wouldn't be surprised if there are other consumers that don't pass a truthy-but-useless string for their accentcolor when they don't have one, and I don't think we should wait for release to find out.
3) I'm also not sure if the add-ons server folks want to take the github change, given that it's touching an old bit of code - who knows what else that change would break, maybe in other versions of Firefox?
4) Belt and suspenders. Maybe AMO changes their install pages next, test with 59 given it's current release, and then later versions break. Or they test with 61 because they're nice mozilla employees using nightly, and accidentally break release. Either way we'd be in trouble.
5) We should have an automated test for this, which should ideally live in m-c integration so it turns the tree orange if we break this again. Which would require a Firefox patch (and fix).

So yeah, I feel pretty strongly we should fix this in both places, but also I'm aware time is short for the 60 release, and so I think we should all do what we can to ensure we don't ship the current state to end-users on release. Even without all the reasons above, I'd rather have that mean we fix it twice than that we don't fix it at all because everyone expects "the other people" to fix it.

Comment 74

[Tim Nguyen :ntim]

(In reply to :Gijs (he/him) from comment #71)
> (In reply to Tim Nguyen :ntim from comment #70)
> > >  we pass null, don't we?
> > 
> > And no, we don't just pass null, we send an theme object with just empty
> > strings.
> 
> We pass null to the observers:
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> LightweightThemeManager.jsm#318
> 
> and pass null to `currentTheme`, e.g.:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 6276ec7ebbf33e3484997b189f20fc1511534187/browser/components/customizableui/
> CustomizableUI.jsm#2646
> 
> Where do we pass a theme object with just empty strings? That sounds like a
> bug to me...

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/parent/ext-theme.js#295-323

Comment 75

[Tim Nguyen :ntim]

> So in particular, should we instead just save `aData` to a temp and check if it was null here (before assigning all the default `""` values to it)?

That doesn't work in the case where we only reset one window to the default theme. `aData` would contain the windowId.

Comment 76

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen :ntim from comment #75)
> > So in particular, should we instead just save `aData` to a temp and check if it was null here (before assigning all the default `""` values to it)?
> 
> That doesn't work in the case where we only reset one window to the default
> theme. `aData` would contain the windowId.

Sounds to me like the ext-theme stuff needs to have a clearer way to communicate "reset this window" to the window where that needs to happen, that doesn't involve all this brittle piggy-backing on top of lwt stuff. Also, if there's a window reference, why broadcast via the observer service instead of just talking to just that window's lwtconsumer directly?

Comment 77

[:Gijs (out for now; he/him)]

Just so we're clear here, consensus on the github ticket is not to change the add-ons server to return non-null here. Hopefully we can move to making it use 'null' everywhere (so that it's consistent and we don't end up with hard-to-trace bugs like this) but either way, this means the fix here is now even more urgent if we don't want to ship busted lwtheme support on 60.

Comment 78

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8968127 [details]
Bug 1450975 - Load themes without an accentcolor.

https://reviewboard.mozilla.org/r/236808/#review243750

Please file follow up bugs for:
1. not relying on the observer service when we know the windowId
2. having a more explicit way to send a 'reset' instead of an empty theme object

Comment 79

[Marion Daly [:mdaly]]

Tim: Update?

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d83f412c924c
Load themes without an accentcolor. r=jaws

Comment 81

[:Gijs (out for now; he/him)]

Comment on attachment 8968127 [details]
Bug 1450975 - Load themes without an accentcolor.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1404688
[User impact if declined]: some lwthemes break seemingly randomly after being installed a while (when auto-update kicks in)
[Is this code covered by automated tests?]: no, because we don't have tests that use production auto-update servers or (it seems) lwthemes without a text/accentcolor.
[Has the fix been verified in Nightly?]: not yet...
[Needs manual test from QE? If yes, steps to reproduce]:
1. install the 'flag of colombia' theme off AMO ( https://addons.mozilla.org/en-US/firefox/addon/flag-of-co-385587-2/ )
2. turn on browser/chrome debugging in the devtools settings
3. open the browser console
3. run:

Cc["@mozilla.org/addons/integration;1"].getService(Ci.nsITimerCallback).notify(null)

4. Restart
5. verify that the theme is still correctly applied (pre-patch, you see something resembling the default theme but with the selected tab foreground colour being wrong on some OSes)

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: a bit
[Why is the change risky/not risky?]: partially reinstating a check for lwthemes that was there before, 1 line change, several people have looked at this at this point. This would be very low risk, but obviously it's late in the cycle, hence "a bit". Given the fallout of leaving this unfixed (for all of esr, too) I don't think we have much of a choice though, and this is definitely the smallest possible patch.
[String changes made/needed]: no

Leaving ni for Tim to file the follow-up bugs Jared and I asked about. Also, are any themes (webextension or lwt) *meant* to be ending up in the pre-patch state here? As noted elsewhere, it's pretty broken... can we add more defense-in-depth fixes to avoid this happening in a follow-up bug?

Comment 82

[:Gijs (out for now; he/him)]

(In reply to :Gijs (he/him) from comment #81)
> Leaving ni for Tim to file the follow-up bugs Jared and I asked about. Also,
> are any themes (webextension or lwt) *meant* to be ending up in the
> pre-patch state here? As noted elsewhere, it's pretty broken... can we add
> more defense-in-depth fixes to avoid this happening in a follow-up bug?

Comment 83

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

Comment on attachment 8968127 [details]
Bug 1450975 - Load themes without an accentcolor.

Fix for lightweight theme regression, let's take this for beta 16.

Comment 84

[Dorel Luca [:dluca]]

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

Comment 85

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/91cc1385616d

Comment 86

[Dão Gottwald [:dao]]

(In reply to (slow to respond 4/23 to 4/27) Jared Wein [:jaws] (please needinfo? me) from comment #78)
> 2. having a more explicit way to send a 'reset' instead of an empty theme
> object

The theme object being null is explicit enough. The problem is that ext-theme.js bypasses LightweightThemeManager.jsm and sends its custom lightweight-theme-styling-update notification, so now we have multiple places sending data with different structure to LightweightThemeConsumer.jsm.

Comment 87

[Alexandru Simonca [:asimonca], Release Desktop QA]

Hi,
I've reproduced this issue using the STR in Comment #51 on Firefox Beta 60.0b3. I can confirm this issue is fixed on Beta 60.0b16 and Nightly 61.0a1 (id: 20180427102245).

Comment 88

[Christopher Shea]

Hi, I'm still having the problem and have been since at least this bug was first reported.

After most every update I have to remove and reinstall the theme I use: https://addons.mozilla.org/en-US/firefox/addon/dream-of-waves/

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180509100510

Should this be reopened? Or is it possible that the theme I use exposes a different problem than the one in the original report?

Comment 89

[:Gijs (out for now; he/him)]

(In reply to Christopher Shea from comment #88)
> Hi, I'm still having the problem and have been since at least this bug was
> first reported.
> 
> After most every update I have to remove and reinstall the theme I use:
> https://addons.mozilla.org/en-US/firefox/addon/dream-of-waves/
> 
> User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:62.0)
> Gecko/20100101 Firefox/62.0
> Build ID: 20180509100510
> 
> Should this be reopened? Or is it possible that the theme I use exposes a
> different problem than the one in the original report?

Please file a separate report.

Comment 90

[Gingerbread Man]

(In reply to :Gijs (he/him) from comment #89)
> Please file a separate report.

Bug 1460287

Also, I don't see how this belongs in Firefox :: Theme. That's for the default theme.

Comment 91

[Artem S. Tashkinov]

!!!

As soon as I upgraded from Firefox 60 beta 16 to 61 beta 3 I could no longer apply the "Flag of Colombia" theme. It started working again after reinstallation. 

And now after I've just exited the browser and started it again I can no longer apply this theme even though it's selected.

I request this bug to be reopened because Firefox 61 beta 3 is affected.

Comment 92

[Artem S. Tashkinov]

Anyways, I've filed bug 1460522.

Comment 93

[Tim Nguyen :ntim]

Filed bug 1464717 and bug 1464716