Closed Bug 1368381 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: xidorn, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Bug 1368381 Part 1: Add a test that exercises rule insertion within cloned import rule sheets.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
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.
Blocks: 1368651
I'll see if I can solve this.
Assignee: nobody → bwerth
Priority: -- → P2
(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?
Flags: needinfo?(xidorn+moz)
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?
Flags: needinfo?(xidorn+moz)
(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.
This is probably the reason of crash in test layout/inspector/tests/test_bug536379-2.html
Blocks: stylo-mochitest
Attachment #8880608 - Attachment is obsolete: true
(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...
Attachment #8880609 - Flags: review?(xidorn+moz)
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 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?
Attachment #8880609 - Flags: review?(xidorn+moz) → review-
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.
Blocks: 1369452
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.
Depends on: 1372041
Attachment #8880609 - Flags: review?(xidorn+moz)
Attachment #8880610 - Attachment is obsolete: true
(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.
Attachment #8880609 - Flags: review?(xidorn+moz)
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.
Attachment #8880609 - Flags: review?(xidorn+moz) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30cb57034d1e
Part 1: Add a test that exercises rule insertion within cloned import rule sheets. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/30cb57034d1e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: