Closed Bug 1384789 Opened 6 years ago Closed 6 years ago

Stylo: DevTools needs a way to filter out import rules reached via an import loop

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

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

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(2 files)

Bug 1384789 - Mark import loop sheet complete.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1384789 - Check ancestor URLs to avoid sheet cycles.
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
DevTools expects import rules reached via an import loop to have a null `styleSheet`.  In Stylo, the `styleSheet` object exists, but its properties are null, like `href`, `cssRules`, etc.

This affects the following test:

* devtools/client/styleeditor/test/browser_styleeditor_import_rule.js[1]

which currently goes into an infinite loop trying ask for data on this sheet that will never arrive.

It loads the page:

devtools/client/styleeditor/test/import.html
├─ simple.css
└─ import.css
   └─ import2.css
      └─ import.css (and on...)

So here's what DevTools expects from the DOM:

let importSheet = document.styleSheets[1];
let import2Sheet = importSheet.cssRules[0].styleSheet;

import2Sheet.cssRules[0].styleSheet <- should be null

[1]: https://treeherder.mozilla.org/logviewer.html#?job_id=117569502&repo=try&lineNumber=170106
This assumption is just wrong. The spec mandates a non-null styleSheet, so I think we should get devtools updated.
The spec does agree, the sheet should be non-null even for loading failure, etc.

I'll try to change DevTools to check for import loops by URL, similar to platform's sheet loading:

http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/layout/style/Loader.cpp#2245
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Summary: Stylo: import rules reached via an import loop should have a null `styleSheet` → Stylo: DevTools needs a way to filter out import rules reached via an import loop
Comment on attachment 8891102 [details]
Bug 1384789 - Check ancestor URLs to avoid sheet cycles.

https://reviewboard.mozilla.org/r/162280/#review167554

This looks fine to me.  I'm assuming that this will be tested by browser_styleeditor_import_rule.js as you mention in Comment 0
Attachment #8891102 - Flags: review?(bgrinstead) → review+
Comment on attachment 8891101 [details]
Bug 1384789 - Mark import loop sheet complete.

https://reviewboard.mozilla.org/r/162278/#review167552

I think Cam would be able to tell sooner than me whether something's wrong with the patch or not :)

::: layout/style/ServoBindings.cpp:2528
(Diff revision 1)
>      // get in here with a URL string that NS_NewURI can't handle.  If so,
>      // silently do nothing.  Eventually we should be able to assert that the
>      // NS_NewURI succeeds, here.
>      RefPtr<ServoStyleSheet> emptySheet =
>        aParent->CreateEmptyChildSheet(media.forget());
> +    emptySheet->SetURIs(uri, uri, uri);

I'm not sure this will handle null URIs fine (which can happen). I think it's fine, but probably Cam can say better.

Also, I think this should maybe be in CreateEmptyChildSheet if it does, since this is its only caller.

Also, at least the third URI is "wrong", as far as I can tell (it expects a base URI, and we're giving it the sheet URI...). The first and the second may differ too, I guess, but maybe this is fine...
Attachment #8891101 - Flags: review?(emilio+bugs)
Attachment #8891101 - Flags: review?(cam)
Comment on attachment 8891101 [details]
Bug 1384789 - Mark import loop sheet complete.

https://reviewboard.mozilla.org/r/162278/#review167552

> I'm not sure this will handle null URIs fine (which can happen). I think it's fine, but probably Cam can say better.
> 
> Also, I think this should maybe be in CreateEmptyChildSheet if it does, since this is its only caller.
> 
> Also, at least the third URI is "wrong", as far as I can tell (it expects a base URI, and we're giving it the sheet URI...). The first and the second may differ too, I guess, but maybe this is fine...

Ah, it does check that sheet and base are non-null (args 1 and 3).  Maybe I'll just only call if `uri` is non-null to begin with?  That's good enough for DevTools at least.

As for passing `uri` as the base URI, I wasn't too sure what was correct, since `Loader::CreateSheet` appears to also[1] pass `uri` for all 3.

[1]: http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/layout/style/Loader.cpp#1239-1283
Comment on attachment 8891101 [details]
Bug 1384789 - Mark import loop sheet complete.

https://reviewboard.mozilla.org/r/162278/#review167552

> Ah, it does check that sheet and base are non-null (args 1 and 3).  Maybe I'll just only call if `uri` is non-null to begin with?  That's good enough for DevTools at least.
> 
> As for passing `uri` as the base URI, I wasn't too sure what was correct, since `Loader::CreateSheet` appears to also[1] pass `uri` for all 3.
> 
> [1]: http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/layout/style/Loader.cpp#1239-1283

I think we assume in some places that the "original sheet URI" is non-null, too.  For example in http://searchfox.org/mozilla-central/source/layout/style/Loader.cpp#506 which I think we will hit if we edit a style sheet in devtools that has an @import pointing to an invalid URL that ends up as null.

Since this handling of invalid URIs is kind of idiosyncratic, if you do need an nsIURI object to pass in here, I think it would be find to generate one that we know is invalid but will be parsed, e.g. "about:invalid".

For the base URI, the part in CreateSheet you point to is specifically for inline style sheets, where it's correct to use the URI of the document as all three.  Can we just pass in aURLExtraData->BaseURI() here?
Comment on attachment 8891101 [details]
Bug 1384789 - Mark import loop sheet complete.

https://reviewboard.mozilla.org/r/162278/#review167552

> I think we assume in some places that the "original sheet URI" is non-null, too.  For example in http://searchfox.org/mozilla-central/source/layout/style/Loader.cpp#506 which I think we will hit if we edit a style sheet in devtools that has an @import pointing to an invalid URL that ends up as null.
> 
> Since this handling of invalid URIs is kind of idiosyncratic, if you do need an nsIURI object to pass in here, I think it would be find to generate one that we know is invalid but will be parsed, e.g. "about:invalid".
> 
> For the base URI, the part in CreateSheet you point to is specifically for inline style sheets, where it's correct to use the URI of the document as all three.  Can we just pass in aURLExtraData->BaseURI() here?

Hmm, is the _entire_ block there for inline style sheets?  If `!aURI`, then it mentions "inline style", and pulls URIs from the document.  I assumed the `else` branch[1] meant "not inline style", which appears to set `aURI` for all 3 args.

I was thinking it seemed best to match "normal" sheet creation as much as possible, which appears to use the same URI, if I am following correctly.

Good idea for the null URI case, I'll try "about:invalid".

[1]: http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/layout/style/Loader.cpp#1252-1254
Comment on attachment 8891102 [details]
Bug 1384789 - Check ancestor URLs to avoid sheet cycles.

https://reviewboard.mozilla.org/r/162280/#review167554

Right, that test fails now and will be fixed by these changes.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10)
> Hmm, is the _entire_ block there for inline style sheets?  If `!aURI`, then
> it mentions "inline style", and pulls URIs from the document.  I assumed the
> `else` branch[1] meant "not inline style", which appears to set `aURI` for
> all 3 args.

Sorry, my mistake, I misread that section.  You're right that it's the not inline style case that uses the same nsIURI for all 3 args.

> I was thinking it seemed best to match "normal" sheet creation as much as
> possible, which appears to use the same URI, if I am following correctly.

I think that is a reasonable approach.
Comment on attachment 8891101 [details]
Bug 1384789 - Mark import loop sheet complete.

https://reviewboard.mozilla.org/r/162278/#review168164

::: layout/style/ServoBindings.cpp:2522
(Diff revision 2)
>      // Servo and Gecko have different ideas of what a valid URL is, so we might
>      // get in here with a URL string that NS_NewURI can't handle.  If so,
>      // silently do nothing.  Eventually we should be able to assert that the
>      // NS_NewURI succeeds, here.

Nit: I think this comment needs updating.
Attachment #8891101 - Flags: review?(cam) → review+
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/48fed875311b
Mark import loop sheet complete. r=heycam
https://hg.mozilla.org/integration/autoland/rev/c92dcf2ce236
Check ancestor URLs to avoid sheet cycles. r=bgrins
You need to log in before you can comment on or make changes to this bug.