1685816 - network.preload causes error looping with media="print"

Status: VERIFIED FIXED

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

Description

[jesse.sierks]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.88 Safari/537.36

Steps to reproduce:

network.preload is true

Using this HTML:
<link href="https://sparqlogon.azureedge.net/SharedContent/styles/Print.1.1.css" onerror="this.onerror=null;window.useBackupCssUrl('https://logon.sparqdata.com/SharedContent/styles/Print.1.1.css')" as="style" onload="this.onload=null;this.rel='stylesheet'" media="print" rel="preload" type="text/css">

But I've been able to reproduce it with all sorts of other URLs.

On version 84.0.2 64-bit, multiple people reporting the issue on this version.

The JS code is very simple, just this (the "found" section to fix this issue, which it did):

<script>
if (typeof window.useBackupCssUrl !== "function") {
window.useBackupCssUrl = function (backupUrl, anotherBackupUrl) {
var found = document.querySelectorAll('link[href="' + backupUrl + '"]');
if (found && found.length > 0) {
console.error('won't add duplicate backup css link for ' + backupUrl);
return;
}
if (anotherBackupUrl)
anotherBackupUrl = " onerror='window.useBackupCssUrl('" + anotherBackupUrl + "')'";
else
anotherBackupUrl = "";
document.querySelector('head').innerHTML += '<link rel="stylesheet" href="' + backupUrl + '"' + anotherBackupUrl + ' type="text/css"/>';
console.error('added backup css link to ' + backupUrl);
};
}
</script>

Actual results:

The onerror code got called endlessly - until I quit adding additional link tags to the head. This code currently works in all other browsers, from lowly IE to Chrome to Edge. And it used to work in Firefox, too...

Expected results:

onerror should not have been called at all (because the URL does work); and if it were called, just once

Comment 1

[jesse.sierks]

With a media tag of "screen", or no media tag, it works as expected.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: test-case from reporter

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Err, actual test-case from reporter.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Hopefully slightly more comprehensible test-case

So... This doesn't seem to quite work in other browsers either does it? As in, sure, the error event doesn't fire, but the link isn't loaded either, and printing doesn't show the styles being applied... What am I missing?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

I can dig into why we're firing the error event anyhow, but curious... What's the point of preloading a print stylesheet? It's not matching, anyhow, so it should be low priority.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

https://w3c.github.io/preload/#x2.link-type-preload says:

Note: When the media attribute's value does not match the environment, the resource will not be preloaded.

But it doesn't specify whether an error event should be fired...

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case that shows the interop issue more clearly

So, there is an interop issue, but I think there should be a better definition on the spec for this.

Comment 8

[jesse.sierks]

Ha, you're right of course, it doesn't actually load elsewhere. But it doesn't error, which is the real issue here. Repeated errors at that. I could see one error if you feel that's warranted, but not repeated errors.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1685816 - Don't fire rel=preload error events for invalid type/media/as attributes. r=smaug

This matches other browsers, https://github.com/w3c/preload/issues/155
is the spec issue. There was a test for invalid as values.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to jesse.sierks from comment #8)

Ha, you're right of course, it doesn't actually load elsewhere. But it doesn't error, which is the real issue here. Repeated errors at that. I could see one error if you feel that's warranted, but not repeated errors.

Repeated errors is because you're re-setting the innerHTML of the <head>, which technically blows up the whole <head> and creates a new <link> element. So that's as expected.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/addf2dba4721
Don't fire rel=preload error events for invalid type/media/as attributes. r=smaug

Comment 12

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27122 for changes under testing/web-platform/tests

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/594d2c60a4c7
Fix a mochitest which expected the wrong thing.

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/addf2dba4721
https://hg.mozilla.org/mozilla-central/rev/594d2c60a4c7

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9196190 [details]
Bug 1685816 - Don't fire rel=preload error events for invalid type/media/as attributes. r=smaug

Beta/Release Uplift Approval Request

• User impact if declined: Preload interop issue reported in the wild.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Open test-case, verify that events are not logged.
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): simple change that matches other browsers.
• String changes made/needed: none

Comment 16

[Bogdan Maris, Desktop QA]

Reproduced the initial issue using 84.0.2, verified that using nightly with fix the events are no longer displayed across platforms (Windows 10, macOS 11 and Ubuntu 18.04), leaving qe-verify+ for when/if this will land in Beta.

Comment 17

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 9196190 [details]
Bug 1685816 - Don't fire rel=preload error events for invalid type/media/as attributes. r=smaug

ok, I guess.. approved for 85.0b9

Comment 19

[Julien Cristau [:jcristau]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #15)

• String changes made/needed: none

Actually there's a new string here, so let's let this ride to 86.

Comment 20

[Bogdan Maris, Desktop QA]

Removing qe-verify+ since this was verified on the milestone Fx version.