Bug 70868 - Stylesheets not created by the parser should not block scripts
: Stylesheets not created by the parser should not block scripts
Status: RESOLVED DUPLICATE of bug 71886
Product: WebKit
Classification: Unclassified
Component: Page Loading
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Johnny(Jianning) Ding
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-25 21:47 PDT by Johnny(Jianning) Ding
Modified: 2011-11-09 03:53 PST (History)
6 users (show)

See Also:


Attachments
test file to show how a stylesheet not created by parse blocks script (972 bytes, text/html)
2011-10-25 21:47 PDT, Johnny(Jianning) Ding
no flags Details
devtool timeline snapshot for the test file. (175.28 KB, image/png)
2011-10-25 21:48 PDT, Johnny(Jianning) Ding
no flags Details
patch v1 (4.90 KB, patch)
2011-10-26 00:08 PDT, Johnny(Jianning) Ding
webkit-ews: commit‑queue-
Details | Formatted Diff | Diff
patch v1, resolved compilation error (5.19 KB, patch)
2011-10-26 00:37 PDT, Johnny(Jianning) Ding
tonyg: review-
Details | Formatted Diff | Diff
test file used by dyna trace (1.29 KB, text/html)
2011-10-31 03:57 PDT, Johnny(Jianning) Ding
no flags Details
Firefox js timeline (42.95 KB, image/png)
2011-10-31 03:58 PDT, Johnny(Jianning) Ding
no flags Details
IE js timeline (45.64 KB, image/png)
2011-10-31 03:58 PDT, Johnny(Jianning) Ding
no flags Details
current webkit js timeline (93.27 KB, image/png)
2011-10-31 03:59 PDT, Johnny(Jianning) Ding
no flags Details
webkit js timeline after merging the patch. (89.73 KB, image/png)
2011-10-31 03:59 PDT, Johnny(Jianning) Ding
no flags Details
patch v2, add layout test (11.71 KB, patch)
2011-11-01 05:14 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch v2, add layout test (11.46 KB, patch)
2011-11-01 05:30 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch v3, add a layout test by leveraging inspector timeline output. (12.17 KB, patch)
2011-11-02 04:33 PDT, Johnny(Jianning) Ding
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
patch v3, update layout test. (12.31 KB, patch)
2011-11-02 05:53 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Johnny(Jianning) Ding 2011-10-25 21:47:22 PDT
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
Comment 1 Johnny(Jianning) Ding 2011-10-25 21:48:51 PDT
Created attachment 112447 [details]
devtool timeline snapshot for the test file.
Comment 2 Johnny(Jianning) Ding 2011-10-26 00:08:28 PDT
Created attachment 112458 [details]
patch v1
Comment 3 Early Warning System Bot 2011-10-26 00:20:13 PDT
Comment on attachment 112458 [details]
patch v1

Attachment 112458 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10208664
Comment 4 Gyuyoung Kim 2011-10-26 00:22:29 PDT
Comment on attachment 112458 [details]
patch v1

Attachment 112458 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10208665
Comment 5 WebKit Review Bot 2011-10-26 00:29:30 PDT
Comment on attachment 112458 [details]
patch v1

Attachment 112458 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10210525
Comment 6 Johnny(Jianning) Ding 2011-10-26 00:37:44 PDT
Created attachment 112460 [details]
patch v1, resolved compilation error
Comment 7 Tony Gentilcore 2011-10-26 02:46:29 PDT
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.
Comment 8 Antti Koivisto 2011-10-26 03:34:22 PDT
I concur with Tony. Makes sense but needs tests. We also need to understand the current behavior of the other engines before jumping.
Comment 9 Johnny(Jianning) Ding 2011-10-31 03:57:51 PDT
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.
Comment 10 Johnny(Jianning) Ding 2011-10-31 03:58:35 PDT
Created attachment 113033 [details]
Firefox js timeline
Comment 11 Johnny(Jianning) Ding 2011-10-31 03:58:56 PDT
Created attachment 113034 [details]
IE js timeline
Comment 12 Johnny(Jianning) Ding 2011-10-31 03:59:26 PDT
Created attachment 113035 [details]
current webkit js timeline
Comment 13 Johnny(Jianning) Ding 2011-10-31 03:59:59 PDT
Created attachment 113036 [details]
webkit js timeline after merging the patch.
Comment 14 Tony Gentilcore 2011-10-31 04:17:10 PDT
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.
Comment 15 Johnny(Jianning) Ding 2011-10-31 04:44:57 PDT
(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.
Comment 16 Johnny(Jianning) Ding 2011-11-01 05:14:17 PDT
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.
Comment 17 Johnny(Jianning) Ding 2011-11-01 05:30:10 PDT
Created attachment 113161 [details]
patch v2, add layout test
Comment 18 Tony Gentilcore 2011-11-01 07:35:10 PDT
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?
Comment 19 Johnny(Jianning) Ding 2011-11-01 07:39:40 PDT
(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.
Comment 20 Johnny(Jianning) Ding 2011-11-01 07:54:14 PDT
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.
Comment 21 Johnny(Jianning) Ding 2011-11-02 04:33:07 PDT
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.
Comment 22 WebKit Review Bot 2011-11-02 05:22:58 PDT
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
Comment 23 Johnny(Jianning) Ding 2011-11-02 05:53:07 PDT
Created attachment 113306 [details]
patch v3, update layout test.
Comment 24 Johnny(Jianning) Ding 2011-11-09 03:53:27 PST
Opened bug 71886 to cover this bug.

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