Closed Bug 1420713 Opened 2 years ago Closed 2 years ago

No live preview of userChrome.css changes with layout.css.servo.chrome.enabled = true

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: gingerbread_man, Assigned: emilio)

References

Details

(Keywords: reproducible)

Attachments

(1 file)

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
20171126100425

1. Create a brand new profile. In that profile folder, create a folder named chrome. In the chrome folder, make a plain text file called userChrome.css
2. Start Firefox in the new profile. In the location bar, enter about:config?filter=layout.css.servo.chrome.enabled Press the big button to bypass the warning. In the search results, double-click layout.css.servo.chrome.enabled to set it to true.
3. Exit Firefox, then restart in the earlier profile.
4. Press F12 to bring up the developer toolbar. Click the cog wheel in the top right and check both "Enable browser chrome and add-on debugging" and "Enable remove debugging".
5. Press Ctrl+Alt+Shift+I or Cmd+Opt+Shift+I to bring up the Browser Toolbox window. Confirm the security prompt.
6. Click Style Editor in the toolbar. On the left, click userChrome.css in the list. On the right, make some changes that would be obvious to spot, e.g.
* { font-weight: bold !important; }

Actual results
Nothing changes.

Expected results
Text in the user interface (tabs, location bar) becomes bold.

Notes
Live preview works just fine with userContent.css and layout.css.servo.enabled = true (the default setting).
Reduced STR:
1. Follow until step 4 in comment #0.
2. Press Ctrl+J to open Browser Console.
3. Execute following scripts:
var DOMUtils = Components.classes["@mozilla.org/inspector/dom-utils;1"].getService(Components.interfaces.inIDOMUtils);
var styleSheets = DOMUtils.getAllStyleSheets(document);
var userChrome = styleSheets.find(s => s.href.endsWith("userChrome.css"));
DOMUtils.parseStyleSheet(userChrome, "* { font-weight: bold !important; }");

userChrome.cssRules[0].cssText is updated, but no visible changes will happen.
Hmm, those are added as user sheets, so there's no reason off-hand that shouldn't be working... I'll take a look.
Ok, I have a fix but, for what is worth, this only used to work in Gecko by chance, kind of...
Assignee: nobody → emilio
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> Ok, I have a fix but, for what is worth, this only used to work in Gecko by
> chance, kind of...

Hmm, this is more bogus than what I expected (not stylo only fwiw). I'll do the right fix...
Depends on: 1420755
Actually I changed my mind... The right fix will take a while, so I'll just do the easy thing for now, and work on the right fix later.
See Also: → 1420762
Comment on attachment 8931969 [details] [diff] [review]
Ensure ReparseStyleSheet refreshes the stylist even when there's no document.

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

::: layout/style/ServoStyleSheet.cpp
@@ +344,5 @@
>    }
>  
> +  // FIXME(emilio): This is kind-of a hack for bug 1420713. As you may notice,
> +  // there's nothing that triggers a style flush or anything similar (neither
> +  // here or in the relevant Gecko path inside DidDirty).

Does this mean we need something similar in CSSStyleSheet::ReparseSheet?

@@ +347,5 @@
> +  // there's nothing that triggers a style flush or anything similar (neither
> +  // here or in the relevant Gecko path inside DidDirty).
> +  //
> +  // The tl;dr is: if we want to make sure scripted changes to sheets not
> +  // attached to any document get properly reflected, we need to rejigger a fair

Maybe s/attached to/associated with/?  Because these sheets are inserted into documents' style sets, they're just not owned by the document.

(And just to be clear, this is just noting a separate issue, since this patch only deals with inspector-reparsed sheets, and not scripted changes, right?)
Attachment #8931969 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #7)
> Comment on attachment 8931969 [details] [diff] [review]
> Ensure ReparseStyleSheet refreshes the stylist even when there's no document.
> 
> Review of attachment 8931969 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/ServoStyleSheet.cpp
> @@ +344,5 @@
> >    }
> >  
> > +  // FIXME(emilio): This is kind-of a hack for bug 1420713. As you may notice,
> > +  // there's nothing that triggers a style flush or anything similar (neither
> > +  // here or in the relevant Gecko path inside DidDirty).
> 
> Does this mean we need something similar in CSSStyleSheet::ReparseSheet?

No, CSSStyleSheet::DidDirty happens to do this, but just by chance, in the sense that the objective of that "ClearRuleCascades()" is just avoiding UAF, not updating the styling. But it doesn't cause a style flush, so I'd expect stuff like getComputedStyle getting the wrong style right after a reparse. You could argue it's not a big deal.

> @@ +347,5 @@
> > +  // there's nothing that triggers a style flush or anything similar (neither
> > +  // here or in the relevant Gecko path inside DidDirty).
> > +  //
> > +  // The tl;dr is: if we want to make sure scripted changes to sheets not
> > +  // attached to any document get properly reflected, we need to rejigger a fair
> 
> Maybe s/attached to/associated with/?  Because these sheets are inserted
> into documents' style sets, they're just not owned by the document.
> 
> (And just to be clear, this is just noting a separate issue, since this
> patch only deals with inspector-reparsed sheets, and not scripted changes,
> right?)

Right. CSSOM in sheets not associated with a document is pretty broken.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb979e1d8a49
Ensure ReparseStyleSheet refreshes the stylist even when there's no document. r=heycam
https://hg.mozilla.org/mozilla-central/rev/fb979e1d8a49
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
20171127220446
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.