Closed Bug 1637651 Opened 1 year ago Closed 3 months ago

`nsContentSink::ProcessLinkHeader` can't parse value-less attributes at the end of the value

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
87 Branch
Tracking Status
firefox76 --- wontfix
firefox77 --- wontfix
firefox78 --- wontfix
firefox87 --- fixed

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.

Breaks WPT preload/link-header-on-subresource.html

Hi Honza, could you re-evaluate the Severity (S2) this bug?

Flags: needinfo?(honzab.moz)

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)

Severity: S2 → S3
Flags: needinfo?(honzab.moz)
Assignee: nobody → emilio

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).

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

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

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
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.