Bug 88869 - renderer should not block on script-inserted stylesheets
Summary: renderer should not block on script-inserted stylesheets
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Performance
Depends on:
Blocks:
 
Reported: 2012-06-12 07:04 PDT by Bryan
Modified: 2017-11-29 11:25 PST (History)
10 users (show)

See Also:


Attachments
Patch (7.10 KB, patch)
2012-06-13 14:21 PDT, Shrey Banga
no flags Details | Formatted Diff | Diff
Patch (7.12 KB, patch)
2012-06-13 14:52 PDT, Shrey Banga
no flags Details | Formatted Diff | Diff
Patch (7.27 KB, patch)
2012-06-13 18:04 PDT, Shrey Banga
koivisto: review-
Details | Formatted Diff | Diff
test case for getComputedStyle (738 bytes, text/html)
2012-10-09 13:29 PDT, Ojan Vafai
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Bryan 2012-06-12 07:04:42 PDT
The HTML5 spec, section 4.2.7, reads:

"A style sheet in the context of the Document of an HTML parser or XML parser is said to be a style sheet that is blocking scripts if the element was created by that Document's parser..."

I read this to mean that script-inserted sheets should not block the parser. Webkit seems to partially follow this, but there are cases where webkit could be more efficient.

For instance, if you look at the network waterfall for http://stevesouders.com/cuzillion/?c0=hc1dfff4_0_f&c1=hj1wfff0_0_f&c2=hb0hfff0_0_f&c3=hj1wfff2_0_f&t=1339507371 in a webkit browser:
http://www.webpagetest.org/result/120612_FQ_M0G/1/details/

You see that the first document-written script after the script-inserted stylesheet is not blocked on the parser, but the second one is. I get the impression that webkit is not immediately marking the sheet as renderer blocking (otherwise the inline script that does the doc-write of the first script would not execute until after the sheet is applied) however webkit does seem to block the second document.write on the sheet being applied, which is unnecessary.

By contrast, the same load in Firefox does not block either script on the sheet:
http://www.webpagetest.org/result/120612_9A_KS7/1/details/

This ends up causing a substantial renderer delay for sites like www.nytimes.com, which inserts a sheet via script which has a series of serialized @imported sheets that cause excess renderer delay due to the serialization of the sheet loads (see homepage.css, which @imports base.css, which @imports tooltip.css). There are other scripts that could be executed during the time these sheets are loading, but they end up getting blocked on each of these sheets getting downloaded serially and finally being applied.
Comment 1 Tony Gentilcore 2012-06-12 07:21:26 PDT
If FF is matching the spec already, it is probably safe for us to do that as well. Patch should be straightforward: probably removing a certain call to Document::addPendingSheet().
Comment 2 Shrey Banga 2012-06-13 14:21:49 PDT
Created attachment 147411 [details]
Patch
Comment 3 WebKit Review Bot 2012-06-13 14:27:41 PDT
Attachment 147411 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Shrey Banga 2012-06-13 14:52:16 PDT
Created attachment 147416 [details]
Patch
Comment 5 Tony Gentilcore 2012-06-13 15:49:03 PDT
Patch looks reasonable. Some quick questions:
- what does this do with @import stylesheets?
- can you verify other browsers' behavior here?
- Bryan mentions nytimes suffering from this issue. If you are able to measure a percent improvement here, that would be worth mentioning in the change log.
Comment 6 Shrey Banga 2012-06-13 18:04:01 PDT
Created attachment 147455 [details]
Patch
Comment 7 Shrey Banga 2012-06-13 18:04:38 PDT
(In reply to comment #5)
> Patch looks reasonable. Some quick questions:
> - what does this do with @import stylesheets?
Essentially for any stylesheet to be counted as blocking, its highest ancestor in the import chain should be an inline <style> or <link> tag. So if a document.write is used to insert a stylesheet which further @imports other stylesheets, all of them will be considered non-blocking.

> - can you verify other browsers' behavior here?
Our behavior matches Firefox's after this fix.

> - Bryan mentions nytimes suffering from this issue. If you are able to measure a percent improvement here, that would be worth mentioning in the change log.
web page replay showed ~100ms (2.7%) improvement in page load time for www.nytimes.com when tested with a connection of 2mbps/400kbps/40ms connection. Added that to the changelog.
Comment 8 Darin Adler 2012-06-13 20:28:50 PDT
Hyatt, what do you think of this change?
Comment 9 Tony Gentilcore 2012-06-15 08:48:35 PDT
Comment on attachment 147455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147455&action=review

A couple of suggestions on the test, but the rest looks good to me.

Hyatt, do you want a chance to review?

> LayoutTests/http/tests/css/script-inserted-stylesheet.html:39
> +      timerID = window.setInterval(function() {checkStyleSheetLoaded(elem)}, 30);

elem.onload would be your friend here.

But rather than checking times at all, why not just verify that the sheet is not in the document at the time the below scripts run. Then you don't even have to wait the 1s for the sheet to load.

> LayoutTests/http/tests/css/script-inserted-stylesheet.html:53
> +    document.write('<scr' + 'ipt type="text/javascript" src="resources/script-inserted-stylesheet-external-script-2.js"></scr' + 'ipt>');

Is there any reason to doc.write these as opposed to including them directly.

Also, it would be a little cleaner to do <script ... onload="onScript1Loaded()"> instead of having to have special script resources that call methods in the root doc.
Comment 10 Antti Koivisto 2012-06-15 10:52:15 PDT
Won't this cause flash of unstyled content with script-inserted stylesheets?
Comment 11 Tony Gentilcore 2012-06-15 11:56:01 PDT
(In reply to comment #10)
> Won't this cause flash of unstyled content with script-inserted stylesheets?

Good point. We want to hold up render tree construction even though (I think) we agree we don't need to hold up script execution.

Currently we marry the concepts into a single list of pending sheets. What if we separated the two. Perhaps give Document two fields: m_pendingStylesheets and m_pendingParserBlockingStylesheets (or maybe the latter fits better on HTMLDocumentParser)?
Comment 12 Antti Koivisto 2012-06-15 12:01:24 PDT
Right, we should separate those concepts. Not sure what the best factoring is.
Comment 13 Antti Koivisto 2012-06-15 12:01:54 PDT
Comment on attachment 147455 [details]
Patch

r- due to FOUC concerns.
Comment 14 Antti Koivisto 2012-06-15 12:11:12 PDT
We should have a tracking mechanism that has more data than just the pending count (which is awfully fragile anyway). A CachedCSSStyleSheet* -> BlockingInfo map or similar maybe.
Comment 15 Ilya Grigorik 2012-06-15 12:43:21 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > Won't this cause flash of unstyled content with script-inserted stylesheets?
> 
> Good point. We want to hold up render tree construction even though (I think) we agree we don't need to hold up script execution.

Wouldn't this break getComputedStyle in script execution? Which brings us back to square one.

FWIW, I don't think its unreasonable to say that script inserted stylesheets will not block the renderer at all. If you inject *all* of your stylesheets via JS, then yes, you have just created a FOUC scenario. However, you can (should) rely on parser created stylesheets to be blocking, and you have the flexibility to inject non-blocking stylesheets via JS. 

I was trying to think through this scenario last night, take a look here: https://gist.github.com/2935269 (WIP)
Comment 16 Ilya Grigorik 2012-06-17 11:44:17 PDT
On some further thinking about this.. It's not clear that FF behavior is what you want all the time. We already have a *working* mechanism to determine if a stylesheet should block renderer / parsing, or not.. media-queries. 

If no media query is specified on the inserted element, then it *should* block - the implicit media is "screen", which always applies. However, if a media query is specified and evaluates to false, then we *should not* hold up the parser or rendering. 

The good news is, that's behavior as it exists today: http://jsbin.com/3/omulof/9/quiet

Making all injected stylesheets non-blocking would break the media query interface - I don't think that's what we want. Perhaps the conditional scenario is: if no media query is specified, then default to non-blocking, but if there is a media query, than make sure to evaluate it. If media is simply "screen" or evaluates to true, then it should hold up the parser and everything else. Although I'm not convinced this is a great idea... Thoughts?
Comment 17 Bryan 2012-06-17 12:28:08 PDT
I can understand where you are coming from, Ilya. On first glance it seems odd to allow sheets to be non-blocking. But (1) script-inserted scripts have worked this way for a long time (I claim the same arguments could be made for scripts: that the side-effects of the node-inserted script are not visible to subsequent scripts, which also can be argued as odd but is the way HTML5 specs it), and (2) the HTML5 spec explicitly calls out that only parser-inserted sheets should block ("A style sheet in the context of the Document of an HTML parser or XML parser is said to be a style sheet that is blocking scripts if the element was created by that Document's parser..."). If someone really needs to make a script or sheet blocking and it is being inserted via javascript, there is a mechanism to make this happen: document.write. If it is critical to make the sheet or script blocking, the spec is clear that document.write is the way to do this.

I'm open to blocking construction of the render tree on these sheets but it seems pretty clear that (1) the HTML5 spec does not indicate that these need to be blocking and (2) there already exists a mechanism to insert either scripts or sheets in a blocking manner from JS (doc.write). So I do not think it would be correct to block the renderer here. Given that Firefox has implemented this in a non-blocking way it also seems clear that sites like nytimes.com that insert sheets via script do not depend on it blocking the renderer. Blocking construction of the render tree does seem reasonable to me, though.
Comment 18 Ilya Grigorik 2012-06-17 12:42:03 PDT
Hmm, so what about the hybrid scenario? Default to nonblocking on script inserted stylesheets without a media-query, but do respect the media-query if one is provided? Not doing so would only create pain and head-scratching from developers (and yet another "browser quirk" for libraries to pave over).

Seems like above would give us the performance benefit you're looking for, while preserving the functionality of the element.
Comment 19 Bryan 2012-06-18 06:23:06 PDT
Sure, if the webkit devs think that it makes sense to key off the media query, I'm not opposed to it. It's really their call. I do think that if a web dev is counting on having a sheet being blocking they should be using doc.write, since that's the only cross-browser way to guarantee blocking loading of a sheet inserted via JS. Can you test and see what FF does (blocking vs non-blocking) when a media query is present in a script-node inserted sheet?
Comment 20 Tony Gentilcore 2012-06-18 11:32:53 PDT
I agree w/ comment 17. Let's stick to the spec and get the performance benefit for existing web content that FF is getting here.

If we want to do something else, that discussion should take place on the whatwg.
Comment 21 Ilya Grigorik 2012-06-18 12:35:49 PDT
(In reply to comment #19)
> Can you test and see what FF does (blocking vs non-blocking) when a media query is present in a script-node inserted sheet?

FF doesn't appear to respect media queries on script inserted stylesheets. WebKit does (currently). 

With respect to doc.write's, there are also differences between WebKit and FF: 

Test: http://jsbin.com/3/omulof/12/quiet
Result: https://img.skitch.com/20120618-twkgw1c7jrsp9c2c4gwrm35k6d.jpg

- FF loads the script inserted stylesheet in parallel with doc.write
- multiple doc.write calls in FF result in parallel downloads, and serialized downloads in WebKit

So, in FF, doc.write is "sort off" blocking - you need to rely on script tag boundaries.
Comment 22 Ojan Vafai 2012-10-09 13:29:42 PDT
Created attachment 167836 [details]
test case for getComputedStyle

Firefox 15 outputs:
appended link
initial font-weight: 400
second font-weight: 400
link loaded
body loaded

WebKit r130046 outputs:
appended link
initial font-weight: normal
second font-weight: bold
link loaded
body loaded

So, it looks like we can get away with treating these loads as truly async, e.g. getComputedStyle doesn't block on the stylesheet loading.
Comment 23 Ojan Vafai 2012-10-09 13:30:51 PDT
Performance aside, the Firefox 15 behavior seems preferable to me for web developers. The WebKit behavior is inconsistent.
Comment 24 Ojan Vafai 2012-10-09 13:49:45 PDT
Relatedly, we seem to have some layout issues when blocking on stylesheets: bug 98821.