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

RESOLVED FIXED in Firefox 56

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bradwerth, Assigned: bradwerth)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
See Also: → 1328223
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8880903 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8880904 - Flags: review?(xidorn+moz)

Comment 4

2 years ago
mozreview-review
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)
(Assignee)

Updated

2 years ago
Blocks: 1376295
(Assignee)

Comment 5

2 years ago
(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 hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
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 11

2 years ago
mozreview-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 12

2 years ago
mozreview-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.
(Assignee)

Comment 13

2 years ago
(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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
See Also: → 1326509
(Assignee)

Comment 19

2 years ago
The test requires support in Stylo for StyleRuleAdded events, thus the dependency on Bug 1367996.
Depends on: 1367996
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8881452 - Flags: review?(xidorn+moz)
(Assignee)

Updated

2 years ago
Attachment #8881596 - Flags: review?(bzbarsky)
(Assignee)

Updated

2 years ago
Attachment #8881452 - Flags: review?(xidorn+moz)

Comment 35

2 years ago
mozreview-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/#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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 43

2 years ago
mozreview-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 44

2 years ago
mozreview-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+

Comment 45

2 years ago
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.