Closed Bug 1405761 Opened 4 years ago Closed 4 years ago

css not loaded correctly with rel=preload

Categories

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

56 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox56 --- wontfix
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: michael.heerklotz, Assigned: dragana)

References

()

Details

(Keywords: dev-doc-needed, regression, site-compat, Whiteboard: [webcompat])

Attachments

(1 file)

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

Steps to reproduce:

Since version 56:
With
<link rel="preload" href="my.css" as="style" onload="this.rel='stylesheet'">
+ loadCSS ( https://github.com/filamentgroup/loadCSS )
+ Server HTTP Headers allow no caching for my.css


Actual results:

Firefox downloads my.css but completely ignores it, the styles are not applied.


Expected results:

Firefox should apply the styles from my.css.
Here is an example page:

http://5hd.de/ff/

-> Shows green in Chrome, white in Firefox
This page allows caching for my.css and works as expected:
http://5hd.de/ff-with-caching/
Component: Untriaged → CSS Parsing and Computation
Worked in 55. Regression in 56. Hiro: can you help find the regression point? Thx!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(hikezoe)
Priority: -- → P3
Blocks: 1222633
Component: CSS Parsing and Computation → DOM: Core & HTML
Flags: needinfo?(hikezoe) → needinfo?(dd.mozilla)
Because the resource is not cacheable the prefetch will be canceled and an onerror event will be triggered instead of onload. Therefore css will never be added as stylesheet.

Preloading of no-cacheable resource will land in next couple of days in 58.

Olli, do you want to disable rel=preload for 57?
Flags: needinfo?(dd.mozilla) → needinfo?(bugs)
If the preload we have in 57 isn't web compatible, then it should be disabled.
Flags: needinfo?(bugs)
Do we know how bad the situation is? How often the non-cacheability causes problems? If often, then disabling makes sense.
(In reply to Olli Pettay [:smaug] from comment #6)
> Do we know how bad the situation is? How often the non-cacheability causes
> problems? If often, then disabling makes sense.

i know about this bug and one other report complaining about 2 loads starting: one preload that gets canceled and then the load for the element. This was only complain but probably it is fixed by the web site (not sure).

I do not know of any other bugs.
We have a case in Webcompat. This is happening in Firefox 58 too.
https://webcompat.com/issues/12144

The CSS is defined through a script with:

<link href="/css/main.css" rel="preload" as="style" onload="this.rel=&quot;stylesheet&quot;">


When I directly query the CSS

HTTP/2.0 200 OK
accept-ranges: bytes
cache-control: public, max-age=0
content-encoding: gzip
content-type: text/css; charset=UTF-8
date: Tue, 10 Oct 2017 23:59:25 GMT
etag: W/"13b3e-15ea4198710"
last-modified: Thu, 21 Sep 2017 11:01:30 GMT
server: Caddy
vary: Accept-Encoding
x-powered-by: Express
X-Firefox-Spdy: h2


which is basically don't cache it → cache-control: public, max-age=0


The site it is happening is https://gdg-siberia.com/ which is Google Sponsored Event.

Chrome and Safari displays the site without issue. I didn't test Edge.
Flags: webcompat?
Whiteboard: [webcompat]
I can probably open an issue on Chromium and WebKit projects. 
What the spec is saying?

/me has read Comment #4

> Preloading of no-cacheable resource will land in next couple of days in 58.

Maybe we should probably upstream this to 57.
(In reply to Karl Dubost :karlcow from comment #9)
> I can probably open an issue on Chromium and WebKit projects. 
> What the spec is saying?
> 
> /me has read Comment #4
> 
> > Preloading of no-cacheable resource will land in next couple of days in 58.
> 
> Maybe we should probably upstream this to 57.

The patch is big, I am not really comfortable uplifting it.
(In reply to Karl Dubost :karlcow from comment #8)
> We have a case in Webcompat. This is happening in Firefox 58 too.
> https://webcompat.com/issues/12144
> 
> The CSS is defined through a script with:
> 
> <link href="/css/main.css" rel="preload" as="style"
> onload="this.rel=&quot;stylesheet&quot;">
> 
> 

using link in this way leads to some problems. If some one disables preload, and there may be people that want to do that link rel=preload will not fire onload. I do not see a reason why would we.

The script on this page detects whether preload is supported and if it is not adds a link rel=stylesheet element.
I could try to change the pref so that return preload not supported if preload is disabled.
Attachment #8917314 - Flags: review?(bugs)
Comment on attachment 8917314 [details] [diff] [review]
bug_1405761_v1.patch

>+++ b/modules/libpref/init/all.js
>@@ -2111,17 +2111,17 @@ pref("network.ftp.idleConnectionTimeout"
> // 3: XUL directory viewer
> // all other values are treated like 2
> pref("network.dir.format", 2);
> 
> // enables the prefetch service (i.e., prefetching of <link rel="next"> and
> // <link rel="prefetch"> URLs).
> pref("network.prefetch-next", true);
> // enables the preloading (i.e., preloading of <link rel="preload"> URLs).
>-pref("network.preload", true);
>+pref("network.preload", false);
This needs intent to unship to dev.platform.

Will you enable preload in https://bugzilla.mozilla.org/show_bug.cgi?id=1394778 ?
Attachment #8917314 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 8917314 [details] [diff] [review]
> bug_1405761_v1.patch
> 
> >+++ b/modules/libpref/init/all.js
> >@@ -2111,17 +2111,17 @@ pref("network.ftp.idleConnectionTimeout"
> > // 3: XUL directory viewer
> > // all other values are treated like 2
> > pref("network.dir.format", 2);
> > 
> > // enables the prefetch service (i.e., prefetching of <link rel="next"> and
> > // <link rel="prefetch"> URLs).
> > pref("network.prefetch-next", true);
> > // enables the preloading (i.e., preloading of <link rel="preload"> URLs).
> >-pref("network.preload", true);
> >+pref("network.preload", false);
> This needs intent to unship to dev.platform.
> 
> Will you enable preload in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1394778 ?

I was thinking about opening another bug just because 1394778 is already reviewed. I can enable it in bug 1394778 as well.

This will need an uplift to beta if granted.
Keywords: checkin-needed
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Comment on attachment 8917314 [details] [diff] [review]
bug_1405761_v1.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1222633, rel=preload feature only for cacheable resources.
[User impact if declined]: there is a web compatibility issues where a css is not loaded.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no, it will be soon. I have also verify it on m.-c.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: It is a pref change with a small additional change.
[String changes made/needed]: none
Attachment #8917314 - Flags: approval-mozilla-beta?
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f4d35d13763
If the preload pref is disabled rel=preload should be shown as not supported. r=smaug
Keywords: checkin-needed
So...  Some comments on the patch:

1)  The "import must come first" comment is clearly wrong, because "import" is not even in the list.  Looks like https://hg.mozilla.org/mozilla-central/diff/c41595a24fbd/dom/html/HTMLLinkElement.cpp forgot to remove that comment, and this patch copied it over to the new list too.

2)  Instead of duplicating the list, and needing to keep two lists in sync now, it would have been better to use the setup we used to have for import.  That way someone can't add a new link type to only one of the two lists.  A followup for this would be nice.
Flags: needinfo?(dd.mozilla)
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #18)
> So...  Some comments on the patch:
> 
> 1)  The "import must come first" comment is clearly wrong, because "import"
> is not even in the list.  Looks like
> https://hg.mozilla.org/mozilla-central/diff/c41595a24fbd/dom/html/
> HTMLLinkElement.cpp forgot to remove that comment, and this patch copied it
> over to the new list too.
> 
> 2)  Instead of duplicating the list, and needing to keep two lists in sync
> now, it would have been better to use the setup we used to have for import. 
> That way someone can't add a new link type to only one of the two lists.  A
> followup for this would be nice.

Tanks a lot for the comments. I will fix that in the follow up bug 1408019.
Flags: needinfo?(dd.mozilla)
Hi Dragana, if the pref network.preload is being turned off in this patch, can we just have a one-liner that turns the pref off and uplift that to 57? Do we still need the entire patch from here and the follow up to come from bug 1408019 in 57 if network.preload will be turned off on 57? Just wondering how to minimize risk with every uplift to 57. Thanks for your help!
Flags: needinfo?(dd.mozilla)
> can we just have a one-liner that turns the pref off and uplift that to 57?

No, because that pref doesn't control whether sites _think_ the feature is supported.  So sites will think it's supported when it's actually not, try to use it, and break.

That's what the rest of the patch is about: making the pref control whether relList.supports() returns true for "preload", plus tests.

> and the follow up to come from bug 1408019

Not needed in 57.  It's code/comment cleanup with no functional changes.
Flags: needinfo?(dd.mozilla)
Thanks Boris.
https://hg.mozilla.org/mozilla-central/rev/1f4d35d13763
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8917314 [details] [diff] [review]
bug_1405761_v1.patch

Thanks Bz, must fix, Beta57+
Attachment #8917314 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
To be clear (correct me if I'm wrong), even with this fix preload is still preffed off, right? In which case I don't need to change the docs for now (Fx currently listed as not supporting this feature)?
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #26)
> To be clear (correct me if I'm wrong), even with this fix preload is still
> preffed off, right? In which case I don't need to change the docs for now
> (Fx currently listed as not supporting this feature)?

This bug turned it off. I will turn it on in bug 1394778.
[Tracking Requested - why for this release]: Firefox 56.0.2 is coming shortly. Given that things are broken and the patch's risk is low, this could be a ride-along.
I will nominate this for release.
I was told that 56.0.2 had already been built. Not sure if there would be build 2... https://archive.mozilla.org/pub/firefox/candidates/56.0.2-candidates/
Comment on attachment 8917314 [details] [diff] [review]
bug_1405761_v1.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1222633, rel=preload feature only for cacheable resources.
[User impact if declined]: on some web sites css will not be loaded, because the css-s are added by the script depending if rel=preload is supported.
[Is this code covered by automated tests?]:no, this is only disabling the feature.
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no, but there is a test http://5hd.de/ff/
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it is a very small patch that only disables a feature. (the bigger part of the patch are changes ot the tests)
[String changes made/needed]: none
Attachment #8917314 - Flags: approval-mozilla-release?
Too late to make it into 56.0.2  --- Unless we know that this is very widespread.  We already built and tested for this dot release so I don't want to hold it up and take up more of releng and QE's time as we ramp up to 57 merge day.
Attachment #8917314 - Flags: approval-mozilla-release? → approval-mozilla-release-

Could I clarify the state of this bug? It says that it's been fixed in 57. I'm currently trying it on 65 for link=preload for a JS script and it doesn't appear to be working.

The state of this bug is that it's fixed. This bug was about use of rel="preload" causing some resources to not be fetched at all. It was fixed by disabling rel=preload.

Is there a bug I could monitor for re-enabling of rel="preload"?

See Also: → rel=preload
You need to log in before you can comment on or make changes to this bug.