1176703 - Theme overrides don't work in safe mode and cause missing/wrong images

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[Elbart]

Attachment: normal_-_safemode.png

m-c:
 2:40.59 LOG: MainThread Bisector INFO Last good revision: 7d4ab4a9febd (2015-06-07)
 2:40.59 LOG: MainThread Bisector INFO First bad revision: 4700d1cdf489 (2015-06-08)
 2:40.59 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7d4ab4a9febd&tochange=4700d1cdf489

m-i:
 1:47.54 LOG: MainThread Bisector INFO Last good revision: ab9471275a90
 1:47.55 LOG: MainThread Bisector INFO First bad revision: 9ef529a9a02b
 1:47.55 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ab9471275a90&tochange=9ef529a9a02b

fx-team:
 5:34.95 LOG: MainThread Bisector INFO Last good revision: 653299500d3c
 5:34.96 LOG: MainThread Bisector INFO First bad revision: 39821d952adc
 5:34.96 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=653299500d3c&tochange=39821d952adc

39821d952adc	Gijs Kruitbosch — Bug 1150417 - use theme manifest for overrides, r=glandium,dao
264fb3d4d4c6	Gijs Kruitbosch — Bug 1170207 - allow overrides of chrome://../skin/ URIs with other chrome://../skin/ URIs within skin manifests, r=bsmedberg

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
This will cause missing icons in e.g. about:addons

Comment 2

[:Gijs (he/him)]

I don't really understand why the default theme manifest is no longer read in this case, when it is clearly still the theme that's in use... Dave, do you know?

As a minimal fix for 40 we could move the overrides for images that do not otherwise exist back to the regular manifest file, or back the change out if we can't find a way of fixing this.

Comment 3

[Dave Townsend [:mossop]]

(In reply to :Gijs Kruitbosch from comment #2)
> I don't really understand why the default theme manifest is no longer read
> in this case, when it is clearly still the theme that's in use... Dave, do
> you know?

Ugh right, because we don't load extension manifests when in safe mode.

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Adding a tracking flag. We should not be messing up the icons in safe mode.

Comment 5

[:Gijs (he/him)]

Benjamin, where does the code live that adds these manifests and checks for safe mode? I've been looking for this last week but I couldn't find it. Would changing those to make an exception for the default theme be doable, or should we consider reverting the decision to move the overrides from the main manifest into the extension manifest? (if we leave the overrides-allowed-in-themes thing in, at least themes can workaround by using overrides themselves rather than packaging extra files in the right places, I guess...)

Comment 6

[Benjamin Smedberg]

The code which loads theme directories is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#644

I don't know how this interacts with safe mode. I suspect it might be through this check: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7106

Comment 7

[Dave Townsend [:mossop]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> I don't know how this interacts with safe mode. I suspect it might be
> through this check:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProvider.jsm#7106

Nope, right here: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#623

Comment 8

[:Gijs (he/him)]

Thanks to both of you!

Comment 9

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1176703 - load default theme manifest in safe mode, r?bsmedberg

Bug 1176703 - load default theme manifest in safe mode, r?bsmedberg

Comment 10

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/12711/#review11383

::: toolkit/xre/nsXREDirProvider.cpp:657
(Diff revision 1)
> +    }

This makes assumptions about the default theme's ID that I think we'll forget down the road. I think it would work to just only load the ThemeDirs from the ini and make sure that the value of general.skins.selectedSkin is ignored in safe mode (I assume it is already but let's check)

Comment 11

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] (PTO till July 21st) from comment #10)
> https://reviewboard.mozilla.org/r/12711/#review11383
> 
> ::: toolkit/xre/nsXREDirProvider.cpp:657
> (Diff revision 1)
> > +    }
> 
> This makes assumptions about the default theme's ID that I think we'll
> forget down the road.

FWIW, I thought about this, but we make this assumption in a lot of places, including automated tests and our packaging manifests, which is why this seemed acceptable...

> I think it would work to just only load the ThemeDirs
> from the ini and make sure that the value of general.skins.selectedSkin is
> ignored in safe mode (I assume it is already but let's check)

... but this does sound neater. I can look at this later today, I hope. Setting ni so I don't forget...

Comment 12

[Benjamin Smedberg]

https://reviewboard.mozilla.org/r/12711/#review11383

> This makes assumptions about the default theme's ID that I think we'll forget down the road. I think it would work to just only load the ThemeDirs from the ini and make sure that the value of general.skins.selectedSkin is ignored in safe mode (I assume it is already but let's check)

I'm similarly concerned, although I'll accept this if we can't find a more elegant solution. Waiting on your investigations later today before marking anything.

Comment 13

[:Gijs (he/him)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> https://reviewboard.mozilla.org/r/12711/#review11383
> 
> > This makes assumptions about the default theme's ID that I think we'll forget down the road. I think it would work to just only load the ThemeDirs from the ini and make sure that the value of general.skins.selectedSkin is ignored in safe mode (I assume it is already but let's check)
> 
> I'm similarly concerned, although I'll accept this if we can't find a more
> elegant solution. Waiting on your investigations later today before marking
> anything.

So, if I do this:

1) lightweight themes remain applied in safe mode
2) full themes don't remain applied *but* the same problem we're trying to fix here reoccurs, in that the overrides from the default theme aren't being applied, if your selected theme isn't the default theme (yeah... not sure what's happening there).

So it seems like if we want to go this route we need more work in the add-on manager / lwtheme code. I can do that, but it will take more time and I'm worried about running out of time for 40.

Benjamin, thoughts?

(my test patch: https://pastebin.mozilla.org/8839005 )

Comment 14

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> > https://reviewboard.mozilla.org/r/12711/#review11383
> > 
> > > This makes assumptions about the default theme's ID that I think we'll forget down the road. I think it would work to just only load the ThemeDirs from the ini and make sure that the value of general.skins.selectedSkin is ignored in safe mode (I assume it is already but let's check)
> > 
> > I'm similarly concerned, although I'll accept this if we can't find a more
> > elegant solution. Waiting on your investigations later today before marking
> > anything.
> 
> So, if I do this:
> 
> 1) lightweight themes remain applied in safe mode
> 2) full themes don't remain applied *but* the same problem we're trying to
> fix here reoccurs, in that the overrides from the default theme aren't being
> applied, if your selected theme isn't the default theme (yeah... not sure
> what's happening there).
> 
> So it seems like if we want to go this route we need more work in the add-on
> manager / lwtheme code. I can do that, but it will take more time and I'm
> worried about running out of time for 40.
> 
> Benjamin, thoughts?
> 
> (my test patch: https://pastebin.mozilla.org/8839005 )

Comment 15

[Benjamin Smedberg]

Comment on attachment 8630369 [details]
MozReview Request: Bug 1176703 - load default theme manifest in safe mode, r?bsmedberg

This patch is simpler, and simple is better here. I'm hoping to get rid of heavy themes anyway since that is a necessary part of kill-XUL, and then the problem will just go away.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d9402c81ffdb

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5995a3f88211

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d9402c81ffdb
https://hg.mozilla.org/mozilla-central/rev/5995a3f88211

Comment 19

[:Gijs (he/him)]

Comment on attachment 8630369 [details]
MozReview Request: Bug 1176703 - load default theme manifest in safe mode, r?bsmedberg

Approval Request Comment
[Feature/regressing bug #]: bug 1150417 / windows theme work
[User impact if declined]: missing/wrong images in safe mode
[Describe test coverage new/current, TreeHerder]: nope...
[Risks and why]: reasonably low, change specific to how we deal with the default theme's manifest in safe mode
[String/UUID change made/needed]: nope

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8630369 [details]
MozReview Request: Bug 1176703 - load default theme manifest in safe mode, r?bsmedberg

We want the safe mode to be safe! Taking it.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1ef901bf9cd6

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6772e746544a

Comment 23

[Cornel Ionce [:noni]]

Confirming the fix for:
- latest 42.0a1 Nightly, build ID: 20150715095506;
- latest 41.0a2 Aurora, build ID: 20150715095519;
- Firefox 40.0b4, build ID: 20150713153304.