1459305 - Pending link rel='stylesheet' loads don't block rendering if a flush happens while they're loading. (was: Recent FOUCs in FF 59 seem to be caused by link rel='stylesheet' failing to block rendering)

Status: NEW

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

Description

[Peter Rowntree]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180326160923

Steps to reproduce:

I noticed a really bad FOUC when I added 'async' to a script loaded immediately below a stylesheet in the head:

<link rel="stylesheet" href="css/index.css" type="text/css" />
<script src='js/compiled.js' async></script>

The problem also happened when I tried moving the script --without async-- to the bottom of the body. My top theory for this is that the script had been blocking rendering --because the stylesheet was failing to block. Isn't that stylesheet supposed to block rendering until it's loaded? This all happens on localhost, so network delays are minimal. I'd be happy to make a screencast if that's necessary.


Actual results:

bad FOUC


Expected results:

no FOUC

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Could you provide a test-case please? Otherwise this is not really actionable.

Comment 2

[Peter Rowntree]

(In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05) from comment #1)
> Could you provide a test-case please? Otherwise this is not really
> actionable.
Sure.
No async:
https://unitinterval.com/
async:
https://unitinterval.com/?async

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Ok, I can repro, tyvm!

Pretty sure this is due to the handling of <script async>, so moving to DOM.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase

Needs slow.php, which is incoming too.

You can repro this putting both files in the same directory and using:

php -S localhost:8000

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: slow.php

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase

Whoops, removed the async attribute while testing.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Note that the test-case is not 100% reliable unfortunately. The provided URL is more reliable, but the testcase still repros from time to time.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: More reliable test-case.

If you explicitly force a layout flush it's 100% reproducible. Other browsers still do layout (given offsetWidth returns > 0), but they don't render until the stylesheets are loaded.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Boris, thoughts? Not sure what the best way to address this should be, and you probably have way more background than me on this...

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Also note that this is not a regression, I can repro at least back to FF 40.

Comment 11

[Peter Rowntree]

It looks like it doesn't even need to be slow-loading. This flashes for me on localhost with a one-line css file coming off an ssd:
https://unitinterval.com/fouc.html

Comment 12

[Boris Zbarsky [:bzbarsky]]

So a few things:

1)  If you force a layout flush, we construct the boxes, do layout, etc.  We don't really have a way to prevent painting at that point.

2)  Some other browsers (Chrome and IE) block the _parser_ on the <link>.  That means you can never get FOUC, sort of, but it also means that you get much slower pageloads in some cases.  We might end up doing this at some point, but note that we used to do it back a long time ago and took it out.  Maybe now that we have the parser preloader we can put it back.  Especially if we beef up that preloader to preload @import from <style>.  Note that the testcase logs the offsetWidth of the body, not of the thing with the text in it, so can't tell whether browsers are doing this.

3)  Safari, last I checked, did have something where it would insert nodes in the DOM but then not render them.  It's complicated.  Complicated enough that Blink removed that part from their fork of WebKit and switched to the IE model.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, and in Firefox, if you don't flush layout sheets do in fact block rendering.  They always block execution of non-async scripts, and non-async scripts block the parser.  So if you have a non-async script after a stylesheet, then you get something like the Chrome/IE behavior anyway.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that doesn't need slow.php

This is a testcase without the manual flush and without needing slow.php; it uses hixie's delayed-file tool.

Emilio, does this testcase reproduce the FOUC for you?  I haven't gotten it to for me so far....

In any case, I expect the original testcase linked in this bug does in fact flush layout from the async script, so we start rendering at that point even if the sheet has not loaded yet.

Comment 15

[Boris Zbarsky [:bzbarsky]]

And all that said, I would expect a non-async script at the bottom of <body> to not cause FOUC on its own, since it would not run until the sheet got loaded and the sheet would block rendering because nothing is flushing layout...  I would love to see a testcase that shows the non-async script at bottom of <body> problem.

One last comment: the sheet at https://unitinterval.com/css/index.css doesn't look like it would all come from localhost, because it imports stuff from fonts.googleapis.com.  Unless the localhost version is loading that bit from localhost too?  So it's not surprising that the script wins the race; it has to make fewer round-trips to fewer servers.

The testcase at https://unitinterval.com/fouc.html doesn't have that problem, though.  I'm not quite sure why the sheet ends up losing the race there; it's worth looking into.  Note that I did turn off "layout.css.parsing.parallel" and the sheet is _still_ losing the race there.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Oh, I guess we do the "apply the sheet async" thing even if parallel parsing is turned off, on tip.  That might be why, and might be an actual regression, of sorts.  But that's bug 1438974, which landed in Firefox 60.  So it's presumablt not implicated in what Peter is seeing.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #14)
> Created attachment 8973395 [details]
> Testcase that doesn't need slow.php
> 
> This is a testcase without the manual flush and without needing slow.php; it
> uses hixie's delayed-file tool.
> 
> Emilio, does this testcase reproduce the FOUC for you?  I haven't gotten it
> to for me so far....

It doesn't repro if I load it locally. But if I load it from localhost, then it repros if I press F5 mid-load... Which is kinda bizarre, probably we have some code there doing a layout flush there and what I'm seeing is the old page instead of the new one?

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> Oh, and in Firefox, if you don't flush layout sheets do in fact block
> rendering.  They always block execution of non-async scripts, and non-async
> scripts block the parser.  So if you have a non-async script after a
> stylesheet, then you get something like the Chrome/IE behavior anyway.

Are you sure Chrome blocks the parser? Wouldn't that mean that the async script would only be executed once the stylesheet is loaded? If so, that's not what I see at least, the console message gets logged way before the sheet loads.

Comment 18

[Peter Rowntree]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> And all that said, I would expect a non-async script at the bottom of <body>
> to not cause FOUC on its own, since it would not run until the sheet got
> loaded and the sheet would block rendering because nothing is flushing
> layout...  I would love to see a testcase that shows the non-async script at
> bottom of <body> problem.
> 
> One last comment: the sheet at https://unitinterval.com/css/index.css
> doesn't look like it would all come from localhost, because it imports stuff
> from fonts.googleapis.com.  Unless the localhost version is loading that bit
> from localhost too?  So it's not surprising that the script wins the race;
> it has to make fewer round-trips to fewer servers.
> 
> The testcase at https://unitinterval.com/fouc.html doesn't have that
> problem, though.  I'm not quite sure why the sheet ends up losing the race
> there; it's worth looking into.  Note that I did turn off
> "layout.css.parsing.parallel" and the sheet is _still_ losing the race there.

Here's a version with script at bottom (I get a FOUC):
https://unitinterval.com/?bottom_script
Also, at one point I commented out the google font @import with no effect.

Comment 19

[Peter Rowntree]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> Oh, and in Firefox, if you don't flush layout sheets do in fact block
> rendering.  They always block execution of non-async scripts, and non-async
> scripts block the parser.  So if you have a non-async script after a
> stylesheet, then you get something like the Chrome/IE behavior anyway.

So it looks as if a temporary workaround to get the advantage of async without the FOUC would be:
<link rel="stylesheet" href="css/index.css" type="text/css" />
<script src='js/compiled.js' async></script>
<script></script>
--which works. My only remaining question then would be is there any chance the empty non-async script would impede the async loading and parsing of the earlier one, making the whole execise moot?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> But if I load it from localhost, then it repros if I press F5 mid-load...

Hmm.  I can't seem to reproduce that either.  If you can, do you want to see where the nsContentSink::StartLayout(true) call is coming from?

> Are you sure Chrome blocks the parser?

No.  And in fact it looks like I was wrong about it.  They had an intent to implement that about 2 years ago (see the "Intent to Implement: Block parser on external CSS" thread on blink-dev at <https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/Intent$20to$20Implement$3A$20Block$20parser$20on$20external$20CSS/blink-dev/ZAPP8aTnyn0/JURBX5uFCAAJ>), and I thought they had in fact implemented it, but it looks like they either didn't or it's not enabled by default.  Which means they're doing the same thing as Safari; what it is is sort of described in that thread.

There's also some relevant discussion in the thread at <https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/Intent$20to$20Implement$20and$20Ship$3A$20Stylesheets$20activated$20after$20the$20body$20is$20started$20do$20not$20block$20paint/blink-dev/QC5iefctcag/kkoBNliBAgAJ> and in particular <https://groups.google.com/a/chromium.org/d/msg/blink-dev/QC5iefctcag/yYa7k7KEBgAJ> seems to say that they block the parser for sheets in <body> but keep doing the old thing for sheets in <head>?  I can't reproduce that behavior either, so maybe Patrick is talking about code that is not written or not enabled by default...

> Here's a version with script at bottom (I get a FOUC):

Thank you, I can reproduce that too.  That's bug 1420362.  If you put the <script> right before that <iframe> near the bottom the FOUC will go away, I bet.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Attachment: stack of StartLayout.

Looks like the loading of the sheet gets aborted (aStatus == NS_BINDING_ABORTED), and thus there are no more sheets and we proceed to do layout.

So nothing terribly unexpected I guess?

Comment 22

[Peter Rowntree]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #20)
> > Here's a version with script at bottom (I get a FOUC):
> 
> Thank you, I can reproduce that too.  That's bug 1420362.  If you put the
> <script> right before that <iframe> near the bottom the FOUC will go away, I
> bet.

Right, that's correct. Moving the script above the iframe --or removing the iframe-- eliminates the FOUC.

Also, while I'm here, I understand that your primary concern is fixing the internals, but for me, and perhaps for others who might come upon this bug through a search, an answer from someone knowledgeable about the internals to my question about a temporary workaround would be very helpful, especially if a fix is a while coming or the resolution is won't-fix.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> So nothing terribly unexpected I guess?

Yeah...  It's not clear what the best way to handle that is, but it's not unexpected given current code.

> an answer from someone knowledgeable about the internals to my question about a temporary workaround
> would be very helpful

Oh, sorry, I meant to reply to that and missed it.  Putting a non-async <script> right before the </head> will basically ensure you never get FOUC if all your stylesheet <link>s are in the <head>, because it will block the parser at that point and there will be nothing to lay out until all the sheets are loaded.  It won't adversely affect preceding async scripts.  Following async scripts will still get preloaded, so it might not really affect them either....

Comment 24

[Emilio Cobos Álvarez (:emilio)]

See https://bugs.chromium.org/p/chromium/issues/detail?id=521692#c59 for chromium considering (once again) making sheets parser-blocking.