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

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Gingerbread Man]

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).

Comment 1

[Masatoshi Kimura [:emk]]

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.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Hmm, those are added as user sheets, so there's no reason off-hand that shouldn't be working... I'll take a look.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Ok, I have a fix but, for what is worth, this only used to work in Gecko by chance, kind of...

Comment 4

[Emilio Cobos Álvarez (: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...

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Ensure ReparseStyleSheet refreshes the stylist even when there's no document.

Comment 7

[Cameron McCormack (:heycam)]

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?)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb979e1d8a49
Ensure ReparseStyleSheet refreshes the stylist even when there's no document. r=heycam

Comment 10

[Eliza Balazs [:ebalazs_]]

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

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 12

[Gingerbread Man]

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