Closed Bug 1855151 Opened 9 months ago Closed 9 months ago

background image theme doesn't seem to apply

Categories

(Firefox :: Theme, defect, P1)

defect

Tracking

()

VERIFIED FIXED
120 Branch
Tracking Status
firefox-esr115 --- unaffected
firefox118 --- unaffected
firefox119 --- verified
firefox120 --- verified

People

(Reporter: nchevobbe, Assigned: emilio)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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)

Flags: needinfo?(emilio)

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

Assignee: nobody → emilio
Severity: -- → S2
Keywords: regression
Priority: -- → P1
Regressed by: 1854486

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

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.

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.

Flags: needinfo?(emilio) → needinfo?(fbraun)

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

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a55e1498654d
Move lwtheme rules back to browser/base/content/browser.css. r=dao,desktop-theme-reviewers

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.

Flags: needinfo?(fbraun)

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

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

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.

Flags: needinfo?(emilio) → needinfo?(fbraun)
See Also: → 221490
Blocks: 1855225
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 120 Branch
Duplicate of this bug: 1855335
Duplicate of this bug: 1855267
Duplicate of this bug: 1855263
Duplicate of this bug: 1855422

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
Attachment #9355036 - Flags: approval-mozilla-beta?

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

Approved for 119.0b3

Attachment #9355036 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

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

Flags: needinfo?(fbraun)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: