1368381 - stylo: Stylesheet reusing causes mismatch between servo sheet and gecko sheet for @import rule

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Xidorn Quan [:xidorn] UTC+10]

If I do
> stylesheet.insertRule("@import url(test.css)", 0);
> stylesheet.insertRule("@import url(test.css)", 0);

The first one would create a ServoStyleSheet instance ("gecko_a"), and store the pointer to the Servo side Stylesheet instance ("servo_a") of the import rule. Gecko's loader would then load data into "servo_a" via "gecko_a".ParseSheet(), IIUC.

When the second line is invoked, then, Loader::CreateSheet() would try reusing "gecko_a", so it returns a clone ("gecko_b") of "gecko_a". Now, "gecko_b"'s inner also contains the pointer to "servo_a", but in the Servo side, the corresponding import rule has an independent stylesheet "servo_b" connected. Gecko side would never connect "gecko_b" to "servo_b" because in Loader::InsertChildSheet(), SetSheetForImport() is skipped because "gecko_b" is already referring "servo_a".

Because of this mismatch, "servo_b" would never be updated, because Gecko side thinks "gecko_b" is for "servo_a".

It is not completely clear to me what should be done. Calling SetSheetForImport() may not be correct either because the inner may be shared by different stylesheets. Calling EnsureUniqueInner() doesn't seem to be the right thing either, because it would clone "servo_a" to a "servo_c", which import rule of Servo side would never be aware of either.

Comment 1

[Brad Werth [:bradwerth]]

I'll see if I can solve this.

Comment 2

[Brad Werth [:bradwerth]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0)
> If I do
> > stylesheet.insertRule("@import url(test.css)", 0);
> > stylesheet.insertRule("@import url(test.css)", 0);
> 
> The first one would create a ServoStyleSheet instance ("gecko_a"), and store
> the pointer to the Servo side Stylesheet instance ("servo_a") of the import
> rule. Gecko's loader would then load data into "servo_a" via
> "gecko_a".ParseSheet(), IIUC.

The warning at http://searchfox.org/mozilla-central/source/layout/style/ServoCSSRuleList.cpp#278 is triggered even when just one imported sheet is added. Is this warning logic correct? Why shouldn't the raw sheet of the first child be the same as the raw sheet loaded by the import rule?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

The logic is reversed... sorry...

This condition should really be "servoSheet->RawSheet() != raw" then show the warning and return nullptr...

Probably you can fix this with your changes in this bug together, so that we don't need an additional bug and review pass?

Comment 4

[Brad Werth [:bradwerth]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> Probably you can fix this with your changes in this bug together, so that we
> don't need an additional bug and review pass?

Yes. I hope to have a patch soon.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

This is probably the reason of crash in test layout/inspector/tests/test_bug536379-2.html

Comment 6

[Brad Werth [:bradwerth]]

Attachment: temp lresolv

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

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1368381 Part 1: Add a test that exercises rule insertion within cloned import rule sheets.

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

Comment 8

[Brad Werth [:bradwerth]]

Attachment: Bug 1368381 Part 2: Add a reftest to verify reuse of import rule stylesheets.

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

Comment 9

[Brad Werth [:bradwerth]]

Comment on attachment 8880609 [details]
Bug 1368381 Part 1: Add a test that exercises rule insertion within cloned import rule sheets.

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

Comment 10

[Brad Werth [:bradwerth]]

Comment on attachment 8880610 [details]
Bug 1368381 Part 2: Add a reftest to verify reuse of import rule stylesheets.

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

Comment 11

[Brad Werth [:bradwerth]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #5)
> This is probably the reason of crash in test
> layout/inspector/tests/test_bug536379-2.html

The proposed patch addresses the issue in comment 0, but this crash persists. Investigating...

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8880610 [details]
Bug 1368381 Part 2: Add a reftest to verify reuse of import rule stylesheets.

https://reviewboard.mozilla.org/r/151948/#review157406

Does the added test fail before your part 1? I cannot figure out why it would fail. To verify that, we probably need some dynamic change to be sure?

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8880609 [details]
Bug 1368381 Part 1: Add a test that exercises rule insertion within cloned import rule sheets.

https://reviewboard.mozilla.org/r/151946/#review157408

This seems to be reasonable at first glance. But I wonder what does it really mean for stylesheet sharing.

IIUC, after you unilaterally accept the raw sheet from Servo side, `Loader::LoadChildSheet` would still think that the sheet has been completely loaded (because it has found a reusable sheet), and thus skip loading it. So the Servo side stylesheet would never get updated. This doesn't seem to be the right result.

(The testcase should ensure that stylesheets of the two import rules are identical, both effective, and are independently mutable from the perspective of script.)

::: layout/style/Loader.cpp:1427
(Diff revision 2)
>    // cloned off of top-level sheets which were disabled
>    aSheet->SetEnabled(true);
>    if (aSheet->IsGecko()) {
>      aGeckoParentRule->SetSheet(aSheet->AsGecko()); // This sets the ownerRule on the sheet
>    } else {
> -    if (!aSheet->AsServo()->RawSheet()) {
> +    MOZ_ASSERT(aSheet->IsServo());

This assertion is unnecessary. `AsServo()` already does the same assert.

::: layout/style/ServoStyleSheet.cpp:252
(Diff revision 2)
> +    ServoStyleSheetInner* emptyInner = new ServoStyleSheetInner(
> +      mInner->mCORSMode,
> +      mInner->mReferrerPolicy,
> +      mInner->mIntegrity);
> +    mInner = emptyInner;

It doesn't seem to me you need to do this in two steps. Wouldn't `mInner = new ServoStyleSheetInner(...);` just work?

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Emilio and I discussed about this a bit just now, and it seems to me that we probably want to always update the stylesheet pointer in Servo side from Gecko side. That said, we should not create Stylesheet for import rule when we see rule, but instead, set it when Gecko creates the Servo sheet (in ServoStyleSheet::{ParseSheet,LoadFailed}) or reuses the inner.

That way, we should be able to get rid of Servo_StyleSheet_ClearAndUpdate, and unify the way we handle @import stylesheet and top level stylesheet.

Comment 15

[Brad Werth [:bradwerth]]

Comment on attachment 8880609 [details]
Bug 1368381 Part 1: Add a test that exercises rule insertion within cloned import rule sheets.

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

Comment 16

[Brad Werth [:bradwerth]]

Comment on attachment 8880610 [details]
Bug 1368381 Part 2: Add a reftest to verify reuse of import rule stylesheets.

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

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

emilio's work in bug 1372041 may fix this as well. Let's make it depend on that one and see what would be left after that.

Comment 18

[Brad Werth [:bradwerth]]

Comment on attachment 8880609 [details]
Bug 1368381 Part 1: Add a test that exercises rule insertion within cloned import rule sheets.

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

Comment 19

[Brad Werth [:bradwerth]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #13)
> (The testcase should ensure that stylesheets of the two import rules are
> identical, both effective, and are independently mutable from the
> perspective of script.)

I've created a new test that does these things. I'm not certain this test is sufficent to demonstrate that the core issue is resolved, since there's no direct JS exposure of the stylesheet inners. The test can measure behavior, but not the presence or absence of hidden clones. Conceptually the changes in Bug 1372041 should resolve this issue.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8880609 [details]
Bug 1368381 Part 1: Add a test that exercises rule insertion within cloned import rule sheets.

https://reviewboard.mozilla.org/r/151946/#review159210

::: layout/style/test/chrome/test_stylesheet_clone_import_rule.html:56
(Diff revision 4)
> +      SimpleTest.finish();
> +      return;
> +    }
> +
> +    // Confirm that these two sheets are meaningfully the same.
> +    is(Array.from(importSheet1.cssRules).join(''), Array.from(importSheet2.cssRules).join(''), "Cloned sheet rules are equivalent.");

This doesn't make much sense. Rule objects would be serialized to something like "[object CSSStyleRule]" rather than their real content.

Something like
```javascript
Array.from(rules).map(rule => rule.cssText).join('')
```
might be more sensible.

You may want to have a function to generate that content from a stylesheet.

Comment 21

[Brad Werth [:bradwerth]]

Comment on attachment 8880609 [details]
Bug 1368381 Part 1: Add a test that exercises rule insertion within cloned import rule sheets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151946/diff/4-5/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30cb57034d1e
Part 1: Add a test that exercises rule insertion within cloned import rule sheets. r=xidorn

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30cb57034d1e