1368782 - CSS cascading order of @import-ed style sheets is broken (regression from bug 1352968)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Sören Hentzschel]

Attachment: screenshot.png

There are a lot of style issues on a lot of websites after landing of bug 1352968 (according to mozregression).

Please the the attached screenshot. It shows a phpbb forum with wrong link colors, missing borders around input fields, borders where no borders should be (around the input fields), …

Demo url: https://www.camp-firefox.de/forum/

There are more examples, for example the backend of Pimcore websites. I'll attach a screenshot in a few minutes.

Comment 1

[Sören Hentzschel]

Attachment: screenshot2.png

Screenshot of Pimcore CMS, also a lot of obvious style bugs.

Comment 2

[[:philipp]]

[Tracking Requested - why for this release]:
sounds like a regression with potentially big impact

Comment 3

[Oriol Brufau [:Oriol]]

Attachment: testcase.htm

Load this testcase.
Result: red background.
Expected: green background.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

My guess is that the code actually depends on the order of child sheets which I changed in bug 1352968. Although the order had already been broken in some case before that, reversing it may make things even worse, I guess.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is a pretty basic testcase to be broken.  Seems like this requires both (a) prompt fix or backout and (b) a test that actually tests that case.

Comment 6

[Oriol Brufau [:Oriol]]

Reversing the order is a violation of https://drafts.csswg.org/css-cascade-4/#cascade-order

> The last declaration in document order wins. For this purpose:
> 
> - Declarations from imported style sheets are ordered as if their
>   style sheets were substituted in place of the @import rule.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Attachment: another testcase

This is a testcase which was already broken before that change because of dependence of child sheet order.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Oriol [:Oriol] from comment #6)
> Reversing the order is a violation of
> https://drafts.csswg.org/css-cascade-4/#cascade-order
> 
> > The last declaration in document order wins. For this purpose:
> > 
> > - Declarations from imported style sheets are ordered as if their
> >   style sheets were substituted in place of the @import rule.

Right. I didn't realize that cascading depends on the internal order of child sheet, because that order already didn't reflect the rule order when there are dynamic changes. Thus the testcase I just uploaded was broken before that change.

I think I have an idea about how to fix both cases.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1368782 - Always use rule order for cascading child sheets.

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

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8872845 [details]
Bug 1368782 - Always use rule order for cascading child sheets.

Doesn't this defeat the de-duplication that the CSS loader does?  In other words, didn't we previously call CascadeSheet once if a child sheet was @import-ed twice, and we now do so twice?

Also, does it have any effect on @import loops?

(What broke the order of child sheets, and can it be reverted?)

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, does this restore all of the tests in https://www.hixie.ch/tests/evil/css/import/ (both parts) to their behavior prior to bug 1352968?

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

OK, so yeah, it doesn't work properly with @import loop. I need to think a bit about how to handle this.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8872845 [details]
Bug 1368782 - Always use rule order for cascading child sheets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144352/diff/1-2/

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #11)
> Also, does this restore all of the tests in
> https://www.hixie.ch/tests/evil/css/import/ (both parts) to their behavior
> prior to bug 1352968?

I manually went through hixie's import test with my latest patch, and it seems to me the result is identical to that from the beta version.

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #10)
> Comment on attachment 8872845 [details]
> Bug 1368782 - Always use rule order for cascading child sheets.
> 
> Doesn't this defeat the de-duplication that the CSS loader does?  In other
> words, didn't we previously call CascadeSheet once if a child sheet was
> @import-ed twice, and we now do so twice?

This has been addressed in the new revision of the patch.

> Also, does it have any effect on @import loops?

An import rule would not have its stylesheet set if there is @import loop involves. See Loader::LoadChildSheet [1] that aGeckoParentRule would get its child sheet only after we finish the checks.

> (What broke the order of child sheets, and can it be reverted?)

Dynamic changes would break the order of child sheets in the linked list. See test 2 and test 3 added in the patch. Both of the two tests fail before bug 1352968.

If we want to keep the order correct in the child sheet linked list... It is doable, but I suspect whether it's worth.


[1] https://dxr.mozilla.org/mozilla-central/rev/39d5cc0fda5e16c49a59d29d4ca186a5534cc88b/layout/style/Loader.cpp#2270-2271

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8872845 [details]
Bug 1368782 - Always use rule order for cascading child sheets.

https://reviewboard.mozilla.org/r/144352/#review148190

::: layout/style/nsCSSRuleProcessor.cpp:3740
(Diff revision 2)
> +    auto& rules = aSheet->Inner()->mOrderedRules;
> +    uint32_t i = 0, len = rules.Length();
> +    for (; i < len; i++) {
> +      if (rules[i]->GetType() != css::Rule::IMPORT_RULE) {
> +        break;
> +      }
> +    }

This isn't going to work if there's an @charset rule before the @import rule.  It's also pretty fragile.

::: layout/style/nsCSSRuleProcessor.cpp:3759
(Diff revision 2)
> +        // 2. the sheet has been referenced by another import rule at a
> +        //    later position.

Doesn't this comment imply that the deduplication code isn't necessary at all, and the first patch would be fine?

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8872845 [details]
Bug 1368782 - Always use rule order for cascading child sheets.

https://reviewboard.mozilla.org/r/144352/#review148190

> This isn't going to work if there's an @charset rule before the @import rule.  It's also pretty fragile.

`@charset` doesn't have CSSOM object, so it is never the case. No other rules can be put before import rules, and that is required in the spec, so I don't think there could be any problem here. Also, this is the reason I added the assertion at the top of `CascadeRuleEnumFunc`.

> Doesn't this comment imply that the deduplication code isn't necessary at all, and the first patch would be fine?

I think it is necessary. This check is done explicitly via the hash set, not the check for whether style sheet returned is null.

If an import rule doesn't have stylesheet connected, this behavior itself would violates the spec, I believe. So I think the explicit extra check is necessary if we want to avoid cascading duplicate stylesheet.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8872845 [details]
Bug 1368782 - Always use rule order for cascading child sheets.

https://reviewboard.mozilla.org/r/144352/#review148198

So two further thoughts:

 1. a hashtable is a rather inefficient structure to use here; it would be more efficient to use booleans on the sheets themselves, perhaps?

 2. this doesn't coalesce multiple sheets except when they're siblings of each other.  For example, a sheet C imported as A->C and A->B->C will still get included twice.

Solving both of them fully seems difficult, though, though switching to solve (1) would solve part of (2).

Hopefully the hashtable inefficiency won't be too big a deal, though.  So I guess r=dbaron.

::: layout/style/nsCSSRuleProcessor.cpp:3758
(Diff revision 2)
> +      for (uint32_t j = i; j > 0; j--) {
> +        auto importRule = static_cast<css::ImportRule*>(rules[j - 1]);
> +        StyleSheet* sheet = importRule->GetStyleSheet();
> +        // There are two cases we want to ignore an import rule:
> +        // 1. the import rule does not have stylesheet connected because
> +        //    it fails in security check or there is a loop involves.

loop involves -> loop involved

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

I wouldn't expect the hashtable to be inefficient here actually. Inefficiency of hashtables are generally from two sides, IIUC: 1. cache miss because of random access in a large memory area, 2. computation of hash key.

For pointer hash key, the computation should be pretty cheap, so the second one shouldn't be an issue. And if there aren't lots of import rules, the hashtable should be pretty small, probably wouldn't be much larger than the array (IIRC, our hash table is usually pretty dense), so I wouldn't expect cache miss to be a significant issue either.

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #17)
>  2. this doesn't coalesce multiple sheets except when they're siblings of
> each other.  For example, a sheet C imported as A->C and A->B->C will still
> get included twice.

And the previous method which iterates child sheet linked list doesn't do so either, as far as I can see.

It is not terribly hard to do this optimization, I mean, we just need to collect all import rules recursively. But it is not clear to me whether it's really worth.

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

If we want to avoid hash table, we can do O(n^2) check with the array as well. I don't expect any real website to have thousands of import rules listed in a single sheet, so that would probably be faster than a hash table check in normal cases... but let's don't worry too much about this until it shows up in some profile, I guess.

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8872845 [details]
Bug 1368782 - Always use rule order for cascading child sheets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144352/diff/2-3/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b19afe698a9e
Always use rule order for cascading child sheets. r=dbaron

Comment 23

[Carsten Book [:Tomcat]]

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