1543815 - Port bug 1540387: Replace browser-compacttheme.js with theme experiments mechanism

Status: RESOLVED FIXED

(Thunderbird :: Theme, task)

Description

[Richard Marti (:Paenglab)]

M-C uses now the theme_experiment mechanism instead of a JS file to inject compacttheme.css. We can try to follow.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1543815-dark-light-experiment.patch

Port of https://hg.mozilla.org/mozilla-central/rev/421820a5e759

I added a fix for Calendar where some labels in the header aren't coloured correctly.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 9057669 [details] [diff] [review]
1543815-dark-light-experiment.patch

Hmm, amazing, seems like a pretty straight-forward port. So with all that code removed, it still works ;-)

Comment 3

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #2)

Comment on attachment 9057669 [details] [diff] [review]
1543815-dark-light-experiment.patch

Hmm, amazing, seems like a pretty straight-forward port. So with all that
code removed, it still works ;-)

Not all works. :-(

Geoff, it seems the theme_experiments are only applied on the main window but not to the other areas like the Quick Filter bar, the message headers or the composer window. Please can you look what happens here? This could also be fixed, if possible, in a new bug.

Comment 4

[Jorg K (CEST = GMT+2)]

Hmm, I checked the AB window and that worked, but not the compose window. There is no need to land this quickly since everything is still working the old way, no?

Comment 5

[Richard Marti (:Paenglab)]

The AB window has the needed styles in addressbook.css. The other things are for the dark theme only. I could try to move the styles to other CSS files but then it would apply to other themes too.

No rush, it works still the old way.

Comment 6

[Geoff Lankow (:darktrojan)]

If I'm understanding the question right, all those places have background colours of their own and just hide what's beneath them. Also, this is a thing:

Security Error: Content at moz-extension://bd9912b5-5604-482b-80d2-6217a9980178/experiment.css may not load or link to chrome://messenger/skin/compacttheme.css.

Comment 7

[Richard Marti (:Paenglab)]

(In reply to Geoff Lankow (:darktrojan) from comment #6)

If I'm understanding the question right, all those places have background colours of their own and just hide what's beneath them. Also, this is a thing:

Security Error: Content at moz-extension://bd9912b5-5604-482b-80d2-6217a9980178/experiment.css may not load or link to chrome://messenger/skin/compacttheme.css.

And how could that be fixed?

Comment 8

[Jorg K (CEST = GMT+2)]

Typically those security errors are a pain to fix. How does it work for FF? It has the same import:
https://hg.mozilla.org/mozilla-central/rev/421820a5e759#l9.3

Does the security error only show on the compose window?

Comment 9

[Tim Nguyen :ntim]

(In reply to Richard Marti (:Paenglab) from comment #3)

Geoff, it seems the theme_experiments are only applied on the main window but not to the other areas like the Quick Filter bar, the message headers or the composer window. Please can you look what happens here? This could also be fixed, if possible, in a new bug.

theme_experiment will only apply the stylesheet to windows/dialogs that initialize a LightweightThemeConsumer. Does that help ?

Comment 10

[Geoff Lankow (:darktrojan)]

(In reply to Jorg K (GMT+2) from comment #8)

Does the security error only show on the compose window?

No, it happens for every window the theme is loaded on.

Comment 11

[Jorg K (CEST = GMT+2)]

In my local build with M-C at
changeset: 469330:846d7680d2de
tag: tip
user: Bogdan Tara <[e-mail]>
date: Sat Apr 13 05:14:11 2019 +0300 (time seems wrong, pushed at Sat Apr 13 09:52:52 2019 +0000)
default, light and dark themes are gone altogether, I only have a few (ugly) WE test themes left in my test profile. Richard, do you see the same?

I came here to check the security errors, but no theme and no error :-(

Comment 12

[Richard Marti (:Paenglab)]

I see all themes and the error too.

Comment 13

[Jorg K (CEST = GMT+2)]

Hmm, well, looks like me normal testing profile is screwed up. On a new profile, default, light and dark themes are there. They also seem to work with the patch for main window, address book window and message stand-alone window. The only thing that doesn't quite work it the compose window. (Edit) NO security error anywhere to be seen. On the compose window the menu and toolbar are also dark, the addressing widget isn't dark. So it must have loaded some CSS, no?

Comment 14

[Richard Marti (:Paenglab)]

Yes, some toolbars. It seems the main toolbox of every window has the CSS applied but all other like the QFB, the msgHeaderView of the main window or the headers-box of the composer.

Comment 15

[Jorg K (CEST = GMT+2)]

Depending on the profile I use, I've seen three variations: 1) Profiles where default/dark/light schemes are missing. 2) Profiles where the main Windows incl. QFB and header pane are styles correctly, only the compose window isn't, no security error 3) On an new profile, the QFB and header pane are light when they should be dark, and there is a security error.

On a new profile you run into bug 1544204 since "Get Add-ons" is the preselected pane, so as soon as you switch to the add-ons manager for the first time, you crash :-( - You need to visit the profile with a version of TB which doesn't have bug 1544204.

Comment 16

[Jorg K (CEST = GMT+2)]

Looking at the patch and the corresponding M-C changeset, I see that this wasn't ported:
https://hg.mozilla.org/mozilla-central/rev/421820a5e759#l7.3
Something is added to a whitelist there, so maybe that avoids the security checks. Apart from that, the patch is a 1:1 port that should work in TB as well.

I'll look into it.

EDIT: Well, the hunk is in /browser/base/content/test/static/browser_all_files_referenced.js, so maybe not so relevant :-(

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1543815-dark-light-experiment.patch

Rebased.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: switch-off-sec-test.patch

OK, I noticed this in the debug console.

[7372, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file c:/mozilla-source/comm-central/dom/security/nsContentSecurityManager.cpp, line 975
[7372, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file c:/mozilla-source/comm-central/dom/security/nsContentSecurityManager.cpp, line 843
[7372, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file c:/mozilla-source/comm-central/layout/style/Loader.cpp, line 2132

0x805303F4 is NS_ERROR_DOM_BAD_URI. Switching off one security check in this patch makes it all work. Now we'll have to see why this test doesn't like our style sheet.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: switch-off-sec-test.patch (v2)

OK, I grabbed the debug patch from from bug 1246500 and adapted it. This is what we get:

=== DoCheckLoadURIChecks
=== aURI: chrome://messenger/skin/compacttheme.css
=== flags: 2
=== rv: 805303f4
loadingPrincipal: SystemPrincipal
triggeringPrincipal: moz-extension://d7748adb-7ee6-4668-8bc2-b510103f3e6f/experiment.css
contentPolicyType: 39
securityMode: 4
initalSecurityChecksDone: no

That matches the console error:
Security Error: Content at moz-extension://bd9912b5-5604-482b-80d2-6217a9980178/experiment.css may not load or link to chrome://messenger/skin/compacttheme.css (different extension since I was using the dark theme).

I don't know how to interpret those numbers, but since Christoph suggested dumping them out back in bug 1246500, they must be relevant.

Note the precedents of CSS file loading problems in TB in bug 1245681 and bug 1246500 (bug 1269254).

Boris, sorry to trouble you again, but this needs a DOM security person to take a look.

Comment 20

[Jorg K (CEST = GMT+2)]

Hold on, the error is 0x805303F4 / NS_ERROR_DOM_BAD_URI. Which URI is bad here?

Comment 21

[Jorg K (CEST = GMT+2)]

I built Firefox and switched to the dark theme: This is the debug I see:

=== DoCheckLoadURIChecks
=== aURI: chrome://browser/skin/compacttheme.css
=== flags: 2
=== rv: 0
loadingPrincipal: SystemPrincipal
triggeringPrincipal: moz-extension://472d17a1-2274-457a-b51b-bd9252083349/experiment.css
contentPolicyType: 39
securityMode: 4
initalSecurityChecksDone: no

Same debug output, only this time the check passed. Looks like I need to debug further into nsContentUtils::GetSecurityManager()->CheckLoadURIWithPrincipal().

Comment 22

[Boris Zbarsky [:bzbarsky]]

Boris, sorry to trouble you again, but this needs a DOM security person to take a look.

Then you should probably needinfo one, e.g. Christoph.

Which URI is bad here?

The one being loaded, "bad" in the sense of "can't load that". That's just what CheckLoadURI returns when the load is blocked. In this case, the URI being loaded is "chrome://browser/skin/compacttheme.css".

And yes, it's worth figuring out how the logic flows in CheckLoadURIWithPrincipal() diverge in the success/failure cases.

Comment 23

[Jorg K (CEST = GMT+2)]

OK, I finally managed to step through it.

Adding debug to nsChromeRegistry::AllowContentToAccess() gives:
=== AllowContentToAccess: package: messenger, URI: chrome://messenger/skin/compacttheme.css, allow=false

Looks like FF has
https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/browser/base/jar.mn#5
% content browser %content/browser/ contentaccessible=yes

But we don't have that here:
https://searchfox.org/comm-central/rev/d0308dc89c251762ff038a7181ab90b7f119e8cc/mail/base/jar.mn#5

Equally, that would have to be added to the compose window. I'll check it out.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1543815-contentaccessible.patch

OK, this does the trick. Now, no one likes contentaccessible=yes, but since FF does it (see comment #23), we might want to do it, too.

This one line fixes the QFB issue and the compose window. I also had
% content messengercompose %content/messenger/messengercompose/ contentaccessible=yes in mail/components/compose/jar.mn but that doesn't even seem to be necessary.

So let me know what you think.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9058423 [details] [diff] [review]
1543815-contentaccessible.patch

Seems to me like ideally the contentaccessible=yes would be only on the thing that really needs to expose.  Whether that's possible in this case (e.g. by moving the thing around to a different packat), I don't know.

Comment 26

[Jorg K (CEST = GMT+2)]

Thanks for the comment/feedback, Boris. Yes, maybe we can reshuffle it some other way.

Comment 27

[Magnus Melin [:mkmelin]]

I'm really surprised firefox has the
% content browser %content/browser/ contentaccessible=yes

Maybe there's some other mechanism preventing reference of img and script internal resources then?

Comment 28

[Jorg K (CEST = GMT+2)]

For Firefox this was added between 1.8 and 1.9.1 here:
https://dxr.mozilla.org/mozilla1.8/source/browser/base/jar.mn (not there)
https://dxr.mozilla.org/mozilla1.9.1/source/browser/base/jar.mn#2 (has been added)
https://hg.mozilla.org/releases/mozilla-1.9.1/rev/6fb1f4eefa4d#l1.5
in bug 292789. Author Daniel Veditz, reviewer Boris.

As Boris said, can we reshuffle it better?

Comment 29

[Richard Marti (:Paenglab)]

Comment on attachment 9058423 [details] [diff] [review]
1543815-contentaccessible.patch

With this all works as before.

Comment 30

[Jorg K (CEST = GMT+2)]

Any idea how to reshuffle this differently/better?

Comment 31

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #30)

Any idea how to reshuffle this differently/better?

I have no idea. For me it's a bit strange as all this parts are in chrome and not content. When Fx does this we could follow.

Comment 32

[Richard Marti (:Paenglab)]

Attachment: 1543815-dark-light-experiment.patch

Updated after landing of bug 1547054.

Magnus, how are we going further with this bug?

Comment 33

[Jorg K (CEST = GMT+2)]

Boris, could you please take a look at comment #27 and comment #28. Why is it OK for FF to have % content browser %content/browser/ contentaccessible=yes? Is there an issue doing the same in TB? Looks like this bug is stuck on this decision.

Comment 34

[Boris Zbarsky [:bzbarsky]]

I have no idea why that's there; I don't think it should be OK.

Based on blame, chances are it was added to keep things working back when contentaccessible was first introduced, and no one has done the work of dealing with it since then. But dveditz may know for sure.

Comment 35

[Daniel Veditz [:dveditz]]

The most immediate reason for adding the checks in bug 292789 was to prevent fingerprinting add-ons (which were also chrome://). There's no direct security problem with loading images or script from the application. Those are essentially static files that a malicious page or extension could simply reproduce. They are executed in a non-privileged context with or without the chrome: scheme. It was not necessary to revamp the browser code to fix the add-on enumeration problem in bug 292789, and various random things in the browser and toolkit directories were accessed from non-chrome contexts.

Although static in any given release those resources do change from time to time fine-grained fingerprinting of browser version is possible by allowing access to those files. They also become a kind of "undocumented API" and we no longer know what 3rd parties might be relying on the constancy of those resources, adding sand in the gears when we want to change them.

There are good engineering reasons to want to keep the internal things private, and to refactor to reduce the amount we expose to the minimum (boy is there a lot more in there than there was in Firefox 2!) but it's not a security problem per se to make it accessible.

Comment 36

[Magnus Melin [:mkmelin]]

Comment on attachment 9058423 [details] [diff] [review]
1543815-contentaccessible.patch

Review of attachment 9058423 [details] [diff] [review]:
-----------------------------------------------------------------

I suppose we will go with this too given Firefox does it.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cdebe30cb934
Port bug 1540387: Replace browser-compacttheme.js with theme experiments. r=jorgk
https://hg.mozilla.org/comm-central/rev/93c409ccf8a2
Add contentaccessible=yes to mail/base/jar.mn's messenger.jar. r=mkmelin DONTBUILD