1855151 - background image theme doesn't seem to apply

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

on latest build, the Alpenglow theme does not apply properly (nor do https://addons.mozilla.org/en-US/firefox/addon/save-the-bees-plz/ for example)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Somehow a regression from bug 1854486... The computed style looks correct tho... wat.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Just moving the rules from browser/content to browser/themes causes the images not to load...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1855151 - Move lwtheme rules back to browser/base/content/browser.css. r=dao

When loaded from there, the triggering principal is the system principal
and we go through:

https://searchfox.org/mozilla-central/rev/95ec620fda8ad6e4096cac1f1a9db8653669b31d/dom/security/nsContentSecurityManager.cpp#1650-1657

Otherwise we fail the check because the triggering principal is a
ContentPrincipal with "chrome://browser/skin/browser.css" as the URI.

That seems a bit unexpected and deserves some looking into, but let's
unbreak themes in the meantime.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Freddy, do you know if creating ContentPrincipals with chrome:// URIs is expected for @import-ed stylesheets?

I suspect the right fix is somehow propagating the triggering principal across @import or so.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Actually just loading browser-shared.css from the top level doesn't quite fix it either. Will take a deeper look after lunch.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a55e1498654d
Move lwtheme rules back to browser/base/content/browser.css. r=dao,desktop-theme-reviewers

Comment 7

[Frederik Braun [:freddy]]

Reading the @import spec as I true to get a better understanding.

in https://www.w3.org/TR/css-cascade-3/#conditional-import:

The origin of an imported style sheet is the origin of the style sheet that imported it.

So that's like with JS? If A loads styles from B, then the code in B is being executed with the context (principal) from A?
Then I believe comment 4 is right that we want to inherit the principal across @import.

Comment 8

[Frederik Braun [:freddy]]

Are imports of nsContentPolicyType::TYPE_STYLESHEET? Surprised they did not get their own.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1854486

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Okay, so I dug into what's going on and what makes chrome://browser/skin and chrome://browser/content different. This doesn't have to do with @import and such after all, just with the result of GetChannelResultPrincipal which is what we end up using.

The answer is this piece of code that comes all the way back from bug 221490.

So, thoughts:

1. Wat, that's rather unexpected.
2. Freddy, any reason we couldn't expand that to all chrome:// channels?
3. Alternatively, any reason why we shouldn't change this check to allow subresources from chrome:// principals? So something like:

index 3110a07b3177a..a855781d1170b 100644
--- a/dom/security/nsContentSecurityManager.cpp
+++ b/dom/security/nsContentSecurityManager.cpp
@@ -1647,8 +1647,11 @@ nsresult nsContentSecurityManager::CheckChannel(nsIChannel* aChannel) {
     return NS_OK;
   }
 
-  // Allow subresource loads if TriggeringPrincipal is the SystemPrincipal.
-  if (loadInfo->TriggeringPrincipal()->IsSystemPrincipal() &&
+  // Allow subresource loads from chrome:// URIs or if TriggeringPrincipal is
+  // the SystemPrincipal.
+  nsIPrincipal* triggeringPrincipal = loadInfo->TriggeringPrincipal();
+  if ((triggeringPrincipal->IsSystemPrincipal() ||
+       triggeringPrincipal->SchemeIs("chrome")) &&
       loadInfo->GetExternalContentPolicyType() !=
           ExtContentPolicy::TYPE_DOCUMENT &&
       loadInfo->GetExternalContentPolicyType() !=

Both approaches would work, afaict.

Comment 11

[Narcis Beleuzu [:NarcisB]]

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

Comment 16

[Dianna Smith [:diannaS]]

Comment on attachment 9355036 [details]
Bug 1855151 - Move lwtheme rules back to browser/base/content/browser.css. r=dao

Beta/Release Uplift Approval Request

• User impact if declined:
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:
• Is Android affected?: Yes

Comment 17

[Dianna Smith [:diannaS]]

Comment on attachment 9355036 [details]
Bug 1855151 - Move lwtheme rules back to browser/base/content/browser.css. r=dao

Approved for 119.0b3

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/f3b8a7aebf57

Comment 19

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on a 2023-09-26 Nightly build on macOS 12.
Verified as fixed on Firefox 119.0b3(build ID: 20230929091427) and Nightly 120.0a1(build ID: 20231001214422) on macOS 12, Windows 10, Ubuntu 22.
Both Alpenglow and Save the Bees themes are now applied correctly.

Comment 20

[Frederik Braun [:freddy]]

Discussion to be continued in review of bug 1855225 w/ dveditz.