1394233 - stylo: [non-e10s] Certain rule in userContent.css is not applied

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P5)

Description

[Tooru Fujisawa [:arai]]

Attachment: rendering result comparison between e10s and non-e10s

Steps to reproduce:
  1. run Nightly 57.0a1 (2017-08-27) (64-bit) with clean profile on macOS
  2. disable e10s
  3. restart Nightly
  4. create chrome directory in profile
  5. create userContent.css inside chrome directory, with the following content

.ProfileTweet-actionList {
    display: none !important;
}

  6. restart Nightly
  7. open https://twitter.com/FirefoxNightly
  8. reload the page (might not be necessary)

Actual result:
  the style is not applied and buttons under tweet (reply, retweet, favorite...) are shown

Expected result:
  the style is applied and those buttons are hidden.
  (see attached screenshot for the difference)


Some experiments and results:
  1. the following rule works, even when it's written after the above rule

body {
  border: 1px solid red !important;
}

  2. happens only on non-e10s.  both rules work on e10s

  3. regression range: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c4eec1e81c108f8c97a4b228a7f27f26f925ffeb&tochange=98a6294158926166c18e57aec1f1adcff4f5e8a2

Comment 1

[Tooru Fujisawa [:arai]]

> Some experiments and results:
  4. happens only with Stylo enabled.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

I have no idea how bug 1355721 can cause this :/

And btw, on stylo, the inspector doesn't seem to display any rule on the element... which is probably a different bug, though.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

It's weird. It happens when the class contains any uppercase letter. That says, if you have a rule `.x { display: none }` in user sheet, and `<div class="x">`, it would work correctly. However, if you have a rule `.X { display: none }`, it would not be applied on `<div class="X">`.

Comment 4

[Chris Peterson [:cpeterson]]

P5 because this bug only affects non-e10s and userContent.css, not web content.

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

This is very strange indeed. I changed the uppercase-lowercase handling but just inside @font-feature-values rule. I don't think that can cause a bug like that. I'll investigate further.

Comment 6

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f939a926b0f800d5967d70985dc0357496023146

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc0acc00d769
Reftest to ensure that uppercase class names work. r=test-only

Comment 8

[Masatoshi Kimura [:emk]]

Can anyone still reproduce the issue? I couldn't.

Comment 9

[Tooru Fujisawa [:arai]]

yes, happens on 57.0a1 (2017-09-15) (64-bit) on macOS, with the steps in comment #0.

Comment 10

[Masatoshi Kimura [:emk]]

I could not reproduce it on Windows 10, with the same STR.
macOS try run could not reproduce it, either (but failing on Linux x64 QuantumRender debug):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e0c0c0bdc29109178ba58279369e76f7bf7ea73

Can you reproducible it with a fresh profile?

Comment 11

[Tooru Fujisawa [:arai]]

the steps in comment #0 is based on fresh profile.
(of course it changes to non-e10s in step 2)

also, the issue happens with the html file and CSS rule in https://hg.mozilla.org/integration/mozilla-inbound/rev/cc0acc00d769
the paragraph is have a red background on non-e10s+stylo configuration.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

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

Backed out for frequently failing own test usercss-uppercase.html on Windows 7 debug:

https://hg.mozilla.org/mozilla-central/rev/80494b13f4928660229d6c39d409cc4094a6b35a

See bug 1400610 for the failures.

Comment 15

[Masatoshi Kimura [:emk]]

Marking non-e10s && stylo as random:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=66bf8b0e545c7f7ad1650dbee5cc7b041a4c8ecc

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d8edcea43b
Reftest to ensure that uppercase class names work. r=test-only

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1394233: Quirks mode changes can happen after a flush.

The reason why bug 1355721 regressed this is because in non-e10s we definitely
flush before parsing the standards quirks-mode. And bug 1355721 introduced an
unconditional UpdateStylistIfNeeded, unless the counter style / font
equivalents.

That means that the stylist wouldn't remain on its initial state after the first
flush, which itself means that when the compat mode changed, the UA and user
rules were already on the stylist with the quirks mode keys. That makes
class-names be keyed in ascii lowercase.

After that no user style changed, so no rebuild happens for the cascade data in
the user origin, so we keep looking at the wrong keys indefinitely.

Review commit: https://reviewboard.mozilla.org/r/219084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219084/

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8949780 [details]
Bug 1394233: Quirks mode changes can happen after a flush.

https://reviewboard.mozilla.org/r/219084/#review224992

So it's about selector_map which uses ascii lowercase atoms as key, okay.

Hope this wouldn't slow down things... It feels like this is going to put penalty on documents using standard mode in that case, because html document is default to quirks mode :/

::: servo/ports/geckolib/glue.rs:2449
(Diff revision 1)
> -    let quirks_mode = unsafe {
> -        (*data.stylist.device().pres_context().mDocument.raw::<nsIDocument>())
> -            .mCompatMode
> +    let doc =
> +        &*data.stylist.device().pres_context().mDocument.raw::<nsIDocument>();
> +    data.stylist.set_quirks_mode(QuirksMode::from(quirks_mode));

How is this code compiled at all? I don't see where the `quirks_mode` variable is from.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949780 [details]
Bug 1394233: Quirks mode changes can happen after a flush.

https://reviewboard.mozilla.org/r/219084/#review224992

Yeah, that's right, but it only affects user sheets I think. UA sheets are cached, and document sheets are still not there when this happens.

Anyway I do want to profile which kind of style flushes really happen during initialization and try to avoid them.

> How is this code compiled at all? I don't see where the `quirks_mode` variable is from.

Whoops, pushed it too fast, then forgot to push the fixup.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80dbdb06f915
Quirks mode changes can happen after a flush. r=xidorn

Comment 22

[Intermittent Failures Robot]

2 failures in 702 pushes (0.003 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* autoland: 2

Platform breakdown:
* windows7-32: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1394233&startday=2018-02-05&endday=2018-02-11&tree=all

Comment 23

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/80dbdb06f915

Comment 24

[Ryan VanderMeulen [:RyanVM]]

I think this can ride the trains.