Bug 70868 – Stylesheets not created by the parser should not block scripts

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Page Loading

[Johnny(Jianning) Ding]

Created attachment 112446 [details]
test file to show how a stylesheet not created by parse blocks script

According to spec(http://dev.w3.org/html5/spec/Overview.html#a-style-sheet-that-is-blocking-scripts),  a style sheet that is blocking scripts if the element was created by that Document's parser. So Stylesheets not created by the parser should not block scripts, which means that that script shouldn't be dependent on style being recalculated.

By currently in WebKit, Stylesheets not created by the parser did block scripts.
The attached test file shows how it happens. (http://en.wikipedia.org/wiki/United_States_of_America is a case in real world).
From the attached devtool timeline snapshot, you can see the stylesheets not created by the parser triggered multiple immediate style recalculations, which block script for long time.

I will send a patch to fix it

[Johnny(Jianning) Ding]

Created attachment 112447 [details]
devtool timeline snapshot for the test file.

[Johnny(Jianning) Ding]

Created attachment 112458 [details]
patch v1

[Early Warning System Bot]

Comment on attachment 112458 [details]
patch v1

Attachment 112458 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10208664

[Gyuyoung Kim]

Comment on attachment 112458 [details]
patch v1

Attachment 112458 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10208665

[WebKit Review Bot]

Comment on attachment 112458 [details]
patch v1

Attachment 112458 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10210525

[Johnny(Jianning) Ding]

Created attachment 112460 [details]
patch v1, resolved compilation error

[Tony Gentilcore]

Comment on attachment 112460 [details]
patch v1, resolved compilation error

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

r- for lack of a test.

As far as I can tell this is a good and safe optimization. Antti should probably take a look too.

> Source/WebCore/ChangeLog:9
> +        which means that that script shouldn't be dependent on style being immediately recalculated.

It is worth linking to the spec.

> Source/WebCore/ChangeLog:11
> +        No new tests. (OOPS!)

It is worth adding a test to demonstrate this behavior so that we don't regress upon it in the future. This is probably easiest done with an http test that has a slow stylesheet. See http/tests/misc/script-defer-after-slow-stylesheet.html for an example.

Once you have the test, another key question is how other browsers behave. If FF or IE has the same behavior of not blocking, it gives us a lot more confidence in the spec and our interpretation of it. If we are the first to introduce this behavior, we should proceed with more prudence.

[Antti Koivisto]

I concur with Tony. Makes sense but needs tests. We also need to understand the current behavior of the other engines before jumping.

[Johnny(Jianning) Ding]

Created attachment 113032 [details]
test file used by dyna trace

I have tested the firefox5 (FF7 is same) and IE8's behavior by using Dyna Trace AJAX edition to run a specific test to see the JS timeline. 
Please see the attachments, from what I observed, the style not created and inserted by parser do not block script on Firefox and IE. 
It's definitely worth landing this patch.

[Johnny(Jianning) Ding]

Created attachment 113033 [details]
Firefox js timeline

[Johnny(Jianning) Ding]

Created attachment 113034 [details]
IE js timeline

[Johnny(Jianning) Ding]

Created attachment 113035 [details]
current webkit js timeline

[Johnny(Jianning) Ding]

Created attachment 113036 [details]
webkit js timeline after merging the patch.

[Tony Gentilcore]

Thanks for confirming the behavior of the other browsers. That's great to hear. I'm really excited for this patch to land, but we do need a test still.

My previous comment had a suggestion for how a test might be added. Please let me know if you still have any questions about it.

[Johnny(Jianning) Ding]

(In reply to comment #14)
> Thanks for confirming the behavior of the other browsers. That's great to hear. I'm really excited for this patch to land, but we do need a test still.
> 
> My previous comment had a suggestion for how a test might be added. Please let me know if you still have any questions about it.

No problem. One more result, the latest opera is also rendering the https://bug-70868-attachments.webkit.org/attachment.cgi?id=113032 very fast.

[Johnny(Jianning) Ding]

Created attachment 113160 [details]
patch v2, add layout test

Add a layout test in fast/dom/StyleSheet. I put the test in that dir because the spec description this bug is dependent on is defined in style element section.

[Johnny(Jianning) Ding]

Created attachment 113161 [details]
patch v2, add layout test

[Tony Gentilcore]

I wasn't expecting a timing based layout test. They tend to be flaky and hard to maintain.

Is there really no web-platform observable behavior difference as a result of this patch? I was thinking that the test would be able to do something like check the computed style of an element which was styled by a script inserted sheet. Does that force a style recalculation?

[Johnny(Jianning) Ding]

(In reply to comment #18)
> Is there really no web-platform observable behavior difference as a result of this patch?
I can't find better way to show the difference so far. 
> I was thinking that the test would be able to do something like check the computed style of an element which was styled by a script inserted sheet. Does that force a style recalculation?
My test actually calls "getComputedStyle" to force style recalculation. Yes, calling "getComputedStyle" does force a style recalculation.

[Johnny(Jianning) Ding]

one possible way may dump sth in DRT when doing style recalc, then we can monitor   it. Not sure whether there is existing interface inside WebKit I can use.

[Johnny(Jianning) Ding]

Created attachment 113303 [details]
patch v3, add a layout test by leveraging inspector timeline output.

Switch to a new way, which uses inspector timeline output to ensure there is no style recalc when the changed style elements are not created by parser.

[WebKit Review Bot]

Comment on attachment 113303 [details]
patch v3, add a layout test by leveraging inspector timeline output.

Attachment 113303 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10256252

New failing tests:
http/tests/inspector-enabled/stylesheet-not-created-by-parser-should-not-block-script.html

[Johnny(Jianning) Ding]

Created attachment 113306 [details]
patch v3, update layout test.

[Johnny(Jianning) Ding]

Opened bug 71886 to cover this bug.

*** This bug has been marked as a duplicate of bug 71886 ***