Michael Heerklotz Reporter
	Description • 7 years ago

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.

	Michael Heerklotz Reporter
	Comment 1 • 7 years ago

Here is an example page:

http://5hd.de/ff/

-> Shows green in Chrome, white in Firefox

	Michael Heerklotz Reporter
	Comment 2 • 7 years ago

This page allows caching for my.css and works as expected:
http://5hd.de/ff-with-caching/

	Jet Villegas (inactive)
	Comment 3 • 7 years ago

Worked in 55. Regression in 56. Hiro: can you help find the regression point? Thx!

	Dragana Damjanovic [:dragana] Assignee
	Comment 4 • 7 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 7 years ago

If the preload we have in 57 isn't web compatible, then it should be disabled.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 7 years ago

Do we know how bad the situation is? How often the non-cacheability causes problems? If often, then disabling makes sense.

	Dragana Damjanovic [:dragana] Assignee
	Comment 7 • 7 years ago

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

	Karl Dubost💡 :karlcow
	Comment 8 • 7 years ago

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.

	Karl Dubost💡 :karlcow
	Comment 9 • 7 years ago

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.

	Dragana Damjanovic [:dragana] Assignee
	Comment 10 • 7 years ago

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

	Dragana Damjanovic [:dragana] Assignee
	Comment 11 • 7 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 7 years ago

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 ?

	Dragana Damjanovic [:dragana] Assignee
	Comment 14 • 7 years ago

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

	Dragana Damjanovic [:dragana] Assignee
	Comment 15 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b68217629b385cd595bac5783ca2e2588a428ce

	Dragana Damjanovic [:dragana] Assignee
	Comment 16 • 7 years ago

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

	Pulsebot
	Comment 17 • 7 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 7 years ago

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.

	Dragana Damjanovic [:dragana] Assignee
	Comment 19 • 7 years ago

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

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 20 • 7 years ago

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!

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 7 years ago

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

	Dragana Damjanovic [:dragana] Assignee
	Comment 22 • 7 years ago

Thanks Boris.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 23 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1f4d35d13763

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 24 • 7 years ago

Comment on attachment 8917314 [details] [diff] [review]
bug_1405761_v1.patch

Thanks Bz, must fix, Beta57+

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/931b496bafe6

	Chris Mills [:cmills]
	Comment 26 • 7 years ago

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

	Dragana Damjanovic [:dragana] Assignee
	Comment 27 • 7 years ago

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

	Kohei Yoshino [:kohei]
	Comment 28 • 7 years ago

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

	Kohei Yoshino [:kohei]
	Comment 29 • 7 years ago

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/css-loaded-with-link-rel-preload-are-not-applied/

	Dragana Damjanovic [:dragana] Assignee
	Comment 30 • 7 years ago

I will nominate this for release.

	Kohei Yoshino [:kohei]
	Comment 31 • 7 years ago

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/

	Dragana Damjanovic [:dragana] Assignee
	Comment 32 • 7 years ago

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

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 33 • 7 years ago

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.