Bug 45310 – Support <script async> as specified by HTML5

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Gentilcore]

Support <script async> as specified by HTML5

[Tony Gentilcore]

Created attachment 66741 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 66741 [details]
Patch

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

Looks fantastic.  Please figure out the non-async blocking onload or not before landing.

> WebCore/dom/AsyncScriptRunner.cpp:88
> +    if (Frame* frame = m_document->frame())
> +        frame->loader()->checkCompleted();
You should add your comment from the changelog here.  It's not immediately obvious why we do this.

> WebCore/loader/FrameLoader.cpp:841
> +    if (m_frame->document()->asyncScriptRunner()->hasPendingScripts())
Will this return true for non-asyc, non-parser inserted scripts?  i.e. an inline <script> element inserting a non-async script tag.  That shouldn't delay onload, should it? Do we have a test for that?

[Tony Gentilcore]

> You should add your comment from the changelog here.  It's not immediately obvious why we do this.
Done.

> Will this return true for non-asyc, non-parser inserted scripts?  i.e. an inline <script> element inserting a non-async script tag.  That shouldn't delay onload, should it? Do we have a test for that?

That is a really good question. This patch would make those dynamically inserted scripts block the load event.

I wrote a quick test that loads a slow script by appending it to the head from an inline script and seeing if it executes before the body's onload handler. Here are the results:

           Blocks load
Chrome 6:  NO
FF3.6:     YES
FF4:       YES
IE8:       NO
IE9:       NO
OP10.5:    YES
Safari 5:  NO

There is no strong consensus about the behavior :-(

However, it looks to me like the HTML5 spec requires that it block the load event. In http://www.whatwg.org/specs/web-apps/current-work/#script step #9, the script is added to the "set of scripts that will execute as soon as possible" whether it is actually marked "async" by the parser or whether it just wasn't parser inserted. Then http://www.whatwg.org/specs/web-apps/current-work/#the-end step #5 causes us to block the load event until all scripts that will execute as soon as possible are complete.

This change actually does break the readystate test that I added in https://bugs.webkit.org/show_bug.cgi?id=45119 (but I didn't notice it before uploading the patch). It makes it behave the same as FF4 as mentioned in https://bugs.webkit.org/show_bug.cgi?id=45119#c10.

So options are:
1. HTML5 (leave the code as-is, update the readystate expectations)
2. Legacy (add a special case to the code so that only explicitly marked async scripts block load, leave expectations as-is).

My preference is #1, and we can write #2 if #1 causes compat problems. What do you think?

[Tony Gentilcore]

Created attachment 66780 [details]
Update readystate expectations. Use increment/decrementLoadEventDelayCount()

[Tony Gentilcore]

> Created an attachment (id=66780) [details]
> Update readystate expectations. Use increment/decrementLoadEventDelayCount()

I updated the patch for #1 and discovered the increment/decrementLoadEventDelayCount which simplifies the change a bit.

[Adam Barth]

Comment on attachment 66780 [details]
Update readystate expectations. Use increment/decrementLoadEventDelayCount()

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

Looks good to me.  It would be good for Eric to look at this change too before landing.

> WebCore/dom/AsyncScriptRunner.cpp:46
> +        m_scriptsToExecuteSoon[i].first->element()->deref(); // Balances ref() in executeScriptSoon().
Manual ref counting is sadness.  Is there no way to do this with RefPtr?

[Tony Gentilcore]

> > +        m_scriptsToExecuteSoon[i].first->element()->deref(); // Balances ref() in executeScriptSoon().
> Manual ref counting is sadness.  Is there no way to do this with RefPtr?

Totally agree. I factored this code out of Document and left it exactly as-is during the refactor. My plan is to refactor this and ScriptElementData a bit so that it can use PendingScripts instead.

I'll wait for Eric before landing.

[Eric Seidel (no email)]

Comment on attachment 66780 [details]
Update readystate expectations. Use increment/decrementLoadEventDelayCount()

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

LGTM!

> WebCore/dom/AsyncScriptRunner.cpp:60
>      m_scriptsToExecuteSoon.append(make_pair(data, cachedScript));
Clearly make_pair should instead be a class, which does the load event increment and ref counting for you.  But that can be a second patch.


You should consider implementing for= event= support for <script> if you've been enjoying this.  That should be an easy fix and will fix some IE-only sites.  (comedycentral.com used to depend on for= event= support for instance)

[Tony Gentilcore]

> Clearly make_pair should instead be a class, which does the load event increment and ref counting for you.  But that can be a second patch.
Yep. Like I mentioned to abarth, I think the right thing to do here is to use PendingScript. Definitely a separate patch.

> You should consider implementing for= event= support for <script> if you've been enjoying this.  That should be an easy fix and will fix some IE-only sites.  (comedycentral.com used to depend on for= event= support for instance)
Thanks. I'll think about those some more, but I'm not really wild about the patterns they encourage and they are listed as non-conforming features. Other than possible compat issues w/ IE-specific sites, do you have any arguments for why they should exist?

[WebKit Commit Bot]

Comment on attachment 66780 [details]
Update readystate expectations. Use increment/decrementLoadEventDelayCount()

Rejecting patch 66780 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20949 test cases.
http/tests/misc/script-async.html -> failed

Exiting early after 1 failures. 20193 tests run.
490.79s total testing time

20192 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
26 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/3948330

[Tony Gentilcore]

Created attachment 67040 [details]
Patch for landing

[Tony Gentilcore]

> > You should consider implementing for= event= support for <script> if you've been enjoying this.  That should be an easy fix and will fix some IE-only sites.  (comedycentral.com used to depend on for= event= support for instance)
> Thanks. I'll think about those some more, but I'm not really wild about the patterns they encourage and they are listed as non-conforming features. Other than possible compat issues w/ IE-specific sites, do you have any arguments for why they should exist?

I caught up w/ Adam about this. He explained to me that HTML5 doesn't suggest these attributes to be implemented the way that IE implements them, it suggests neutering them. That seems like a reasonable way to have sites that use them degrade more gracefully without polluting the web platform. I'll put a patch together.

[WebKit Commit Bot]

Comment on attachment 67040 [details]
Patch for landing

Clearing flags on attachment: 67040

Committed r67163: <http://trac.webkit.org/changeset/67163>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.