Closed Bug 1371453 Opened 3 years ago Closed 3 years ago

stylo: adding @import rule with malformed url to a stylesheet hits an assert in ServoCSSRuleList::InsertRule

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(6 files, 1 obsolete file)

Attached file data-added-import.html
ServoCSSRuleList::InsertRule asserts that an import rule's stylesheet has already been added. There are cases that can prevent this from happening, specifically an early exit case in Gecko_LoadStyleSheet. The attached data-added-import.html testcase demonstrates this.
Assigning to Brad, since IIUC he's looking into stuff in this area.
Assignee: nobody → bwerth
Priority: -- → P2
See Also: → 1328223
Attachment #8880903 - Attachment is obsolete: true
Attachment #8880904 - Flags: review?(xidorn+moz)
Comment on attachment 8880904 [details]
Bug 1371453 Part 1: Relax the logic ServoCSSRuleList::InsertRule to fail soft when processing an @import rule with a malformed URL.

https://reviewboard.mozilla.org/r/152266/#review157402

So... it seems the attached testcase is a valid case in Gecko where an import rule can have no stylesheet associated (which doesn't match the spec, though).

I guess the right thing to do is: make `ConstructImportRule` tolerate null stylesheet, and construct an import rule object anyway, without associated styesheet. Then we can turn the `NS_WARNING` in `ServoCSSRuleList::GetRule` into an assertion. `ServoImportRule` may need some update, so that it doesn't crash when `mChildSheet` is null, and comments in callsites of `ConstructImportRule` should also be updated to mention the valid case that they can return null.

In the long term, we may want to be more spec-conforming, that we should create an empty sheet for it anyway. But it is not necessary to handle that for now.
Attachment #8880904 - Flags: review?(xidorn+moz)
Blocks: 1376295
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)

> In the long term, we may want to be more spec-conforming, that we should
> create an empty sheet for it anyway. But it is not necessary to handle that
> for now.

Solving the problem this way -- by creating an empty stylesheet for the import rule -- may be easier. If it is easier, should we do it now?
Flags: needinfo?(xidorn+moz)
That's probably fine... We should file a bug against bug 1365771 for this behavior change, though. Also it might be good to ensure that new stylesheet is visible to stylist, which is the tricky part I'm thinking about.

(FWIW, it seems in Chrome, stylesheet from data url isn't allowed to be accessed from script, so this doesn't have any effect at all.)
Flags: needinfo?(xidorn+moz)
Comment on attachment 8880904 [details]
Bug 1371453 Part 1: Relax the logic ServoCSSRuleList::InsertRule to fail soft when processing an @import rule with a malformed URL.

https://reviewboard.mozilla.org/r/152266/#review157710
Attachment #8880904 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8881451 [details]
Bug 1371453 Part 2: Update ConstructImportRule callsites and other affected code to tolerate ServoImportRules with no attached raw sheet.

https://reviewboard.mozilla.org/r/152586/#review157712

::: layout/style/ServoImportRule.cpp:68
(Diff revision 1)
>  #endif
>  
>  dom::MediaList*
>  ServoImportRule::Media() const
>  {
> +  MOZ_ASSERT(mChildSheet);

Shouldn't we add a if here rather than doing assertion? `CSSImportRule.media` is accessible from script, so we should protect it from crashing rather than asserting, given it is possible that it can be null.
Attachment #8881451 - Flags: review+
Comment on attachment 8881452 [details]
Bug 1371453 Part 4: Add a test demonstrating we can handle bad URLs in import rules.

https://reviewboard.mozilla.org/r/152588/#review157718

Probably it is better having a mochitest checking that we get null in this case, preferably with comment mentioning this doesn't really match the spec at the moment.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #12)
> Probably it is better having a mochitest checking that we get null in this
> case, preferably with comment mentioning this doesn't really match the spec
> at the moment.

I will do that. I couldn't find the spec that states what the expected behavior is for this case. Can you direct me to it?
Flags: needinfo?(xidorn+moz)
The spec is https://drafts.csswg.org/cssom/#the-cssimportrule-interface

It just says that stylesheet should never be null. It should be an empty stylesheet when it is not loadable.

This is tricky, though, because we would have problem on how we should resolve url inside if script inserts new rules. But we probably don't need to care about this for now.
Flags: needinfo?(xidorn+moz)
See Also: → 1326509
The test requires support in Stylo for StyleRuleAdded events, thus the dependency on Bug 1367996.
Depends on: 1367996
Attachment #8881596 - Flags: review?(cam)
Comment on attachment 8881596 [details]
Bug 1371453 Part 3: Change CSSImportRule definition so that media is optional, since in Servo it is derived from an optional stylesheet.

https://reviewboard.mozilla.org/r/152754/#review158248

Sounds fine, but please get a DOM peer's review for the .webidl change.
Attachment #8881596 - Flags: review?(cam) → review+
Attachment #8881452 - Flags: review?(xidorn+moz)
Attachment #8881596 - Flags: review?(bzbarsky)
Attachment #8881452 - Flags: review?(xidorn+moz)
Comment on attachment 8881452 [details]
Bug 1371453 Part 4: Add a test demonstrating we can handle bad URLs in import rules.

https://reviewboard.mozilla.org/r/152588/#review158330

::: layout/style/test/chrome/test_bug1371453.html:18
(Diff revision 7)
> +               .getService(SpecialPowers.Ci.inIDOMUtils);
> +const Cu = SpecialPowers.Components.utils;
> +const { ContentTaskUtils } = Cu.import("resource://testing-common/ContentTaskUtils.jsm", {});
> +document.styleSheetChangeEventsEnabled = true;
> +
> +let isStylo = SpecialPowers.getBoolPref("layout.css.servo.enabled");

You can use `SpecialPowers.DOMWindowUtils.isStyledByServo` instead.

::: layout/style/test/chrome/test_bug1371453.html:29
(Diff revision 7)
> +  is(event.rule.styleSheet, null, "Import rule contains expected null stylesheet.");
> +  todo_isnot(event.rule.styleSheet, null, "Import rule contains a stylesheet.");

I think we only need the `todo_isnot` here, which should effectively be an `is` except that it reports UNEXPECTED-PASS when the two are not equal.

::: layout/style/test/chrome/test_bug1371453.html:32
(Diff revision 7)
> +  if (isStylo) {
> +    is(event.rule.media, null, "stylo: Import rule contains expected null media.");
> +    todo_isnot(event.rule.media, null, "stylo: Import rule contains a media list.");
> +  } else {
> +    isnot(event.rule.media, null, "gecko: Import rule contains a media list.");
> +  }

Probably shorter to just
```javascript
let stylo_todo_isnot = SpecialPowers.DOMWindowUtils.isStyledByServo ? todo_isnot : isnot;
stylo_todo_isnot(event.rule.media, null, "...");
```
Attachment #8881452 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8881596 [details]
Bug 1371453 Part 3: Change CSSImportRule definition so that media is optional, since in Servo it is derived from an optional stylesheet.

https://reviewboard.mozilla.org/r/152754/#review158578

r=me
Attachment #8881596 - Flags: review?(bzbarsky) → review+
Comment on attachment 8882190 [details]
Bug 1371453 Part 5: Fix an unexpected-pass web-platform test.

https://reviewboard.mozilla.org/r/153300/#review158580
Attachment #8882190 - Flags: review?(bzbarsky) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6710e3941e63
Part 1: Relax the logic ServoCSSRuleList::InsertRule to fail soft when processing an @import rule with a malformed URL. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/922e499286f1
Part 2: Update ConstructImportRule callsites and other affected code to tolerate ServoImportRules with no attached raw sheet. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/ca5201127e80
Part 3: Change CSSImportRule definition so that media is optional, since in Servo it is derived from an optional stylesheet. r=bz,heycam
https://hg.mozilla.org/integration/autoland/rev/0715877bf01c
Part 4: Add a test demonstrating we can handle bad URLs in import rules. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/bd1543a9e0f8
Part 5: Fix an unexpected-pass web-platform test. r=bz
You need to log in before you can comment on or make changes to this bug.