Bug 20710 – WebKit should support defer and async on script elements

• Component: Page Loading
• Status: RESOLVED FIXED

[Anthony Ricaud]

IE already support defer and Firefox will support it (https://bugzilla.mozilla.org/show_bug.cgi?id=28293)

Preloading is already making things better but rendering is still blocked while parsing and executing scripts.

http://www.w3.org/TR/html5/tabular.html#script

[Antti Koivisto]

While this won't help complete load time (since we are already able to load scripts in parallel) this is probably worth implementing for the early display benefit.

Does anyone else support async or just defer?

[Robert Blaut]

Test case for "defer" atribute:

Result of the test with the latest Gecko nightly with defer support implemented:

Inline Head
External Head
Inline Body
External Body
Inline Head Deferred
External Head Deferred
Inline Body Deferred
External Body Deferred

Actual WebKit result:

Inline Head Deferred
Inline Head
External Head Deferred
External Head
Inline Body Deferred
Inline Body
External Body Deferred
External Body

[Robert Blaut]

Here you are the link to the above mentioned test: http://www.websiteoptimization.com/speed/tweak/defer/test/

[Michael Dayah]

Firefox finished this. It's in the 3.1 betas now. However, they fire DOMContentLoaded before executing deferred scripts, and they may change their mind on that. Seems like that'd be something worth coordinating. They also don't appear to expose any programmatic way of detecting whether defer is supported, something we also might want to coordinate on doing.

[Michael Dayah]

Firefox decided to fire DCL after deferred scripts to faciliate the load pattern mentioned in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=28293

[Peter Kasting]

Note, the w3 link at top is now invalid.  Here's a link to the current spec on this: http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.html#script

[Michael Dayah]

Internet Explorer before version 8 have a strange defer order. They execute deferred inline scripts before deferred external scripts. IE8 and Firefox 3.5, however, consistently load in the order described in comment #2.

Firefox also has moved the DOMContentLoaded event to fire after deferred scripts are loaded. This appears to be final and a closed issue. I don't anticipate any further changes in their defer implementation.

External dependencies blocking page load is a problem we now have the tools to deal with thanks to defer covering 85% of the web. Defer is also part of the XHTML and HTML 4 specifications. With Webkit's implementation of defer, this will go over 90% and start becoming a mainstream pattern. I suggest this be moved up in priority for these reasons.

[kangax]

> They also don't appear to expose any programmatic way of 
> detecting whether defer is supported, something we also 
> might want to coordinate on doing.

The inference based on property existence works quite well, actually:

'defer' in document.createElement('script'); // true in 3.5+
'async' in document.createElement('script'); // true in 3.6+

// etc.

[Tony Gentilcore]

Created attachment 56451 [details]
Patch

[Tony Gentilcore]

Here's a proposed patch. It supports defer and async on only external scripts and loaded them prior to OnDOMContentLoaded.

I'm slightly worried that although the spec says async and defer must only be supported for external scripts, since Moz and IE support defer on inline scripts, we might have to do that for compat. What do others think?

[Brian Kuhn]

I can't find the exact language in the spec, but Ian claims to have modified it to say that async scripts should not block DOMContentLoaded.  I'd prefer they also didn't block window.onload, but I'll take what I can get...

http://canvex.lazyilluminati.com/misc/cgi/issues.cgi/message/[e-mail]%3E

"On 3/16/2010 5:05 PM, Ian Hickson wrote:
I've changed the spec to fire 'DOMContentLoaded' without waiting for the
async scripts, so that if you need this you can just listen for that event
instead of 'load'. 'load' still waits for all scripts. 'DOMContentLoaded'
still waits for deferred scripts. As far as I can tell this handles all
the above (still makes sense, still consistent with the way other 'load'
events work, but still lets you do things without waiting)."

[WebKit Review Bot]

Attachment 56451 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2315301

[Tony Gentilcore]

Created attachment 56522 [details]
Patch

[Tony Gentilcore]

Brian, you were right. Async should not block DOMContentLoaded. Here's the relevant part of the spec:
http://dev.w3.org/html5/spec/Overview.html#delay-the-load-event

As further validation, I've compiled this table lists the behavior of current browsers and specs as it related to async/defer.

           defer inl.  defer ext.  async  block DCL   block load
           ----------  ----------  -----  ----------  ----------
HTML4 spec      Y          Y         N      undef       undef
HTML5 spec      N          Y         Y    only defer      Y
IE 8            Y          Y         N       N/A          Y
Opera 10.5      N          N         N       N/A         N/A
Firefox 3.6     N          Y         Y        N           Y
Curr WebKit     N          N         N       N/A         N/A
New WebKit      N          Y         Y    only defer      Y

This patch matches the HTML5 spec and only differs from FF3.6 in that we block DCL for defer. So we shouldn't have any web compat problems that aren't in FF3.6.

I've updated the patch as describe above (and it should also now fix the gtk/qt builds).

To reviewer: I could break defer and async into separate patches if you prefer, but I thought it important to see the whole thing first.

[WebKit Review Bot]

Attachment 56522 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2291318

[Tony Gentilcore]

Created attachment 56538 [details]
Patch

[Tony Gentilcore]

Created attachment 56542 [details]
Patch

[Tony Gentilcore]

Adam/Eric, I noticed you guys are landing the HTML5 parser. Thought I should check with you early on this patch to make sure it fits with those plans.

[Tony Gentilcore]

Darin/Maciej, this seems like the sort of thing you guys might be interested in.

[Eric Seidel (no email)]

I'll review this later today.  I just got through re-writing the Script architecture for HTML5 as part of bug 39716.

[Eric Seidel (no email)]

It is unfortunate to put this code on Document since it's only needed during parsing.  Tony and I talked in #webkit he said he'd look at the possibility of re-using HTML5ScriptRunner to do this once a first-pass implementation lands as part of bug 39716 (likely tonight).

I'm not sure we would actually be able to make HTML5ScriptRunner work with the old Tokenizer, but there is also the question of whether we even want to bother fixing this for the soon-to-be-old parser path.  I'm not sure when we'll have the HTML5 parser system ready for prime time though so it may not make sense to block this work either.

Regardless, the tests he wrote for this are super useful.

[Tony Gentilcore]

Created attachment 57565 [details]
Patch

[Tony Gentilcore]

(In reply to comment #21)
> It is unfortunate to put this code on Document since it's only needed during parsing.  Tony and I talked in #webkit he said he'd look at the possibility of re-using HTML5ScriptRunner to do this once a first-pass implementation lands as part of bug 39716 (likely tonight).
> 
> I'm not sure we would actually be able to make HTML5ScriptRunner work with the old Tokenizer, but there is also the question of whether we even want to bother fixing this for the soon-to-be-old parser path.  I'm not sure when we'll have the HTML5 parser system ready for prime time though so it may not make sense to block this work either.
> 
> Regardless, the tests he wrote for this are super useful.

I've update the patch to work with the HTML5ScriptRunner. It worked beautifully with defer. However, async is a little different since it needs to block the load event but not parsing or the DCL event. Since async scripts may outlive the parser, the two options I see for tracking pending async scripts are either the Document or the FrameLoader. This patch uses the document for now. Let me know if you think it would be cleaner to move to FrameLoader or if you have an idea for a third option.

Also, I don't have any expectations along with my new layout tests. What is the right procedure to add the expectations?

[Adam Barth]

Comment on attachment 57565 [details]
Patch

We'll want Eric to do the final review, but here are some comments for now.

LayoutTests/http/tests/local/resources/async-script.js:1
 +  window.status_ += " async ";
The trailing underscore is a Google-ism.  It's probably ok for tests to use a variety of styles, but I just mention it so you're aware.

LayoutTests/ChangeLog:5
 +          Added tests for defer and async script attributes.
Might be worth noting there that these tests are supposed to pass in the HTML5 parser but fail in the old parser.

LayoutTests/ChangeLog:14
 +          * http/tests/local/script-defer.html: Added.
If you run-webkit-tests, it should generate the missing expectation files.  You can specify tests to run on the command line if you don't want to run the whole suite.

LayoutTests/http/tests/local/resources/slow-async-script.cgi:4
 +  sleep(0.5);
Boo.  This will make the test somewhat slow and possibly flaky...  Not sure how to make it better though...

LayoutTests/http/tests/local/script-async.html:28
 +          layoutTestController.notifyDone();
If you're going to call notifyDone in the onload handler anyway, you don't need to waitUntilDone().  The default end of the test is after the onload handler finishes.

LayoutTests/http/tests/local/script-async.html:32
 +  <script src="http://127.0.0.1:8000/local/resources/slow-async-script.cgi" async="ASYNC"></script>
Do we want to test some syntactic variations of these attributes?

LayoutTests/http/tests/local/script-defer.html:20
 +      if (window.status_ != expected)
If there's just one expected result, we can just dump out the result and manage the expectations in the -expected.txt file, but it doesn't really matter.

LayoutTests/ChangeLog:12
 +          * http/tests/local/resources/slow-defer-script.cgi: Added.
Why are these tests in the "local" directory?  They don't seem to interact with the file system (which is why things go in local).

WebCore/ChangeLog:9
 +          the legacy parser.
Would be nice to have more information in the ChangeLog for future developers trying to understand why you did what you did.

WebCore/dom/ScriptElement.cpp:314
 +      return !m_scriptElement->sourceAttributeValue().isEmpty() && m_scriptElement->asyncAttributeValue();
Can you add a link to the spec where this is defined?  It's not obvious to me that this is the correct resolution when an inline script has an async attribute.

WebCore/html/HTML5ScriptRunner.cpp:148
 +      m_scriptsToExecuteAfterParsing.removeFirst();
There isn't a takeFirst method that does both of these?  Maybe we should add one?

WebCore/html/HTML5ScriptRunner.cpp:151
 +          return executeScriptsWaitingForParsing();
Will this recursion explode if there are thousands of these scripts?  Maybe a looping construct is better?  Can running the script cause re-entrancy here we need to worry about?

WebCore/dom/ScriptElement.cpp:320
 +      return !m_scriptElement->sourceAttributeValue().isEmpty() && !m_scriptElement->asyncAttributeValue() && m_scriptElement->deferAttributeValue();
We need a test for this logic.

WebCore/dom/ScriptElement.cpp:314
 +      return !m_scriptElement->sourceAttributeValue().isEmpty() && m_scriptElement->asyncAttributeValue();
Also, we need a test for this logic.

WebCore/html/HTML5ScriptRunner.cpp:147
 +      m_parsingBlockingScript = m_scriptsToExecuteAfterParsing.first();
How can a script be blocking parsing if we're done parsing?

WebCore/html/HTML5ScriptRunner.cpp:238
 +      PendingScript pendingScript = PendingScript();
No need to call the default constructor explicitly.

WebCore/html/HTML5ScriptRunner.cpp:239
 +      requestScript(&pendingScript, scriptElement);
If pendingScript is a value type, we should pass it by non-const reference.  If pendingScript is an output parameter, we should lass it last.

WebCore/html/HTML5ScriptRunner.cpp:253
 +          // Asynchronous scripts are not handled by the script runner because they do not block parsing.
But deferred scripts do block parsing?

WebCore/html/HTML5Tokenizer.cpp:91
 +      if (shouldContinueParsing)
Do we need a not here?  If we should continue parsing, then we don't want to finish the tree builder.  If this branch goes the other way, who will call finish on the tree builder?

WebCore/html/HTMLScriptElement.cpp:164
 +      return deferAttributeValue();
Why add this virtual method call?  Maybe have the virtual one call the non-virtual one?

WebCore/html/HTMLScriptElement.cpp:159
 +      setAttribute(asyncAttr, async ? "" : 0);
Please add a test for this.

[Eric Seidel (no email)]

I recommend we split this into two patches.

1.  Which just adds the "async" and "defer" parsing and idl changes.  We add some tests which tests parting variations of the modes and which one trumps which (assuming that's exposed to JS at the property level).

2.  The patch which actually does the scheduling.  The scheduling parts are slightly tricky since HTML5ScriptRunner is still changing.

It will be simple to get patch #1 reviewed by anyone.  Patch #2 you and I can work through over the next couple days?

I think it's OK to put async scripts on the document.  We could also make the ScriptRunner out-live the tokenizer (be owned by the Document or something else instead).  But I think it's fine to put the logic on the document.  We could make a new class to handle holding the async scripts, unsure.

I'm not when exactly async scripts get executed.  The spec says:
If the async attribute is present, then the script will be executed asynchronously, as soon as it is available.

That seems to me that it should be executed as soon as stylesheets are ready but possibly before parsing is completed, no?  I guess since they may execute after the load event they can't be executed by the ScriptRunner exclusively (since I think the tokenizer is deleted before the load event).  But it seems like async scripts will have trouble with our current document.write implementation since it (wrongly) assumes that the only thing executing scripts during parsing is the HTML5ScriptRunner.

I think this is a good change, but we should do it in smaller pieces.  Then there is less possible discussion and risk with each piece.

[Tony Gentilcore]

(In reply to comment #25)
> I recommend we split this into two patches.

Good idea.

> 
> 1.  Which just adds the "async" and "defer" parsing and idl changes.  We add some tests which tests parting variations of the modes and which one trumps which (assuming that's exposed to JS at the property level).

Sounds good, I'll mail this soon with tests in fast/dom (let me know if they should live somewhere else).

> 
> 2.  The patch which actually does the scheduling.  The scheduling parts are slightly tricky since HTML5ScriptRunner is still changing.
> 
> It will be simple to get patch #1 reviewed by anyone.  Patch #2 you and I can work through over the next couple days?
> 
> I think it's OK to put async scripts on the document.  We could also make the ScriptRunner out-live the tokenizer (be owned by the Document or something else instead).  But I think it's fine to put the logic on the document.  We could make a new class to handle holding the async scripts, unsure.

I think the cleanest approach would be to make ScriptRunner live longer, but that is a harder change and might be easier once the old parser is out of the picture. Perhaps I should add a FIXME comment in the Document mentioning that.

> 
> I'm not when exactly async scripts get executed.  The spec says:
> If the async attribute is present, then the script will be executed asynchronously, as soon as it is available.

That is defined here:
http://dev.w3.org/html5/spec/Overview.html#the-end

Unfortunately, we don't have one area that implements "the end", that is spread across parser/tokenizer, document and frameloader.

> 
> That seems to me that it should be executed as soon as stylesheets are ready but possibly before parsing is completed, no?  I guess since they may execute after the load event they can't be executed by the ScriptRunner exclusively (since I think the tokenizer is deleted before the load event).  But it seems like async scripts will have trouble with our current document.write implementation since it (wrongly) assumes that the only thing executing scripts during parsing is the HTML5ScriptRunner.
> 
> I think this is a good change, but we should do it in smaller pieces.  Then there is less possible discussion and risk with each piece.

Adam, I'll address your comments in #2 after #1 is landed.