Bug 40934 – Support <script defer> as specified by HTML5

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

[Tony Gentilcore]

Support <script defer> as specified by HTML5

[Tony Gentilcore]

Created attachment 59276 [details]
Patch

[Tony Gentilcore]

Adam, hopefully I've addressed all your original comments from my big patch in:
https://bugs.webkit.org/show_bug.cgi?id=20710

This patch only does defer so it is much more manageable. I'll mail one for async separately.

[Adam Barth]

Comment on attachment 59276 [details]
Patch

Eric should review this one.

[Eric Seidel (no email)]

Comment on attachment 59276 [details]
Patch

First of all, thank you! It's wonderful to have you working on this!

This is a complicated patch.  It may take me a couple passes to review it, but I'll go through it once with comments now.

LayoutTests/http/tests/loading/resources/defer-script.js:1
 +  window.status += " defer ";
I think I probably would have used some other logging mechanism. :)

LayoutTests/http/tests/loading/script-defer-expected.txt:6
 +  main frame - didFinishLoadForFrame
I'm not sure you want to have this in the loading directory, I don't think you need/want these callbacks.  Maybe you do, but it makes the tests possibly more complicated than needed.

LayoutTests/http/tests/loading/script-defer.html:8
 +    window.status += (' DCL ')
function log(text) {
   document.getElementById("preTag").innerHTML += text + "\n";
}
is the easy hack for logging.  It's also just as easy to create text nodes and br nodes in a normal div, like the "console" div and log() function for script-tests do.

LayoutTests/http/tests/loading/script-defer.html:17
 +      var results = "PASS";
You can actually use the shouldBe, log, etc. from script-tests w/o being a template-based script-test by including fast/js/js-test-pre.js.  I think that's exposed in some manner to http tests, but I could be wrong.

Honestly all the window.status stuff is fine. :)  It was just a little strange at first to read.

WebCore/html/HTML5DocumentParser.cpp:306
 +      m_endWasDelayed = !m_scriptRunner->executeScriptsWaitingForParsing();
This seems very strange.  You mean we'd end up calling end() twice?  Since we delayed the end inside end?

WebCore/html/HTML5DocumentParser.cpp:306
 +      m_endWasDelayed = !m_scriptRunner->executeScriptsWaitingForParsing();
Maybe we should have a wrapper function for calling end instead?  runAnyDeferredScriptsAndEnd() or similar?

WebCore/html/HTML5ScriptRunner.cpp:233
 +              watchForLoad(m_parsingBlockingScript);
Feels very strange to me to pull this watchForLoad call out from right next to where we make the request.  Seems like it would be easy to end up watching twice or not at all.  If we fail to "watch" a script, it will end up with no clients and can end up purging its data from the cache any time we hit the run-loop.

WebCore/html/HTML5ScriptRunner.cpp:241
 +  void HTML5ScriptRunner::requestScript(PendingScript& pendingScript, Element* scriptElement)
I think this is a great change.  It is definitely better to pass in the m_parsingBlockingScript as a reference than to use it directly!

WebCore/html/HTML5ScriptRunner.cpp:279
 +      m_scriptsToExecuteAfterParsing.append(pendingScript);
I think the lack of watchForLoad is going to bite us here.  I think instead the load function needs to know to ignore the notification.  That will be slightly sad as we may have to remove more ASSERTS (which are the only thing which makes this sort of async programming possible).

WebCore/html/HTML5ScriptRunner.cpp:294
 +              requestDeferredScript(script);
This is nice and clean. :)

WebCore/html/HTML5ScriptRunner.h:114
 +      Deque<PendingScript> m_scriptsToExecuteAfterParsing;
I think this design seems totally workable.  I wonder if we want the ScriptRunner to be in charge of running defer scripts, or should something else like the Document do so (since it happens after parsing is done?)  How are async scripts going to wire in?  Do those need to be run by the ScriptRunner or Document or something else?

We may need to split the scripting logic out of Document ont something else.  Maybe the ScriptRunner should be owned by the document directly and just used by the parser?  Or maybe a new ScriptRunner-type-class needs to exist for the Documetns' script running needs?

Those big design questions don't need to be answered by this patch, but should enter into your thinking in terms of planning async and defer. :)

r- because I think this will end up with defer scripts being purged from the cache before being run (thus causing flakiness and ASSERTs in debug mode).

[Tony Gentilcore]

(In reply to comment #4)
> (From update of attachment 59276 [details])
> First of all, thank you! It's wonderful to have you working on this!
> 
> This is a complicated patch.  It may take me a couple passes to review it, but I'll go through it once with comments now.

Thanks for the quick and detailed review. I have a few questions inline below before I update the patch.

> 
> LayoutTests/http/tests/loading/resources/defer-script.js:1
>  +  window.status += " defer ";
> I think I probably would have used some other logging mechanism. :)

I'll switch to what you suggested below.

> 
> LayoutTests/http/tests/loading/script-defer-expected.txt:6
>  +  main frame - didFinishLoadForFrame
> I'm not sure you want to have this in the loading directory, I don't think you need/want these callbacks.  Maybe you do, but it makes the tests possibly more complicated than needed.
> 

Which directory should these go in? Maybe http/tests/misc/?

> LayoutTests/http/tests/loading/script-defer.html:8
>  +    window.status += (' DCL ')
> function log(text) {
>    document.getElementById("preTag").innerHTML += text + "\n";
> }
> is the easy hack for logging.  It's also just as easy to create text nodes and br nodes in a normal div, like the "console" div and log() function for script-tests do.
> 

Good idea I'll do that.

> LayoutTests/http/tests/loading/script-defer.html:17
>  +      var results = "PASS";
> You can actually use the shouldBe, log, etc. from script-tests w/o being a template-based script-test by including fast/js/js-test-pre.js.  I think that's exposed in some manner to http tests, but I could be wrong.
> 
> Honestly all the window.status stuff is fine. :)  It was just a little strange at first to read.

I like your suggestion, will switch to that instead.

> 
> WebCore/html/HTML5DocumentParser.cpp:306
>  +      m_endWasDelayed = !m_scriptRunner->executeScriptsWaitingForParsing();
> This seems very strange.  You mean we'd end up calling end() twice?  Since we delayed the end inside end?

Yes, with this approach we'd call end() twice. The root problem is that we don't have one method that implements:
http://dev.w3.org/html5/spec/Overview.html#the-end

It is currently spread across Document, DocumentParser, TreeBuilder, and FrameLoader.

The key bit I'm trying to add is 8.2.6.3. It could either go here, in DocumentParser, or it could go in Document as I originally coded it in https://bugs.webkit.org/attachment.cgi?id=56542&action=prettypatch.

> 
> WebCore/html/HTML5DocumentParser.cpp:306
>  +      m_endWasDelayed = !m_scriptRunner->executeScriptsWaitingForParsing();
> Maybe we should have a wrapper function for calling end instead?  runAnyDeferredScriptsAndEnd() or similar?

Yes, I can do that. Does that address your above concern about calling end() twice?

> 
> WebCore/html/HTML5ScriptRunner.cpp:233
>  +              watchForLoad(m_parsingBlockingScript);
> Feels very strange to me to pull this watchForLoad call out from right next to where we make the request.  Seems like it would be easy to end up watching twice or not at all.  If we fail to "watch" a script, it will end up with no clients and can end up purging its data from the cache any time we hit the run-loop.

I believe PendingScript makes this safe. It holds a CachedResourceHandle which guarantees that it will not be purged even after it has loaded with no watcher. Also, we can't watch twice because of the ASSERT(!pendingScript.watchingForLoad()) in watchForLoad(). Let me know if you think I'm missing something.

Perhaps you can suggest a test case I can add that would prove this case?

> 
> WebCore/html/HTML5ScriptRunner.cpp:241
>  +  void HTML5ScriptRunner::requestScript(PendingScript& pendingScript, Element* scriptElement)
> I think this is a great change.  It is definitely better to pass in the m_parsingBlockingScript as a reference than to use it directly!

Would it be worthwhile to break this out into a separate patch to reduce the size of this one?

> 
> WebCore/html/HTML5ScriptRunner.cpp:279
>  +      m_scriptsToExecuteAfterParsing.append(pendingScript);
> I think the lack of watchForLoad is going to bite us here.  I think instead the load function needs to know to ignore the notification.  That will be slightly sad as we may have to remove more ASSERTS (which are the only thing which makes this sort of async programming possible).

Yes, I don't want to remove those ASSERTs. I believe that the PendingScript's CachedResourceHandle prevents us from having to worry about this. If I missed something, then I probably need to figure out a way to have a different host that can be notified when deferred scripts load. Perhaps a separate DeferredScriptRunnerHost.

> 
> WebCore/html/HTML5ScriptRunner.cpp:294
>  +              requestDeferredScript(script);
> This is nice and clean. :)
> 
> WebCore/html/HTML5ScriptRunner.h:114
>  +      Deque<PendingScript> m_scriptsToExecuteAfterParsing;
> I think this design seems totally workable.  I wonder if we want the ScriptRunner to be in charge of running defer scripts, or should something else like the Document do so (since it happens after parsing is done?)  How are async scripts going to wire in?  Do those need to be run by the ScriptRunner or Document or something else?

Async scripts can be run after the DocumentParser is destroyed. So I was planning to track the number of outstanding async scripts on the Document. Similar to this:
https://bugs.webkit.org/attachment.cgi?id=57565&action=prettypatch

I could track both deferred and async scripts on the document, but you initially didn't like the idea of having code on the document that isn't used after it is loaded.

> 
> We may need to split the scripting logic out of Document ont something else.  Maybe the ScriptRunner should be owned by the document directly and just used by the parser?  Or maybe a new ScriptRunner-type-class needs to exist for the Documetns' script running needs?

Letting FrameLoader or Document own an AsyncScriptRunner and DeferredScriptRunner and DocumentParser own a SyncScriptRunner would be one way to go. It is hard for me to tell if the additional classes are warranted just yet.

> 
> Those big design questions don't need to be answered by this patch, but should enter into your thinking in terms of planning async and defer. :)
> 
> r- because I think this will end up with defer scripts being purged from the cache before being run (thus causing flakiness and ASSERTs in debug mode).

[Eric Seidel (no email)]

(In reply to comment #5)
> Which directory should these go in? Maybe http/tests/misc/?

If I remember correctly the "loading" directory may automatically get these callbacks turned on (which is a horrible design IMO).  If you want the callbacks then sure, leave them there.  Otherwise anywhere you like is fine.  Maybe http/tests/local/HTMLScriptElement  Since we don't actually need to load the main page off the network.  I'm not sure if local/ is reserved for a special purpose or if it's just for tests which don't actually need to be sent over HTTP, just use http resources.

> I believe PendingScript makes this safe. It holds a CachedResourceHandle which guarantees that it will not be purged even after it has loaded with no watcher. Also, we can't watch twice because of the ASSERT(!pendingScript.watchingForLoad()) in watchForLoad(). Let me know if you think I'm missing something.

Nope!  That's what I thought too.  See http://trac.webkit.org/changeset/61374 for more context.  CachedResource will mark itself purgeable if it has no clients.  CachedResourceHandle seems to just keep the actual CachedResource alive, but that doesn't mean it will still have any data attached to it. :)

> Perhaps you can suggest a test case I can add that would prove this case?

Get a DEFER to finish loading, but not run yet due to the parser being blocked on something else.
Load a bunch of data or somehow convince the cache to start purging stuff.
Try and run the script, it will have been marked purged and thus have no data (which will do nothing in Release mode, or hit an ASSERT inside ScriptSource in debug mode.)

> > WebCore/html/HTML5ScriptRunner.cpp:241
> >  +  void HTML5ScriptRunner::requestScript(PendingScript& pendingScript, Element* scriptElement)
> > I think this is a great change.  It is definitely better to pass in the m_parsingBlockingScript as a reference than to use it directly!
> 
> Would it be worthwhile to break this out into a separate patch to reduce the size of this one?

Totally up to you.  I'm happy to r=me that change if you want to post it separately.  This isn't a very big change, just complicated due to the end() symantecs which are delicate (and hence guarded by a zillion ASSERTS).

> Yes, I don't want to remove those ASSERTs. I believe that the PendingScript's CachedResourceHandle prevents us from having to worry about this. If I missed something, then I probably need to figure out a way to have a different host that can be notified when deferred scripts load. Perhaps a separate DeferredScriptRunnerHost.

Well, it would be easy to have some separate thing signed up as the client.  Even something which implemented an empty notifyFinished().

> I could track both deferred and async scripts on the document, but you initially didn't like the idea of having code on the document that isn't used after it is loaded.

Yup.  I worry our poor Document class is a bit of a dumping ground.  I still need to go back and read the spec on DEFER vs. ASYNC.

[Tony Gentilcore]

Created attachment 60047 [details]
Patch

[Tony Gentilcore]

I believe this addresses all of your previous comments. Should be ready for another pass.

[Eric Seidel (no email)]

Attachment 60047 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3352240

[Tony Gentilcore]

So odd that VisualStudio doesn't warn/error about initialization order. I'll upload a fixed patch shortly.

[Eric Seidel (no email)]

Comment on attachment 60047 [details]
Patch

End should be final.  How are we getting past the first end check to need a second?

Are you sure you need that code or does:

    bool shouldDelayEnd() const { return inWrite() || isWaitingForScripts() || inScriptExecution() || isScheduledForResume(); }

already catch your case?

[Eric Seidel (no email)]

Comment on attachment 60047 [details]
Patch

I'm slightly onfused why we need RefCounted PendingScript?

Wouldn't the blocking script just move off the queue  to become the pending blocking script when it's turn has come?

[Tony Gentilcore]

(In reply to comment #12)
> (From update of attachment 60047 [details])
> I'm slightly onfused why we need RefCounted PendingScript?
> 
> Wouldn't the blocking script just move off the queue  to become the pending blocking script when it's turn has come?

Yes, you are right, RefPtr functionality is not needed here, it was just convenient. The problem is that PendingScript is Noncopyable so I can't just assign it.

Here are some options:
1. Remove Noncopyable and implement the = operator. It seemed desirable to keep it non copyable, what do you think?
2. Make it an OwnPtr. Then I need a create() method that returns a PassOwnPtr and one that returns a PassRefPtr. Perhaps that isn't so bad?

What do you recommend?

[Adam Barth]

A class either inherits from RefCounted or it doesn't.  If it does, then you can't use OwnPtr.  You have to use RefPtr.  If it doesn't, you can't use RefPtr and you have to use OwnPtr (or hold it directly).  (There are some exceptions where folks implement the RefCounted API without inheriting from RefCounted, but this doesn't sound like one of them.)

Having create methods for both RefPtr and OwnPtr doesn't make any sense.

[Tony Gentilcore]

(In reply to comment #14)
> A class either inherits from RefCounted or it doesn't.  If it does, then you can't use OwnPtr.  You have to use RefPtr.  If it doesn't, you can't use RefPtr and you have to use OwnPtr (or hold it directly).  (There are some exceptions where folks implement the RefCounted API without inheriting from RefCounted, but this doesn't sound like one of them.)
> 
> Having create methods for both RefPtr and OwnPtr doesn't make any sense.

I'm kind of glad the API doesn't allow that. It would have been weird. So maybe the best option is to make PendingScript copyable and not use smart ptrs at all?

[Adam Barth]

I haven't looked at the details at all, but that could well be a good solution.

[Tony Gentilcore]

(In reply to comment #11)
> (From update of attachment 60047 [details])
> End should be final.  How are we getting past the first end check to need a second?
> 
> Are you sure you need that code or does:
> 
>     bool shouldDelayEnd() const { return inWrite() || isWaitingForScripts() || inScriptExecution() || isScheduledForResume(); }
> 
> already catch your case?

A deferred script needs to be run after parsing, but before DOMContentLoaded. See:
http://dev.w3.org/html5/spec/Overview.html#the-end

When HTMLDocumentParser::end() calls m_treeBuilder->finished(), the Document fires DOMContentLoaded. So I need to hook in before that point.

However, shouldDelayEnd() doesn't handle my case because I need to wait for that to be completely finished. For example, I couldn't call executeScriptsWaitingForParsingAndEnd() if shouldDelayEnd() would return true because a synchronous script is still loading.

This all feels weird at the moment because there is no single place that implements "#the-end". Those steps are mostly spread through Document and FrameLoader. An alternate approach is what I did in the original patch (way back when) which was to add a phase to Document.

Adding the phase here feels cleaner to me than adding it to Document. If you have a suggestion for a third alternative, I'm happy to pursue that. It would be really nice to eventually have one method that performs "#the-end".

[Tony Gentilcore]

Created attachment 60076 [details]
Patch

[Eric Seidel (no email)]

Attachment 60076 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3372026

[Tony Gentilcore]

Created attachment 60078 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 60078 [details]
Patch

WebCore/html/HTMLDocumentParser.cpp:292
 +      if (m_scriptRunner && !m_scriptRunner->executeScriptsWaitingForParsing()) {
This is kinda subtle.  I might have broken the running of m_scriptRunner->executeScriptsWaitingForParsing into a separate inner if:

if (m_scriptRunner) {
    bool continueParsing = m_scriptRunner->executeScriptsWaitingForParsing();
    if (!continueParsing) {


WebCore/html/HTMLScriptRunner.cpp:237
 +              watchForLoad(m_parsingBlockingScript);
Is this always right?  What about the case where parsing finshed, but a stylesheet hasn't loaded.  That seems like it will end up recursing under watchForLoad() because the m_parsingBlockingScript will have already loaded but can't run because we're blocked on CSS.  Or?

WebCore/html/HTMLScriptRunner.cpp:245
 +  void HTMLScriptRunner::requestScript(PendingScript& pendingScript, Element* scriptElement)
I still think this is a fantastic change to this function to make it not affect the instance, only its args.


otherwise it looks fine.  r- for the possible slow CSS problem.

[Tony Gentilcore]

Created attachment 60271 [details]
Patch

[Tony Gentilcore]

(In reply to comment #21)
> (From update of attachment 60078 [details])
> WebCore/html/HTMLDocumentParser.cpp:292
>  +      if (m_scriptRunner && !m_scriptRunner->executeScriptsWaitingForParsing()) {
> This is kinda subtle.  I might have broken the running of m_scriptRunner->executeScriptsWaitingForParsing into a separate inner if:
> 
> if (m_scriptRunner) {
>     bool continueParsing = m_scriptRunner->executeScriptsWaitingForParsing();
>     if (!continueParsing) {

Good call. That is much more readable :-)

> 
> 
> WebCore/html/HTMLScriptRunner.cpp:237
>  +              watchForLoad(m_parsingBlockingScript);
> Is this always right?  What about the case where parsing finshed, but a stylesheet hasn't loaded.  That seems like it will end up recursing under watchForLoad() because the m_parsingBlockingScript will have already loaded but can't run because we're blocked on CSS.  Or?
> 

It should be impossible to move into #the-end 8.2.6.3 phase until after all stylesheets have loaded. I've added a new LayoutTest to verify this case. Please let me know if I'm still missing something or if that isn't what you had in mind. I'd also be interested in adding more tests if you can think of other tricky scenarios.

> WebCore/html/HTMLScriptRunner.cpp:245
>  +  void HTMLScriptRunner::requestScript(PendingScript& pendingScript, Element* scriptElement)
> I still think this is a fantastic change to this function to make it not affect the instance, only its args.
> 
> 
> otherwise it looks fine.  r- for the possible slow CSS problem.

[Eric Seidel (no email)]

I think it's clearer if we ASSERT that stylesheets are done in end() instead of assuming.

[Tony Gentilcore]

(In reply to comment #24)
> I think it's clearer if we ASSERT that stylesheets are done in end() instead of assuming.

You were right! There was a bug, but it was slightly different.

It isn't valid to ASSERT that stylesheets are done in end(). Stylesheets don't block the end of parsing, they only block parser-inserted script execution. The suggested ASSERT would trigger for this document "<html><link rel=stylesheet href=slow.css></html>". Currently executeScriptsWaitingForParsing() calls executeParsingBlockingScripts() which calls executePendingScript() which performs an ASSERT(m_document->haveStylesheetsLoaded()). So we are guaranteed that we can never run a script (deferred or otherwise) if there is a pending stylesheet.

Anyway, I added another new test that writes a slow stylesheet from the first deferred script and ensures that the second deferred happens after the stylesheet is applied. It uncovered the bug. In HTMLScriptRunner::executeScriptsWaitingForParsing(), executeParsingBlockingScripts() may return false under two conditions:
1. The script isn't loaded yet
2. A blocking stylesheet isn't loaded yet
It is only valid to call watchForLoad() if it returns false for #1. So I guarded watchForLoad() with a check for #1.

[Tony Gentilcore]

Created attachment 60302 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 60302 [details]
Patch

Why can't we ASSERT right here:
33     while (!m_scriptsToExecuteAfterParsing.isEmpty()) {
 234         ASSERT(!haveParsingBlockingScript());

that stylesheets are done?

I want to make it impossible for us to think we're running the very last script only to have us block on stylesheet loads.  Or worse, have us try to run the very last script, have it block, and then call waitForLoad which imemdiately calls us back re-entrantly.

[Eric Seidel (no email)]

I guess waitForLoad() asserts that the cachedScript isn't loaded...

[Tony Gentilcore]

(In reply to comment #27)
> (From update of attachment 60302 [details])
> Why can't we ASSERT right here:
> 33     while (!m_scriptsToExecuteAfterParsing.isEmpty()) {
>  234         ASSERT(!haveParsingBlockingScript());
> 
> that stylesheets are done?

My latest test case triggers the condition where stylesheets are not done at that line (a doc.write of a stylesheet from a deferred script).

In that case, the first deferred script becomes the parsing blocking script, and executeParsingBlockingScripts()'s call to isPendingScriptReady() sets us into m_hasScritpsWaitingForStylesheets state. When the stylesheet loads, the m_parsingBlockingScript executes and the remaining deferred scripts are run (if any).

> 
> I want to make it impossible for us to think we're running the very last script only to have us block on stylesheet loads.  Or worse, have us try to run the very last script, have it block, and then call waitForLoad which imemdiately calls us back re-entrantly.

My new guard of !m_parsingBlockingScript.cachedScript()->isLoaded() should prevent this. Am I missing something still?

[Tony Gentilcore]

(In reply to comment #29)
> (In reply to comment #27)
> > (From update of attachment 60302 [details] [details])
> > Why can't we ASSERT right here:
> > 33     while (!m_scriptsToExecuteAfterParsing.isEmpty()) {
> >  234         ASSERT(!haveParsingBlockingScript());
> > 
> > that stylesheets are done?
> 
> My latest test case triggers the condition where stylesheets are not done at that line (a doc.write of a stylesheet from a deferred script).
> 
> In that case, the first deferred script becomes the parsing blocking script, and executeParsingBlockingScripts()'s call to isPendingScriptReady() sets us into m_hasScritpsWaitingForStylesheets state. When the stylesheet loads, the m_parsingBlockingScript executes and the remaining deferred scripts are run (if any).
> 
> > 
> > I want to make it impossible for us to think we're running the very last script only to have us block on stylesheet loads.  Or worse, have us try to run the very last script, have it block, and then call waitForLoad which imemdiately calls us back re-entrantly.
> 
> My new guard of !m_parsingBlockingScript.cachedScript()->isLoaded() should prevent this. Am I missing something still?

I don't want to let this drop. Does that explanation convince or do you think I'm still missing a bug?

[Eric Seidel (no email)]

Comment on attachment 60302 [details]
Patch

Totally agreed!  You shouldn't let this drop. :)  Nor should I have.WebCore/html/HTMLScriptRunner.cpp:281
 +      m_scriptsToExecuteAfterParsing.append(pendingScript);
Why don't we want to check m_parsingBlockingScript.cachedScript() here?  It seems strange to have the watchForLoad stuff outside of requestScript.

requestScript coudl return a success bool which we could use to judge if we should waitForLoad if it's not loaded or something.  requestScript could simply return a pointer to the cachedScript() too.

It seems that we don't want to put the pendingScript on the "executeAfterParsing" list if it failed to be requested?

Do we need to test when the before-load event fires for deferred scripts?  Currently it fires synchronously as they're parsed.  is that correct?


This is just complicated stuff.  Your job as the patch submitter is to convince me it's right. :)  And my job as the reviewer is to use my (hopefully helpful) knowledge of the code to understand what could go wrong.

I think this is very close, and we could probably land it as-is.  But I'm curious as to your answers to the above questions first.

[Tony Gentilcore]

Created attachment 63027 [details]
Patch

[Tony Gentilcore]

As per our offline conversation, I've added a lot of new tests. Please let me know if they happen to trigger any more ideas for tricky scenarios we could throw at it.

The code change is just that bit about returning a boolean from requestScript and only adding to the queue if a request was actually made.

[Tony Gentilcore]

Created attachment 63029 [details]
Fix tab in ChangeLog

[Eric Seidel (no email)]

Comment on attachment 63029 [details]
Fix tab in ChangeLog

LayoutTests/fast/dom/HTMLScriptElement/defer-double-defer-write.html:14
 +      onbeforeload="doubleWrite(4)"
Why does this onbeforeload run at a different time than the one in the next test?  This one seems to run after the 3, the next test it runs immediately?  Is it racey?

LayoutTests/fast/dom/HTMLScriptElement/defer-double-write.html:14
 +      onbeforeload="doubleWrite(3)"
Here is the other onbeforeload your'e expecting to run at a different time than the other one.  I'm not sure how tehse two tests are different.

Oh, I see, the first one uses a "double defer".  But I still wouldn't expect the onbeforeload time to change?LayoutTests/fast/dom/HTMLScriptElement/defer-inline-script.html:11
 +  <script defer>
Excellent.  This test coudl probably be simpler with just a little document.write(). :)  The script-testing stuff is useful, but you don't really seem to be uing it here. :)

LayoutTests/fast/dom/HTMLScriptElement/defer-onbeforeload.html:8
 +  Checks that deferred scripts fire onbeforeload immediately and that it is cancellable.
This test seems to have conflicting expectations with teh first one...

LayoutTests/fast/dom/HTMLScriptElement/defer-onbeforeload.html:10
 +  <script src="resources/shouldnotexecute.js" onbeforeload="return false;" defer></script>
Seems you'd want to log here, no?

LayoutTests/fast/dom/HTMLScriptElement/defer-script-invalid-url.html:10
 +  <script src="http://invalid:999999/" defer></script>
Do we need/want to check timing expectations of onerror?  That might be racey though.

LayoutTests/fast/dom/HTMLScriptElement/resources/shouldnotexecute.js:1
 +  debug("should not execute");
Might put FAIL in there.

LayoutTests/http/tests/misc/resources/script-debug-body-background.js:1
 +  debug('Body background: ' + getComputedStyle(document.body)['background-color']);
This should ideally contain the expectation in it.  Since you're using the full-blown js-test-pre.js harness, you could use shouldBe here or shouldBeEqualToString

LayoutTests/http/tests/misc/script-defer-write-slow-stylesheet.html:18
 +  <script src="http://127.0.0.1:8000/misc/resources/script-write-slow-stylesheet.js" defer="defer"></script>
So this is testing that writing a stylesheet from a defer script will delay DOMContentLoaded, right?

WebCore/html/HTMLScriptRunner.cpp:251
 +      AtomicString srcValue = scriptElement->getAttribute(srcAttr);
const AtomicString& srcValue should be sufficient (and prevents one uneeded ref).

WebCore/html/HTMLScriptRunner.cpp:257
 +      pendingScript.element = scriptElement;
Do we need to end up clearing this in the failure case?  I guess not, since we weren't doing so before?

WebCore/html/HTMLScriptRunner.cpp:276
 +      if (madeRequest && !m_parsingBlockingScript.cachedScript()->isLoaded())
I think I would have made the m_parsingBlockingScript.cachedScript() dependance explicit here:

if (madeRequest) {
    ASSERT(m_parsingBlockingScript.cachedScript());
    if (!m_parsingBlockingScript.cachedScript()->isLoaded())
         watchForLoad(m_parsingBlockingScript);
}

Since requestScript can return w/o a cachedScript(), we should ASSERT that the bool and cachedScript() always agree somewhere.

WebCore/html/HTMLScriptRunner.cpp:283
 +      bool madeRequest = requestScript(pendingScript, scriptElement);
This is a success bool, it seems.

I'm not sure the name "madeRequest" helps me here.  My first thought was "oh, do I need to call it again?"

Maybe "requestSuccessfull"?  Or just throw it in the if block itself w/o a local variable would be OK too.

(This is a nit, and not a big deal.)

In general this looks fine.  I think we could go one more round for nits.

[Tony Gentilcore]

(In reply to comment #35)
> (From update of attachment 63029 [details])
> LayoutTests/fast/dom/HTMLScriptElement/defer-double-defer-write.html:14
>  +      onbeforeload="doubleWrite(4)"
> Why does this onbeforeload run at a different time than the one in the next test?  This one seems to run after the 3, the next test it runs immediately?  Is it racey?

It isn't racey. The onbeforeload runs at the right time. Notice that it doesn't write any numbers, it only writes deferred scripts which don't write the numbers until later. I've added a write directly to the onbeforeload handler to make that clear and verify that it runs at the right time.

> 
> LayoutTests/fast/dom/HTMLScriptElement/defer-double-write.html:14
>  +      onbeforeload="doubleWrite(3)"
> Here is the other onbeforeload your'e expecting to run at a different time than the other one.  I'm not sure how tehse two tests are different.
> 
> Oh, I see, the first one uses a "double defer".  But I still wouldn't expect the onbeforeload time to change?

See above comment. Should be clear that the onbeforeload time is the same now.

> LayoutTests/fast/dom/HTMLScriptElement/defer-inline-script.html:11
>  +  <script defer>
> Excellent.  This test coudl probably be simpler with just a little document.write(). :)  The script-testing stuff is useful, but you don't really seem to be uing it here. :)

Done.

> 
> LayoutTests/fast/dom/HTMLScriptElement/defer-onbeforeload.html:8
>  +  Checks that deferred scripts fire onbeforeload immediately and that it is cancellable.
> This test seems to have conflicting expectations with teh first one...

See first comment.

> 
> LayoutTests/fast/dom/HTMLScriptElement/defer-onbeforeload.html:10
>  +  <script src="resources/shouldnotexecute.js" onbeforeload="return false;" defer></script>
> Seems you'd want to log here, no?

Good idea. Done.

> 
> LayoutTests/fast/dom/HTMLScriptElement/defer-script-invalid-url.html:10
>  +  <script src="http://invalid:999999/" defer></script>
> Do we need/want to check timing expectations of onerror?  That might be racey though.

There is currently a bug about onerror not being fired for invalid URLs. I have a local patch started to fix that, but don't want to complicate these expectations for now. http://crbug.com/39423

> 
> LayoutTests/fast/dom/HTMLScriptElement/resources/shouldnotexecute.js:1
>  +  debug("should not execute");
> Might put FAIL in there.

Done.

> 
> LayoutTests/http/tests/misc/resources/script-debug-body-background.js:1
>  +  debug('Body background: ' + getComputedStyle(document.body)['background-color']);
> This should ideally contain the expectation in it.  Since you're using the full-blown js-test-pre.js harness, you could use shouldBe here or shouldBeEqualToString

Done.

> 
> LayoutTests/http/tests/misc/script-defer-write-slow-stylesheet.html:18
>  +  <script src="http://127.0.0.1:8000/misc/resources/script-write-slow-stylesheet.js" defer="defer"></script>
> So this is testing that writing a stylesheet from a defer script will delay DOMContentLoaded, right?

Yes. I've updated the description to reflect that.

> 
> WebCore/html/HTMLScriptRunner.cpp:251
>  +      AtomicString srcValue = scriptElement->getAttribute(srcAttr);
> const AtomicString& srcValue should be sufficient (and prevents one uneeded ref).

Oops. Good catch. Fixed.

> 
> WebCore/html/HTMLScriptRunner.cpp:257
>  +      pendingScript.element = scriptElement;
> Do we need to end up clearing this in the failure case?  I guess not, since we weren't doing so before?

No, because it ASSERTs that it is clear in the first line of this method.

> 
> WebCore/html/HTMLScriptRunner.cpp:276
>  +      if (madeRequest && !m_parsingBlockingScript.cachedScript()->isLoaded())
> I think I would have made the m_parsingBlockingScript.cachedScript() dependance explicit here:
> 
> if (madeRequest) {
>     ASSERT(m_parsingBlockingScript.cachedScript());
>     if (!m_parsingBlockingScript.cachedScript()->isLoaded())
>          watchForLoad(m_parsingBlockingScript);
> }
> 
> Since requestScript can return w/o a cachedScript(), we should ASSERT that the bool and cachedScript() always agree somewhere.

Good call. I did that here and in requestDeferredScript().

> 
> WebCore/html/HTMLScriptRunner.cpp:283
>  +      bool madeRequest = requestScript(pendingScript, scriptElement);
> This is a success bool, it seems.
> 
> I'm not sure the name "madeRequest" helps me here.  My first thought was "oh, do I need to call it again?"
> 
> Maybe "requestSuccessfull"?  Or just throw it in the if block itself w/o a local variable would be OK too.

I got rid of the variable.

> 
> (This is a nit, and not a big deal.)
> 
> In general this looks fine.  I think we could go one more round for nits.

[Tony Gentilcore]

Created attachment 63078 [details]
Patch

[Tony Gentilcore]

> In general this looks fine.  I think we could go one more round for nits.

Do you want to do one more round or should I take the previous r+?

[Eric Seidel (no email)]

Comment on attachment 63078 [details]
Patch

LGTM!

[WebKit Commit Bot]

Comment on attachment 63078 [details]
Patch

Rejecting patch 63078 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--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 20784 test cases.
fast/dom/HTMLScriptElement/defer-script-invalid-url.html -> failed

Exiting early after 1 failures. 6694 tests run.
132.45s total testing time

6693 test cases (99%) succeeded
1 test case (<1%) had incorrect layout

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

[Tony Gentilcore]

Committed r64674: <http://trac.webkit.org/changeset/64674>

[Tony Gentilcore]

> fast/dom/HTMLScriptElement/defer-script-invalid-url.html -> failed

This test was working as intended but displaying a "blocked external resource request" message at the top.

I changed http://invalid:99999 to http://localhost:99999 and landed.

[WebKit Review Bot]

http://trac.webkit.org/changeset/64674 might have broken GTK Linux 64-bit Debug

[Tony Gentilcore]

Rolled back due to incompatibility with HTML5 tree builder which was enabled last night.

I'm going to rework this so that the insertion point is undefined when deferred scripts are run (which means no doc.write from deferred scripts). That is the behavior specced by HTML5 and it should be fine as IE's original definition of defer was basically a contract that the script won't do any doc.writes.

[Tony Gentilcore]

Updating with a quick survey of document.write in deferred script behavior:

IE5-IE9: Blow away document
FF3.6: Blow away document
Minefield: Drop document.write

The HTML5 spec, says to set the insertion point to undefined before running deferred scripts, but it isn't immediately clear to me whether that means writes should blow away the doc or be dropped. I'm reading more carefully now.

[Tony Gentilcore]

Created attachment 63968 [details]
Work in progress

[Tony Gentilcore]

Created attachment 64600 [details]
Testcase

This is the test case I had in mind.

First close() detaches, then write() implicitly reopens the document.

In the previous attempts, we bailed out of pumpTokenizer() after the first token, so I'd expect this would have resulted in a document with only "<p>" (but haven't verified).

Your patch should continue the loop through all tokens and result in a document with "<p>1<p>2".

This also means your comment that the parser doesn't touch webcore after detach isn't strictly true.

[Tony Gentilcore]

(In reply to comment #47)
> Created an attachment (id=64600) [details]
> Testcase

Oops, wrong tab == wrong bug :-(

[Tony Gentilcore]

Created attachment 66066 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 66066 [details]
Patch

> WebCore/html/parser/HTMLDocumentParser.cpp:346
> +    stop();
I'm not sure stop() is the right name here.  Since it doesn't set m_parserStopped.

> WebCore/html/parser/HTMLDocumentParser.h:137
> +    bool m_active; // http://www.whatwg.org/specs/web-apps/current-work/#active-parser
Why not just use m_parserStopped?

[Tony Gentilcore]

(In reply to comment #50)
> (From update of attachment 66066 [details])
> > WebCore/html/parser/HTMLDocumentParser.cpp:346
> > +    stop();
> I'm not sure stop() is the right name here.  Since it doesn't set m_parserStopped.

Very good point. More details below, but I'm trying out prepareToEnd().

> 
> > WebCore/html/parser/HTMLDocumentParser.h:137
> > +    bool m_active; // http://www.whatwg.org/specs/web-apps/current-work/#active-parser
> Why not just use m_parserStopped?

m_parserStopped is set too late -- closer to the end of 10.2.6 #the-end. When it is set, it does things like prevent further inserts/appends which isn't correct when running deferred scripts. I need something that indicates we're in the deferred scripts phase so that notifiyFinished() can direct the load and inScriptExecution() won't prevent implicit open()s. So I've changed it to m_runningDeferredScripts, please let me know what you think.

[Tony Gentilcore]

Created attachment 66099 [details]
Naming fixes

[Eric Seidel (no email)]

OK.  Seems we're building a state machine with bools.

What about this:

enum ParserState {
   InitialState,
   ParsingState,
   RunningDeferredScriptsState,
   StoppedState,
   DetachedState,
};

That's an approximation, but maybe the idea is sound?

[Tony Gentilcore]

(In reply to comment #53)
> OK.  Seems we're building a state machine with bools.
> 
> What about this:
> 
> enum ParserState {
>    InitialState,
>    ParsingState,
>    RunningDeferredScriptsState,
>    StoppedState,
>    DetachedState,
> };
> 
> That's an approximation, but maybe the idea is sound?

Yeah, that sounds very reasonable. Before I go down that path, just wanted to bounce one alternative by you.

The state will need to be tracked on the Document's readyState as well. It currently has an m_bParsing attribute and a really shady implementation of readyState(). I could instead do this on Document:

enum ReadyState {
  Loading,
  Interactive,
  Complete
}

Then DocumentParser could use m_document->readyState() == Interactive as RunningDeferredScriptsState and everything else would be untouched.

[Eric Seidel (no email)]

I don't know what that would look like, but sounds sane.

I think we eventually need a state machine in the parser too, or maybe they're the same thing.  it's unclear to me.

[Tony Gentilcore]

(In reply to comment #55)
> I don't know what that would look like, but sounds sane.
> 
> I think we eventually need a state machine in the parser too, or maybe they're the same thing.  it's unclear to me.

OK. I'll explore and see which one turns out cleaner.

As another point, it would be kind of odd to have a RunningDeferredScriptsState in DocumentParser when not all subclasses would utilize that state.

[Tony Gentilcore]

Created attachment 66157 [details]
Use enum for parser state

[Early Warning System Bot]

Attachment 66157 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3917003

[Tony Gentilcore]

Created attachment 66162 [details]
Fix Qt build

[Adam Barth]

Comment on attachment 66162 [details]
Fix Qt build

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

This looks great.  A few minor comments below.  The bitfield one is the main thing.

> WebCore/ChangeLog:16
> [phone]  Tony Gentilcore  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Support <script defer> as specified by HTML5
> +        https://bugs.webkit.org/show_bug.cgi?id=40934
> +
> +        Tests: fast/dom/HTMLScriptElement/defer-double-defer-write.html
> +               fast/dom/HTMLScriptElement/defer-double-write.html
> +               fast/dom/HTMLScriptElement/defer-inline-script.html
> +               fast/dom/HTMLScriptElement/defer-onbeforeload.html
> +               fast/dom/HTMLScriptElement/defer-script-invalid-url.html
> +               fast/dom/HTMLScriptElement/defer-write.html
> +               fast/dom/HTMLScriptElement/two-defer-writes.html
> +               http/tests/misc/script-defer-after-slow-stylesheet.html
> +               http/tests/misc/script-defer.html
So much boilerplate in the ChangeLog but no actual text.  :)

> WebCore/dom/DocumentParser.h:69
> +    bool isParsing() const { return m_state == ParsingState; }
> +    bool isActive() const { return m_state & (ParsingState | StoppingState); }
> +    bool isStopping() const { return m_state == StoppingState; }
> +    bool isDetached() const { return m_state == DetachedState; }
Can you ASSERT m_document (as appropriate) in these states?

> WebCore/dom/DocumentParser.h:100
> +    enum ParserState {
> +        ParsingState = 0x1,
> +        StoppingState = 0x2,
> +        StoppedState = 0x4,
> +        DetachedState = 0x8
> +    };
Woah.  Do we really need a bit field here?  It seems like an enum should be enough.

[Tony Gentilcore]

> So much boilerplate in the ChangeLog but no actual text.  :)
Sorry, I keep losing my comments with all the iterations here. I'll add them back before landing.

> Can you ASSERT m_document (as appropriate) in these states?
Unfortunately that wouldn't be very clean as these are just checking the state so they may be called at any time. I'd have to do something like ASSERT(m_state == ParsingState || m_state == StoppingState || m_state == StoppedState || !m_document); Is that what you have in mind?
 
> Woah.  Do we really need a bit field here?  It seems like an enum should be enough.
I thought it would be more efficient, but it looks like the compiler is smart enough to optimize away the two states in isActive(). So I've removed the bitfield.

Thanks for the review. Will land in the morning.

[Adam Barth]

> > Woah.  Do we really need a bit field here?  It seems like an enum should be enough.
> I thought it would be more efficient, but it looks like the compiler is smart enough to optimize away the two states in isActive(). So I've removed the bitfield.

Thanks.

[Eric Seidel (no email)]

Comment on attachment 66162 [details]
Fix Qt build

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

> WebCore/dom/Document.cpp:1827
> +        if (m_frame->loader()->isLoadingMainResource() || (parser && parser->isParsing() && parser->isExecutingScript()))
Why the extra check needed here?

> WebCore/dom/DocumentParser.h:61
>      virtual bool processingData() const { return false; }
We should just remove this now since clearly it doesn't work anymore...  (A matter for a separate bug.)

> WebCore/dom/DocumentParser.h:67
> +    bool isActive() const { return m_state & (ParsingState | StoppingState); }
I would have used a <  I thought you moved off of bitfields?

> WebCore/dom/DocumentParser.h:100
> +    };
Ah, I see, not yet moved off of a bitfield in this patch.

> WebCore/dom/XMLDocumentParser.cpp:135
> +    if (!isActive() || m_sawXSLTransform)
!isActive() seems a strange check.  why not just isStopped() and have "stopped" check m_state >= Stopped?

> WebCore/html/parser/HTMLDocumentParser.cpp:163
> +    if (m_scriptRunner) {
> +        if (!m_scriptRunner->executeScriptsWaitingForParsing())
Why not combine these into one line?

!isActive() feels a little strange.  Thanks again for all your work on this!

[Tony Gentilcore]

> > +        if (m_frame->loader()->isLoadingMainResource() || (parser && parser->isParsing() && parser->isExecutingScript()))
> Why the extra check needed here?
When this condition is satisfied, the method bails out early thus preventing an implicit open from a write() in a deferred script. In an earlier version of the patch, I had added the check to isExecutingScript() so that it only returns true when executing a parsing-blocking script, not a deferred script. But now that the method is accessible on parser, this seems cleaner. Let me know if you want it moved.

> >      virtual bool processingData() const { return false; }
> We should just remove this now since clearly it doesn't work anymore...  (A matter for a separate bug.)
Yep, I can put together a subsequent patch.

> > +    bool isActive() const { return m_state & (ParsingState | StoppingState); }
> I would have used a <  I thought you moved off of bitfields?
Done.

> > +    if (!isActive() || m_sawXSLTransform)
> !isActive() seems a strange check.  why not just isStopped() and have "stopped" check m_state >= Stopped?
Good call. Done.

> > +    if (m_scriptRunner) {
> > +        if (!m_scriptRunner->executeScriptsWaitingForParsing())
> Why not combine these into one line?
That was the original. You asked to break it up in Comment #21. Going back to one line now ;-)

[Tony Gentilcore]

Created attachment 66219 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 66219 [details]
Patch for landing

Rejecting patch 66219 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 20902 test cases.
fast/dom/HTMLScriptElement/defer-inline-script.html -> failed

Exiting early after 1 failures. 6743 tests run.
164.54s total testing time

6742 test cases (99%) succeeded
1 test case (<1%) had incorrect layout

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

[Tony Gentilcore]

Created attachment 66281 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 66281 [details]
Patch for landing

Clearing flags on attachment: 66281

Committed r66649: <http://trac.webkit.org/changeset/66649>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66649 might have broken Qt Linux Release

[Csaba Osztrogonác]

Reopened, because it was rolled out by http://trac.webkit.org/changeset/66660

Qt bot got the following (or similar) error for ~2000 tests:
"This page contains the following errors:
error on line 1 at column 2: 
Below is a rendering of the page up to the first error."

[Tony Gentilcore]

> Qt bot got the following (or similar) error for ~2000 tests:
> "This page contains the following errors:
> error on line 1 at column 2: 
> Below is a rendering of the page up to the first error."

Oops, it looks like I swapped startParsing() and stopParsing() in the XMLDocumentParserQt.cpp :-(

Fixed and landing w/o the cq. I'll watch it as it lands this time.

[Tony Gentilcore]

Committed r66670: <http://trac.webkit.org/changeset/66670>