Closed Bug 1685816 Opened 2 months ago Closed 2 months ago

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

Categories

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

Firefox 84
defect

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox-esr78 --- disabled
firefox84 --- wontfix
firefox85 --- wontfix
firefox86 --- verified

People

(Reporter: jesse.sierks, Assigned: emilio)

References

Details

Attachments

(2 files, 3 obsolete files)

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

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

Attached file test-case from reporter (obsolete) —
Attached file Err, actual test-case from reporter. (obsolete) —
Attachment #9196181 - Attachment is obsolete: true

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?

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.

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

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

Attachment #9196182 - Attachment is obsolete: true
Attachment #9196184 - Attachment is obsolete: true
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Core & HTML
Ever confirmed: true
Product: Firefox → Core

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.

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

Assignee: nobody → emilio
Status: NEW → ASSIGNED

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

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/addf2dba4721
Don't fire rel=preload error events for invalid type/media/as attributes. r=smaug
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27122 for changes under testing/web-platform/tests
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/594d2c60a4c7
Fix a mochitest which expected the wrong thing.
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch

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
Attachment #9196190 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

Upstream PR merged by moz-wptsync-bot

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

Attachment #9196190 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Attachment #9196190 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.