`nsContentSink::ProcessLinkHeader` can't parse value-less attributes at the end of the value
Categories
(Core :: DOM: Core & HTML, defect, P2)
Tracking
()
People
(Reporter: mayhemer, Assigned: emilio)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
For response header:
Link: </fonts/CanvasTest.ttf?link-header-on-subresource>; rel=preload;as=font;crossorigin
the crossorigin should be a non-void empty string value leading to anonymous internal value. But the algorithm ignores it and skips to the end of parsing. Candidate to rewrite with Tokenizer.
|
Reporter | |
Comment 1•4 years ago
|
||
Breaks WPT preload/link-header-on-subresource.html
|
||
Comment 2•4 years ago
|
||
Hi Honza, could you re-evaluate the Severity (S2) this bug?
|
|
Reporter | |
Comment 3•4 years ago
|
||
I think we can go with S3 here, but this is actually a major flaw. The case of having e.g. "crossorigin" in the end is likely very common and leads to a preload being ignored and thus hurt performance (making an early request for something we will redownload again with different initial conditions)
|
Assignee | |
Updated•3 years ago
|
|
|
Assignee | |
Comment 4•3 years ago
|
||
This is an issue I found while going through this code and
writing/debugging a test for the bug at hand. Without this, the test in
the actual fix for this bug will fail to actually reuse the preloaded
stylesheet.
It seems reasonable to assume that the intersection of quirks mode
documents using link preload headers is small (and in that case we'd
parse the sheet twice, but oh well).
|
|
Assignee | |
Comment 5•3 years ago
|
||
I was hitting the assertion and was investigating a bit. This makes the
registration a bit faster, but doesn't change behavior.
Depends on D103567
|
|
Assignee | |
Comment 6•3 years ago
|
||
This is equivalent to the check in Document::MaybePreLoadImage, since
otherwise this code won't check the preload service at all. Without
this, we can trigger the assertion in PreloaderBase::NotifyOpen when we
have identical Link header and speculative link element preloads.
It's not a correctness nor perf issue, because the CSS loader will
coalesce the stylesheet load anyways, but it seems better to do this
than to remove the assertion, specially given images already do that.
The test-case in the following patch triggers the issue.
Depends on D103568
|
|
Assignee | |
Comment 7•3 years ago
|
||
With crossorigin, empty value and no value are two different things, so
the link header parser needs to handle it.
Without this change the updated test would fail with two entries rather
than one getting returned from:
numberOfResourceTimingEntries("resources/dummy.css?link-header-crossorigin-preload2")
Depends on D103569
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d99414759409 Assume non-quirks mode for style Link header preloads. r=smaug https://hg.mozilla.org/integration/autoland/rev/dd95492ffe11 Make RegisterPreload slightly more efficient. r=smaug https://hg.mozilla.org/integration/autoland/rev/7343844d32f1 Don't trigger duplicate style link preloads for the same preload key. r=smaug https://hg.mozilla.org/integration/autoland/rev/a32d58612fb7 Don't skip over empty value in link header parsing. r=smaug
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27434 for changes under testing/web-platform/tests
|
||
Comment 10•3 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/d99414759409
https://hg.mozilla.org/mozilla-central/rev/dd95492ffe11
https://hg.mozilla.org/mozilla-central/rev/7343844d32f1
https://hg.mozilla.org/mozilla-central/rev/a32d58612fb7
Upstream PR merged by moz-wptsync-bot
|
||
Updated•3 years ago
|
Description
•