Please implement async=false for dynamic script loading (REGRESSION: LABjs is broken)

[Kyle Simpson]

Summary:

There is an informal W3C proposal being discussed for addressing a now-broken-in-Webkit use-case for dynamic script loading (in parallel) with ordered execution for dependencies. Mozilla has implemented the proposal in their nightlies (to address a recent similar breakage in FF4), and the request is that Webkit consider doing the same.


Back-story:

There is a need for dynamic script loaders (like LABjs http://labjs.com) to be able to dynamically load multiple script resources in parallel, but ensure that they execute in order (for dependency sake). For instance, for performance, one may desire to load jQuery, jQuery-UI, and all your plugins (in one file), each of these 3 files loaded in parallel, but executed in order (must be for depedencies).

Dynamic script loaders like LABjs need to be able to load such scripts on-demand (meaning later in the page lifetime, after the parser has finished), and also they need to be able to load any script that the author doesn't control (meaning they can't change the contents), and finally they need to be able to load such scripts from remote-domain locations (like CDN's). When you combine all these factors, "on-demand-parallel-load-serial-execute" becomes a pretty challenging feature.

Up until recently, LABjs has exploited some quirks of various browsers (non-HTML5 standard) to achieve this ability in all browsers. In IE and Webkit, LABjs has taken advantage of the non-standard behavior that a script-tag with an unrecognized mime-type like "script/cache" will still be loaded into cache, and the load event fired when complete, but the script wouldn't be executed. By fetching a group of scripts in parallel *only into cache* and being notified of the load events for all of them, LABjs could then wait for all scripts to load into cache, and then make a second pass through the list to load each (pratically synchronously/immediately) from the cache, by making a new script tag with the proper "text/javascript" type value.

This is obviously a big hack to get around the fact that in IE and Webkit, dynamically loaded scripts will fire as-soon-as-possible and there's no way to get the execution order to be in insertion order. In addition to being non-standard, this hack is brittle in that the resources in question must be served with proper caching headers (LOTS of javascript files on the web are not), or the hack breaks down and results in non-ordered and double-loaded behavior.

Nevertheless, it's been the only available feature for achieving parallel-load-serial-execute in IE and Webkit. Until the recent change made by Webkit nightly, which stops fetching scripts if the script-tag mime-type is unrecognized. This change essentially means that LABjs' technique is completely broken in Webkit now, and moreover, there's no way in Webkit for LABjs to achieve the desired loading functionality.

A similar breakage (although for a different reasoning) happened recently in FF4 nightlies/beta7. Working with the W3C (public-html list), and the Mozilla developers, an informal proposal has been discussed for changing the HTML5 spec to accomodate the need for this loading functionality.

You can read more about it here:

http://wiki.whatwg.org/wiki/Dynamic_Script_Execution_Order

http://hsivonen.iki.fi/script-execution/

---------------------------

So, the request is that Webkit also implement the current main (informal) proposal to the W3C in the same way that Mozilla has, to restore the ability for dynamic script loaders to serve this use-case in Webkit-based browsers.

The proposal basically preserves Webkit (and IE)'s current default behavior, but in a more formalized and flexible way, which is that any script inserted dynamically will essentially default to behaving like a parser-inserted script with `async=true`.

The requested extension is that a dynamically inserted script tag which has `async=false` set on it will go into a separate loading "queue" of sorts, in that all such scripts with `async=false` will load in parallel but will execute in insertion order.

This proposal has a sort of simple symmetry to it, in that it's basically making dynamically inserted scripts be able to have the same behaviors available to it as a parser-inserted script. By changing async to `true` or `false`, the author controls if they want "as-soon-as-possible execution" or "insertion-order execution". For parser-inserted scripts, the author already has that control (by setting or not setting `async`), so the request is that the author have the same control over script-inserted scripts as well.

By defaulting inserted scripts to having `async=true`, it is felt a greater amount of compat with previous content will be achieved (especially for existing IE/Webkit content). Moreover, by defaulting the `async` property of a dynamically inserted script to `true`, a feature-test is then obvious and possible for this new ordered execution behavior.

As stated earlier, this behavior has now been implemented in FF 4b8pre (nightlies), and LABjs 1.0.4 now takes advantage of it via the feature-test described. If Webkit were to implement the proposal as requested, such behavior would immediately be found and used, in favor of the previous hacky mime-type caching trick.

---------------------------

For the record, at the moment, LABjs (and any script loader which tries to do parallel loading of resources that have execution order dependencies) is *broken* in Webkit nightlies. The desire is to address this problem before the Webkit nightly/trunk is absorbed into a major release of any Webkit browser (Chrome, Safari, etc), which would break a lot of sites currently using such techniques via LABjs or other loaders.

I respectfully request that Webkit consider implementing the informal W3C proposal as described above.

[Henri Sivonen]

The spec bug is http://www.w3.org/Bugs/Public/show_bug.cgi?id=11295

By code inspection (I didn't test), I believe the "order" plug-in for RequireJS is broken in WebKit nightlies just like LABjs and, just like with LABjs, the latest version would work if WebKit implemented the change described in the spec bug and comment 0.

[Alexey Proskuryakov]

See also: bug 28783, bug 46398, bug 7722.

> The desire is to address this problem before the Webkit nightly/trunk is absorbed into a major release of any Webkit browser

Could you please provide a test case that behaves differently in Safari 5.0.3 and in nightlies? Per the bug I linked, we have long-standing issues with script execution order, and it's not clear to me what regression you have in mind.

[Kyle Simpson]

For the record, the changeset to Webkit nightly that caused the breakage in LABjs is:

http://trac.webkit.org/changeset/67245

Specifically, stopping the fetching of scripts into the cache that are from a script tag with an unrecognized mime-type (like "script/cache"). This non-standard behavior was a long-term behavior in Webkit, and was relied on as the only way to do parallel loading, as the "hack" described in the above original post describes.

So, to be clear, *this current bug* is requesting that Webkit implement the newly proposed `async=false` behavior, to give a more proper and direct way of handling the described use case. 

I'm *not* requesting that the hackish fake mime-type behavior (removed in the above changeset) be restored. Honestly, that was only a gap hack because there was no other way to do the required behavior in Webkit. If Webkit will implement `async=false`, the hack can be retired as only a legacy hack for older browsers in favor of the more proper approach from the proposal.

I am happy to make a test case to illustrate, but I think it's probably not necessary given this further description I've just made. But if you want a test, I can make one. All it needs to do is create a script element with a fake mime-type value and test whether that resource is loaded or not.

[Alexey Proskuryakov]

> http://trac.webkit.org/changeset/67245

CC'ing the author of this change.

> So, to be clear, *this current bug* is requesting that Webkit implement the newly
>  proposed `async=false` behavior, to give a more proper and direct way of handling
> the described use case. 

Understood. However, if we can't do that in time, fixing just the recent regression sounds like an important thing to do.

> I am happy to make a test case to illustrate, but I think it's probably not necessary
> given this further description I've just made.

Having a ready to run test case would help. We can probably make one from your description indeed.

[Alexey Proskuryakov]

> CC'ing the author of this change.

Actually, he was already CC'ed :)

[Tony Gentilcore]

I'm on vacation and travelling all next week. If this bug is still open when I return on Dec 6, I will put together a patch for async=false that week.

(Also copying rniwa who has been looking into script execution lately and simonjam who has been looking for bugs to pick up.)

[James Simonsen]

Created attachment 75397 [details]
Patch

[Alexey Proskuryakov]

Please do investigate how this affects bugs mentioned in comment 2.

[Early Warning System Bot]

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

[James Simonsen]

Created attachment 75400 [details]
Patch

[James Simonsen]

(In reply to comment #8)
> Please do investigate how this affects bugs mentioned in comment 2.

It doesn't have any effect on those. The only new behavior is when a dynamic script sets async=false. None of those test cases do that. Since I'm already in the area, I'll take a look at those other bugs separately.

Also, I fixed the QT build.

[Build Bot]

Attachment 75397 [details] did not build on win:
Build output: http://queues.webkit.org/results/6800011

[WebKit Review Bot]

Attachment 75397 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6830012

[Early Warning System Bot]

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

[James Simonsen]

Created attachment 75406 [details]
Patch

Argh. webkit-patch didn't upload the renamed files. This is a manually generated patch.

[Build Bot]

Attachment 75400 [details] did not build on win:
Build output: http://queues.webkit.org/results/6726011

[Adam Barth]

Would be nice to get tonyg to do an informal review of this patch.

[Ryosuke Niwa]

Comment on attachment 75406 [details]
Patch

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

> WebCore/dom/ScriptRunner.cpp:47
> +        m_scriptsToExecuteSoon[i].first->element()->deref(); // Balances ref() in executeScriptSoon().
> +        m_document->decrementLoadEventDelayCount();

I really don't like the fact we're calling ref/deref all over the places.  It's very error prone.  Can't we just use RefPtr or that's too expensive because of Vector operations?

> WebCore/dom/ScriptRunner.cpp:113
> +        m_scriptsToExecuteInOrder[0].first->execute(m_scriptsToExecuteInOrder[0].second.get());
> +        m_scriptsToExecuteInOrder[0].first->element()->deref(); // Balances ref() in executeScriptSoon().
> +        m_document->decrementLoadEventDelayCount();
> +        m_scriptsToExecuteInOrder.remove(0);

Is it really safe to remove the first script after executing the script?

[Adam Barth]

Comment on attachment 75406 [details]
Patch

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

>> WebCore/dom/ScriptRunner.cpp:47
>> +        m_scriptsToExecuteSoon[i].first->element()->deref(); // Balances ref() in executeScriptSoon().
>> +        m_document->decrementLoadEventDelayCount();
> 
> I really don't like the fact we're calling ref/deref all over the places.  It's very error prone.  Can't we just use RefPtr or that's too expensive because of Vector operations?

Yeah, it seems like we should be able to use RefPtrs to manage this memory.

[James Simonsen]

Created attachment 75516 [details]
Patch

[James Simonsen]

(In reply to comment #18)
> (From update of attachment 75406 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75406&action=review
> 
> > WebCore/dom/ScriptRunner.cpp:47
> > +        m_scriptsToExecuteSoon[i].first->element()->deref(); // Balances ref() in executeScriptSoon().
> > +        m_document->decrementLoadEventDelayCount();
> 
> I really don't like the fact we're calling ref/deref all over the places.  It's very error prone.  Can't we just use RefPtr or that's too expensive because of Vector operations?

Done.

I copied it from the async script handling. I assumed the Vector operations were the reason it was originally done with ref/deref.

> > WebCore/dom/ScriptRunner.cpp:113
> > +        m_scriptsToExecuteInOrder[0].first->execute(m_scriptsToExecuteInOrder[0].second.get());
> > +        m_scriptsToExecuteInOrder[0].first->element()->deref(); // Balances ref() in executeScriptSoon().
> > +        m_document->decrementLoadEventDelayCount();
> > +        m_scriptsToExecuteInOrder.remove(0);
> 
> Is it really safe to remove the first script after executing the script?

Yeah, it's the same as the async case, which removes the scripts after executing them too.

[Ryosuke Niwa]

Comment on attachment 75516 [details]
Patch

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

> WebCore/dom/ScriptRunner.cpp:105
> +    while (!m_scriptsToExecuteInOrder.isEmpty() && m_scriptsToExecuteInOrder[0].cachedScript->isLoaded()) {
> +        m_scriptsToExecuteInOrder[0].scriptElement->execute(m_scriptsToExecuteInOrder[0].cachedScript.get());
> +        m_document->decrementLoadEventDelayCount();
> +        m_scriptsToExecuteInOrder.remove(0);

Now that we use RefPtr, it might be not desirable to remove an element of m_scriptsToExecuteInOrder in each iteration of the loop.
Is it possible to count the number of items we remove, and mass-remove later?
Better yet, can we extract the scripts that have been loaded into a separate vector just like we do for m_scriptsToExecuteSoon?

[James Simonsen]

Created attachment 75523 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 75523 [details]
Patch

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

> WebCore/dom/ScriptRunner.cpp:101
> +    size_t numInOrderScriptsToExecute = 0;
> +    for (; numInOrderScriptsToExecute < m_scriptsToExecuteInOrder.size() && m_scriptsToExecuteInOrder[numInOrderScriptsToExecute].cachedScript->isLoaded(); ++numInOrderScriptsToExecute)
> +        scripts.append(m_scriptsToExecuteInOrder[numInOrderScriptsToExecute]);
> +    if (numInOrderScriptsToExecute)
> +        m_scriptsToExecuteInOrder.remove(0, numInOrderScriptsToExecute);

How about:
for (size_t i = 0; ; ++i) {
    if (i >= m_scriptsToExecuteInOrder.size() || !m_scriptsToExecuteInOrder[i].cachedScript->isLoaded()) {
        m_scriptsToExecuteInOrder.remove(0, i);
        break;
    }
    scripts.append(m_scriptsToExecuteInOrder[i]);
}

[Ryosuke Niwa]

(In reply to comment #24)
> How about:
> for (size_t i = 0; ; ++i) {
>     if (i >= m_scriptsToExecuteInOrder.size() || !m_scriptsToExecuteInOrder[i].cachedScript->isLoaded()) {
>         m_scriptsToExecuteInOrder.remove(0, i);
>         break;
>     }
>     scripts.append(m_scriptsToExecuteInOrder[i]);
> }

Oops, this doesn't quite work.  It needs to be:
for (size_t i = 0; ; ++i) {
    if (i >= m_scriptsToExecuteInOrder.size() || !m_scriptsToExecuteInOrder[i].cachedScript->isLoaded()) {
        if (i)
            m_scriptsToExecuteInOrder.remove(0, i);
        break;
    }
    scripts.append(m_scriptsToExecuteInOrder[i]);
}

Your version might be better after all.

[Tony Gentilcore]

Comment on attachment 75523 [details]
Patch

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

> LayoutTests/fast/dom/HTMLScriptElement/resources/big.js:3
> +    About 16KB to make this resource load slowly.

I'm not sure if this will reliably check your condition (although it won't be flaky). Did you consider making this an http test with a php/cgi resource that sleeps before returning? That is what some other script ordering tests do.

> LayoutTests/fast/dom/HTMLScriptElement/script-async-attr.html:17
> +<script id="s7" async></script>

We should also test <script id="s8" async="false"></script>

> WebCore/dom/ScriptRunner.cpp:1
> +/*

It is really difficult to review a rename with changes to the renamed file in the same patch. Is it too much trouble to ask to separate them?

[Alexey Proskuryakov]

> It is really difficult to review a rename with changes to the renamed file in the same patch. Is it too much trouble to ask to separate them?

An alternative is to use Subversion - in that case, svn-create-patch will separate a rename and later edits, making reviewing easy.

[James Simonsen]

(In reply to comment #26)
> (From update of attachment 75523 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75523&action=review
> 
> > LayoutTests/fast/dom/HTMLScriptElement/resources/big.js:3
> > +    About 16KB to make this resource load slowly.
> 
> I'm not sure if this will reliably check your condition (although it won't be flaky). Did you consider making this an http test with a php/cgi resource that sleeps before returning? That is what some other script ordering tests do.

No, but that's exactly what I wanted. I've moved it to an http/ test.

> > LayoutTests/fast/dom/HTMLScriptElement/script-async-attr.html:17
> > +<script id="s7" async></script>
> 
> We should also test <script id="s8" async="false"></script>

Added. It's counter-intuitive, but it matches the old behavior.

> > WebCore/dom/ScriptRunner.cpp:1
> > +/*
> 
> It is really difficult to review a rename with changes to the renamed file in the same patch. Is it too much trouble to ask to separate them?

I changed the options to git diff so that it detects renamed files.

[James Simonsen]

Created attachment 75748 [details]
Patch

[WebKit Review Bot]

Attachment 75748 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'HEAD']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'HEAD'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Simonsen]

Created attachment 76717 [details]
Patch

Updated to sync with recent changes.

[WebKit Review Bot]

Attachment 76717 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7173050

[James Simonsen]

Created attachment 76721 [details]
Patch

[Early Warning System Bot]

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

[Build Bot]

Attachment 76717 [details] did not build on win:
Build output: http://queues.webkit.org/results/7150052

[Tony Gentilcore]

FYI: http://www.nczonline.net/blog/2010/12/21/thoughts-on-script-loaders/

[Kyle Simpson]

Please consider this bug report I just filed with Mozilla regarding their implementation of this `async=false` behavior:

https://bugzilla.mozilla.org/show_bug.cgi?id=620852

In short:

Related to the functionality in this ticket, I would thus suggest a slight modification to the behavior of a parser-inserted script element's `async` attribute value interpretation:

1. If the value is exactly "false" (or any capitalization thereof), the script element should behave EXACTLY the same as if `async` attribute wasn't specified at all.

2. If the value is anything other than "false" (including no value at all, or "true", or "async", or "foobar"...), the async attribute is turned on.

This minor change will bring the `async` attribute's semantics more into line with the reflective `async` property's semantics.

---------
I always intended for my proposal to create symmetry and consistency between the attribute and property values and behavior. It was an oversight on my part that my original proposal left this detail out. I would ask that Webkit consider this additional change, to close up the inconsistency gap.

I can't imagine that there's any existing web content that is actually setting `async="false"` on markup script tags and still expecting that to turn async on, so I don't believe my request here will create backwards compatibility issues.

[Alexey Proskuryakov]

See discussion in <http://www.w3.org/Bugs/Public/show_bug.cgi?id=11295>. This probably shouldn't be implemented in WebKit until there is a reasonable level of agreement in the HTML WG.

[Kyle Simpson]

@Alexey--

With all due respect, I believe the opposite. The discussoin with WHATWG and the W3C members has been going for quite awhile. Some people support the proposal, others don't. This is true of any proposal, no matter how small or how big.

I think what's needed is browsers to *agree* to implement something, so it can be tried out (and proven/disproven) in the real world. Mozilla has done so. Opera is potentially considering it. And thus far in this discussion thread, Webkit is attempting to do so. If we have 2 or 3 major browsers agreeing on a feature, and it works, there's a good chance the others will come around, and the spec will eventually codify it.

-----------
There've been lots of other possible alternatives talked about (including <scriptgroup>, "depends", etc), but ALL of them are more complicated and have a wider surface-area of spec and browser functionality change. And many of them are not suitable to the main use-case.

To be clear, "async=false" as a proposal is not completely what I would want eventually from all browsers. There's another mechanism, already as a "may" suggestion in spec, that I would much prefer. But only IE implements it now, and the spec under-defines it only as a suggestion thus far. This is much further away from being a viable solution. So "async=false" is the next best thing, and certainly has a smaller surface-area of change and potential breakage.

*Right now*, at this moment, Webkit nightlies (by following the letter of the law in spec) have broken a hugely important use-case, and a bunch of sites that use script loaders like LABjs that rely on that use-case.

Just like when Mozilla's change broke LABjs back in October, and then they implemented the "async=false" proposal as a stop-gap to prevent existing content from breaking, I'm requesting that Webkit do the same, so that a browser like Safari or Chrome do not release with a trunk of Webkit that breaks the sites and the use-case.

[Ian 'Hixie' Hickson]

ap: If you think this is a good idea, implement it. If you don't, comment on the W3C bug.

[Adam Barth]

> *Right now*, at this moment, Webkit nightlies (by following the letter of the law in spec) have broken a hugely important use-case, and a bunch of sites that use script loaders like LABjs that rely on that use-case.

Which sites are broken today with the WebKit nightly?  Last time I asked, the list I got was a couple of sites that actually worked fine and the personal web site of the author of LABjs.

[Adam Barth]

Comment on attachment 76721 [details]
Patch

AFACT, our current behavior matches the current spec.  The process we've been using with the HTML5 parser, including the script execution part, is to track the spec unless compelled by compatibility to do something different.  If the HTML WG adopts this change, we'll likely follow suit.  However, I haven't seen any reason for us to lead the working group in this direction.

[Kyle Simpson]

@Adam-

> Which sites are broken today with the WebKit nightly?

I've listed several major sites which are using LABjs that *will* break in Webkit nightlies.

* Twitter
* Zappos
* Vimeo
* Mefeedia
* Formspring

The reason you don't see this breakage right now is for a few different reasons, but the bottom line is most of these sites have the "preloading" feature of LABjs turned off, mostly for "safety" reasons until this is all sorted out.

Basically, that means these sites have chosen to have LABjs operate in "serial-load-serial-execute" mode, which is loading each script one at a time, executing it, then loading the next, and so on.

Obviously, this is horribly worse performance than if they just used script tags (and ditched a script loader altogether), because with script tags in the markup, the browser automatically loads in parallel but executes in order.

I am personally in communication with the development teams for many of these major sites using LABjs, and keep them up to speed on the progress of this issue. They are aware of the danger of Webkit being "broken" on preloading. If Webkit refuses to address this use case, and a major browser release (Chrome or Safari) comes out with that broken behavior baked in, most of those sites will have to abandon using a script loader and go back to using regular script tags.

There's quite a bit of performance optimization that will be lost on these sites if they are forced to go back to using script tags and abandon using script loader techniques. This would be a huge loss for the web community to have Webkit not be forward-thinking (ahead of the spec) and address this obviously needed use case. 

Mozilla realized this same thing and that's why they acted with a stop-gap measure to address the need before the spec catches up. I'm asking Webkit to do the same.

[Antti Koivisto]

(In reply to comment #43)
> Obviously, this is horribly worse performance than if they just used script tags (and ditched a script loader altogether), because with script tags in the markup, the browser automatically loads in parallel but executes in order.

So why is async needed at all? As you point out, parallel load/serial execution is our normal script loading behavior. Non-serialized execution is a terrible idea as it leads to untestable, seemingly random content breakages.

[Kyle Simpson]

@Antti-

> So why is async needed at all?

I assume you mean the markup `async` attribute, not the (currently being discussed) `async` property on a dynamic script element in JavaScript?

The usefulness of the `async` attribute in markup is that it tells the browser that this particular script being loaded DOES NOT have any other dependencies, and it can thus execute "as soon as possible" once it downloads.

Scripts like "ga.js" (Google Analytics) are a perfect example of a script that is standalone and has no other dependencies, and so is a candidate for "run me as soon as possible, but don't make me wait on anyone and definitely don't make anyone else wait on me."

The whole point is that in markup, I can choose to either have the "async" attribute or not, and by doing so, I'm choosing if I need ordering behavior or not. For some scripts, I need ordering, for others I don't.

In JavaScript (using a script loader), the same is of course true. Sometimes I need to preserve execution order for dependencies, sometimes I do not want to. But in JavaScript, I have *no way* to choose the ordering behavior. It's *always* "async", even if I don't want it to be. So the only way for a script loader to get ordered execution is to also do ordered loading (ie, serial one-at-a-time loading), which is of course bad for performance.

[Kyle Simpson]

I have a new detour problem I need help understanding. This entire process of me requesting the "async=false" of Webkit was because of this changeset (and anticipated LABjs breakage):

http://trac.webkit.org/changeset/67245

...which auspiciously is supposed to prevent Webkit from fetching a script resource if the script `type` attribute/property is unrecognized. Indeed, if you try something like:

<script type="foo/bar" src="foo.js"></script>

...in markup, that script will in fact NOT be fetched.

However, if you do:

var script = document.createElement("script");
script.type = "foo/bar";
script.src="foo.js";
document.getElementsByTagName("head")[0].appendChild(script);

...*THAT* script will still be fetched (but not executed, obviously).

So, at the moment, we have that changeset only applying to parser-inserted script elements, and not script-inserted script elements.

Here's what the spec says, which prompted changeset 67245:

http://www.w3.org/TR/html5/scripting-1.html#running-a-script

> 5. If either:
>
> * the script element has a type attribute and its value is the empty string, 
> or 
> * the script element has no type attribute but it has a language attribute 
> and that attribute's value is the empty string, or 
> * the script element has neither a type attribute nor a language attribute, 
> then 
> 
> ...let the script block's type for this script element be "text/javascript".
>
> Otherwise, if the script element has a type attribute, let the script 
> block's type for this script element be the value of that attribute with any 
> leading or trailing sequences of space characters removed.
>
> Otherwise, the element has a non-empty language attribute; let the script 
> block's type for this script element be the concatenation of the 
> string "text/" followed by the value of the language attribute.
>
> The language attribute is never conforming, and is always ignored if there 
> is a type attribute present.
>
> 6. If the user agent does not support the scripting language given by the 
> script block's type for this script element, then the user agent must abort 
> these steps at this point. The script is not executed.

------

In other words, the above language in the spec is a little ambiguous as to whether or not the instruction to ignore fetching the script if the type is invalid ONLY applies to parser-inserted scripts, or if it applies ALSO to script-inserted scripts.

Many other parts of the algorithm at this point in the spec DO apply to both, and indeed I can't imagine that the spec is completely silent on how a script inserted script element's type is determined, so I believe that above language should apply to both.

But it appears that the Webkit changeset was only interpreted to apply to the parser-inserted scripts.

Is this intentional, and if so, what's the reasoning? If not, should a new bug be filed against that changeset to get it applied to dynamic script elements as well?

----------------

In any case, this is PART of why you're not yet seeing breakage in Webkit nightlies, because pretty much all script loaders use dynamic script elements and not script tags in markup.

[Ryosuke Niwa]

(In reply to comment #45)
> In JavaScript (using a script loader), the same is of course true. Sometimes I need to preserve execution order for dependencies, sometimes I do not want to. But in JavaScript, I have *no way* to choose the ordering behavior. It's *always* "async", even if I don't want it to be. So the only way for a script loader to get ordered execution is to also do ordered loading (ie, serial one-at-a-time loading), which is of course bad for performance.

Isn't this also true for script element created by a parser?  If I set async=false or didn't specify the attribute, then the script is loaded and executed in the order. I'm not sure if we should let it load out-of-order just because it's inserted by a script.  Why should load the resource differently for a script element created by a parser and one created by script?

[Kyle Simpson]

(In reply to comment #47)

> Isn't this also true for script element created by a parser?  If I set 
> async=false or didn't specify the attribute

Small nitpick, but actually, because of the definition of "boolean attributes" in markup, setting `async="false"` in markup will *not* be the same thing as not specifying the attribute. If the attribute is present, regardless of its value, the behavior is turned on.

> I'm not sure if we should let it load out-of-order just because it's 
> inserted by a script.  Why should load the resource differently for a script 
> element created by a parser and one created by script?

What you're really asking is: why is the *default behavior* for a script-inserted script element that it be unordered execution, while the default behavior for a markup script element is ordered?

Well, for one, because that's how Webkit (and IE) have always treated dynamic script elements, so changing that default behavior now would cause potentially a lot of compat issues for pages in those browsers. 

Also, and more importantly, because the spec says that dynamically created elements must be executed "as soon as possible".

My whole reason for asking for "async=false" is so that an author can CHOOSE which ordering behavior a particular dynamic script element needs. Some scripts will need ordered behavior, others will benefit from unordered behavior.

[Adam Barth]

> I've listed several major sites which are using LABjs that *will* break in Webkit nightlies.

So, currently, there isn't a compatibility problem to solve.  There's only a performance problem to solve, correct?

> I am personally in communication with the development teams for many of these major sites using LABjs, and keep them up to speed on the progress of this issue.

Great.  Thanks for doing that.  That's very valuable.

> There's quite a bit of performance optimization that will be lost on these sites if they are forced to go back to using script tags and abandon using script loader techniques.

Is this statement backed up with experiments or just intuition?  Antti's point, which I'm not sure you understood, is that "parallel load, sequential execute" is WebKit's default behavior if you have a bunch of script tags:

<script src="aa.js"></script>
<script src="bb.js"></script>

The preload scanner will kick off both loads in parallel and then execute them sequentially.

> This would be a huge loss for the web community to have Webkit not be forward-thinking (ahead of the spec) and address this obviously needed use case. 

This statement doesn't really make sense.  We should be agreeing on behavior in the HTML working group and then implementing those agreements.  Trying to end-run the process by sneaking a patch into WebKit isn't really productive.  

> Mozilla realized this same thing and that's why they acted with a stop-gap measure to address the need before the spec catches up. I'm asking Webkit to do the same.

You seem to view the spec as "slow" and in need of "catching up."  That's not true at all.  The spec can often be changed faster than implementations because it ships instantly whereas browsers have shipping cycles that last varying amounts of time, but as long as a year.

(In reply to comment #45)
> > So why is async needed at all?
> 
> I assume you mean the markup `async` attribute, not the (currently being discussed) `async` property on a dynamic script element in JavaScript?

Today these two things are the same.  The JavaScript property reflects the DOM attribute, like a ton of other JavaScript properties.

> But in JavaScript, I have *no way* to choose the ordering behavior.

Right.  That's the real problem.  Why don't we create an API that solves this particular problem instead of goofing up the connection between the async property and the async attribute?

> I have a new detour problem I need help understanding. This entire process of me requesting the "async=false" of Webkit was because of this changeset (and anticipated LABjs breakage):
> 
> http://trac.webkit.org/changeset/67245

I'm not sure I understand the connection with this change.

> So, at the moment, we have that changeset only applying to parser-inserted script elements, and not script-inserted script elements.

Would it help to apply it to script-inserted scripts as well?

> http://www.w3.org/TR/html5/scripting-1.html#running-a-script

The TR version of the spec is usually wildly out of date.  I recommend always using the whatwg version.

[[
If the user agent does not support the scripting language given by the script block's type for this script element, then the user agent must abort these steps at this point.
]]

> In other words, the above language in the spec is a little ambiguous as to whether or not the instruction to ignore fetching the script if the type is invalid ONLY applies to parser-inserted scripts, or if it applies ALSO to script-inserted scripts.

It seems clear that we shouldn't fetch the src in either case.

> Many other parts of the algorithm at this point in the spec DO apply to both, and indeed I can't imagine that the spec is completely silent on how a script inserted script element's type is determined, so I believe that above language should apply to both.

Indeed.

> But it appears that the Webkit changeset was only interpreted to apply to the parser-inserted scripts.

That sounds like a bug we should fix.

> Is this intentional, and if so, what's the reasoning? If not, should a new bug be filed against that changeset to get it applied to dynamic script elements as well?

It's not intentional.  Please file a new bug (and ideally attach a patch).

Bottom line: Let's work this issue out in the HTML WG.  That's the proper forum for this discussion.  Once we decided what to do in that venue, we can implement that decision here.

[Kyle Simpson]

> So, currently, there isn't a compatibility problem to solve.  There's only a 
> performance problem to solve, correct?

Wrong. As of earlier this morning, that's what I thought. But it turns out even if they were using the cache-preloading, they still wouldn't *currently* break in Webkit, because of what I discovered and explained in comment #46.


>> There's quite a bit of performance optimization that will be lost on these 
>> sites if they are forced to go back to using script tags and abandon using 
>> script loader techniques.
> Is this statement backed up with experiments or just intuition?  

It's not intuition, it's provable/experimentable fact.

Regular markup script tags *do* load in parallel, yes. But that's only a small fraction of the web performance optimization that's possible.

I've explained the 2 primary (performance) advantages of script loaders using dynamic script elements over regular script tags many times, including on the W3C list and bugs. But I'll restate them here for completeness.

> 1. Markup script tags *block* many other page-load activities, like
> downloading/rendering of images/css/etc, DOMContentLoaded/DOM-ready events,
> etc. Even though they may load in parallel to each other, because of
> document.write(), the browser must be pessimistic and must force everything
> else on the page to wait in case a document.write() is found in one of the
> scripts.
> 
> 2. There are potentially HUGE performance advantages to on-demand or
> lazy-loading techniques, where a page-load can be optimized by reducing to 
> the bare minimum (or none at all!) the scripts that are loaded during the 
> initial page-load, and instead putting off until later the loading (or 
> execution) of such scripts. By getting content in front of users quicker, 
> the perception of page-load speed is greatly improved, which improves user 
> satisfaction on sites.

And even if you want to completely ignore the performance advantages that dynamic script tags have over script loaders, there's still the module dependency management concern, which is that sometimes you dynamically (on-demand) load one or several modules in response to user actions in the page. You can't do that with regular script tags in markup, because the page has already been parsed. You *have* to use dynamic script tags. And if you have more than one script to load, you may very well run into this unordered execution pitfall.


> Antti's point, which I'm not sure you understood, is that "parallel load, 
> sequential execute" is WebKit's default behavior if you have a bunch of 
> script tags

No, I fully understand Antti's point. It's apples and oranges though. What a set of script tags can do in markup is not particularly relevant to what a set of dynamically created script elements can (or in this case, CANNOT) do in JavaScript.

The fact, whether we like it or not, is that dynamically created script tags DIFFER completely from markup inserted script tags, in their ordering behavior. And dynamic script loaders almost exclusively use dynamically created script elements via JavaScript.

As described above, dynamic script tags are far preferable to most script loaders because there's several performance advantages to them over markup script tags.

So, Webkit's "parallel load, sequential execute" default beahvior is all well and fine, except that it ONLY applies to markup script tags, and not to dynamic script tags, which isn't helpful to me, or any other script loaders for that matter.


> We should be agreeing on behavior in the HTML working group and then 
> implementing those agreements.  Trying to end-run the process by sneaking a 
> patch into WebKit isn't really productive.  

I am not trying to end-run the process at all. That's a ridiculous miscontruing of my actions and intent. I'm a complete outsider and this is my first foray into this world of specs and browser functionality. And to be honest, it's all completely opaque, obtuse, confusing, and frustrating.

I tried long and hard to work it out in the W3C comment threads and on the WHATWG wiki page. The long and short of that process thus far basically reveals that the spec process wasn't convinced this use-case was valid, and that input from, and 2 independent implementations by, browser vendors would be the likely next step before spec progress could continue. Mozilla was the first. And since Webkit was making a change that was also going to break LABjs, Webkit seemed like the likely second candidate to work this out with.

Now you tell me that I'm jumping the gun by coming to Webkit. You'll have to forgive me if I throw my hands up and cry "Shenanigans" here. I'm not trying to "tail-wag-the-dog", but depending on who I talk to, I get completely opposite advice.

Some people say that the spec should decide first, and the browsers should follow. Others say the browsers should experiment and when they agree on something that works, then the spec will codify it.

I have no idea how it should really work. But I do know this. If each group points the finger at the other (as is currently happening), deadlock is GUARANTEED.

Plus, you saw Ian's comment #40. He's basically responding (subtely) to the question of if the browser should wait on the spec, and saying "if you think it's a good idea, go ahead."

Now, you may not think it's a good idea, which we're trying to hash out. But suggesting that we have to wait on the spec SEEMS pretty counter-productive in this case.


>> I assume you mean the markup `async` attribute, not the (currently being 
>> discussed) `async` property on a dynamic script element in JavaScript?
> Today these two things are the same.  The JavaScript property reflects the 
> DOM attribute, like a ton of other JavaScript properties.

Wrong. These two things are NOT the same, and that's my whole point! The `async` attribute in markup can be present or not present, and that alone is enough to control the ordering behavior of that script tag with respect to other script tags.

But the `async` property in JavaScript on a dynamic script element CANNOT control this ordering behavior. The property is NOT fully consistent with, nor is it symmetrical to, the attribute.

If you examine a reflected JavaScript property of an existing markup-defined script element, it *will* indeed reflect `true` or `false` properly.

But, and here's the key, if you dynamically create a script element via JavaScript (not via markup), that `async` property is completely dead and useless. It doesn't default to `true` to indicate that this script will in fact have the same unordered behavior as a markup script tag with `async` attribute flagged on. And you can't change it to `true` or `false` to control it.

So, NO, the `async` attribute and the `async` property are definitely NOT the same.

My request is to ****make them much more the same****. My request is that the `async` property on a dynamically created script element (not from markup) reflect `true` for async, since that's how it will behave, and that I be able to change that value to false if I want it to behave "not async" (that is, ordered).


> Why don't we create an API that solves this particular problem 

This idea has been brought up several times. In fact, I don't have a huge problem with the idea of a new API, except that it is obviously going to be a lot more complicated than to tweak the existing mechanisms.

The chances of designing a whole new loading API and getting all the browsers to agree on it anytime soon are practically zero. LOTS of work and further discussion and arguing about API preferences and syntax styles and all that junk will be had before anything like that sees the light of day.

By contrast, async="false" has a very small footprint of change compared to that. It piggybacks on an existing mechanism. It extends it in what I think is a very logical, consistent, and symmetrical way (as described above). AND, it's already been implemented, tested, and proven in one browser. That automatically puts this idea further along in the horserace than any other yet-to-be-designed new loader API.


> instead of goofing up the connection between the async property and the async attribute?

I wholeheartedly disagree, as I just explained, that we are "goofing up the connection" between attribute and property. They *are*, as currently implemented (and spec'd) GOOFED UP and inconsistent, and I'm trying to make them consistent and un-goofed up.


>> http://trac.webkit.org/changeset/67245
> I'm not sure I understand the connection with this change.

This change is why we got into this whole discussion in the first place. Because ostensibly, Webkit was going to disable the only viable "preloading" mechanism available to a script loader (the "cache preload fake mimetype" trick), which they rely on to be able to achieve the intended parallel-load, serial-execute behavior.

"Cache preloading" enables the use-case in a hacky round-about way by pre-caching all the scripts, then re-requesting them (from cache) with proper script tags, in the desired order.

By Webkit removing the ability to do "cache preloading", and NOT enabling the use-case in another straight forward facility/mechanism, you are neutering script loaders like LABjs.


> Bottom line: Let's work this issue out in the HTML WG.  That's the proper 
> forum for this discussion.  Once we decided what to do in that venue, we can 
> implement that decision here.

Again, that would be fine EXCEPT that I've already tried that and they essentially, in a round-about way, have said that browser vendors need to have more input and implementations to give better spec guidance.

[Kyle Simpson]

If I have in my markup:

<input id="button1" type="button" value="Click Me" disabled />

That button is disabled as I want it to be. If I have another button:

<input id="button2" type="button" value="No, Click Me" />

That button is NOT disabled, because I didn't add the `disabled` attribute. I controlled the disabled state by presence or absence of an attribute.

If I create a button in JavaScript dynamically like this:

var btn = document.createElement("input");
btn.type = "button";
btn.value = "Really, just click me";

I can control if that button is disabled or not by saying:

btn.disabled = true;
btn.disabled = false;

**THIS** is an example of, in my opinion, a markup tag's attribute and its corresponding property in JavaScript being VERY consistent.

However, the same is NOT true for the `async` attribute of script tags.

Is it really so hard to see why this is the "goofed up" inconsistency with `async` attributes and properties I want to solve?

[Antti Koivisto]

I think async was originally specced mostly to address the basic problem that browsers would serialize loading of <script> tags. That problem no longer exist since we have figured out how to parallelize parser initiated loading.

If we do something bad with with dynamically inserted script elements (do we not serialize execution? I didn't quite get what is going wrong) then that is an important bug to fix but does not justify async.

Generally it sounds to me like async is mostly just a workaround (and an excuse) for bad or buggy regular loading behavior. The actual unique behavior (non-serialization) is a just bad idea (untestable fragility). I might be missing some big upside but it sounds to me that the best way to fix any and all async bugs would be to eliminate the feature from the specs and engines.

[Kyle Simpson]

(in reply to comment #52)

@Antti-

The "async" attribute on a markup tag does something very important on top of parallel loading that a normal script tag without that attribute in markup will not get. It tells the browser "this script is totally independent and can EXECUTE as soon as possible, at any time. don't wait on any other scripts for it to execute, and don't make any other scripts wait on this one before they execute."

This is quite important, but it's distinct and in addition to the parallel loading that all modern browsers have now figured out.

It's the difference between loading and execution that's at stake. Some scripts (like Google Analytics, for instance) are totally independent of all other scripts, and thus I not only want good parallel loading behavior but I ALSO want to have its execution happen independently (that is, not waiting on or blocking) of other scripts.

Again, it feels like your underlying question is actually: why is a dynamic script element behaving like an "async" script element? 

The answer is legacy compat and because the spec says to.

[Kyle Simpson]

While they may not be entirely in a blocks/depends relationship, I believe this bug is highly related to bug #51760. If bug #51760 is fixed, then we will finally be able to see some demonstratable breakage from "cache preloading" as asserted in this ticket.

[Henri Sivonen]

(In reply to comment #37)
> Please consider this bug report I just filed with Mozilla regarding their implementation of this `async=false` behavior:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=620852

RESOLVED INVALID. (You may alternatively consider it WONTFIX. There's no need to stir boolean attribute reflection to consider attribute values in order to achieve the aims of http://www.w3.org/Bugs/Public/show_bug.cgi?id=11295 )

> Related to the functionality in this ticket, I would thus suggest a slight modification to the behavior of a parser-inserted script element's `async` attribute value interpretation:
> 
> 1. If the value is exactly "false" (or any capitalization thereof), the script element should behave EXACTLY the same as if `async` attribute wasn't specified at all.

I think it's best not to try to make async any more magic than http://www.w3.org/Bugs/Public/show_bug.cgi?id=11295#c0 says. Making the DOM property not initially reflect the content attribute is bad enough. Please, please, let's not derail the chance to get interop between Gecko and WebKit here by having an argument about whether to also make the reflection of the async attribute even more magic by considering the attribute values.

[Kyle Simpson]

(in reply to comment #55)

Yeah, I've tried over on the Mozilla thread to stir up any kind of support for that idea, and it failed miserably. It's obviously a dead end. Notice that's why it hasn't even been brought up here again since.

[Kyle Simpson]

If it wasn't clearly communicated, a few weeks ago, Ian posted this comment on the W3C bug thread regarding this "async=false" proposal:

"I plan to spec what's proposed in comment 0 (modulo editorial issues)."

http://www.w3.org/Bugs/Public/show_bug.cgi?id=11295#c13

So, as time permits, I expect that the proposal in question will indeed become part of the spec, which I hope should help address the uneasiness expressed here about if this bug should proceed to being committed to Webkit.

[James Simonsen]

Created attachment 81682 [details]
Patch

[Adam Barth]

Comment on attachment 81682 [details]
Patch

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

Looks great!  Ideally Tonyg would look at this patch as well.  Thanks!

> Source/WebCore/dom/ScriptRunner.cpp:68
> +    ASSERT_ARG(scriptElement, scriptElement);

Why ASSERT_ARG and not just ASSERT?

[Tony Gentilcore]

> Looks great!  Ideally Tonyg would look at this patch as well.  Thanks!
I'll take a look in the morning. Thanks for putting it together!

[James Simonsen]

Created attachment 81749 [details]
Patch

[James Simonsen]

(In reply to comment #59)
> (From update of attachment 81682 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81682&action=review
> 
> Looks great!  Ideally Tonyg would look at this patch as well.  Thanks!
> 
> > Source/WebCore/dom/ScriptRunner.cpp:68
> > +    ASSERT_ARG(scriptElement, scriptElement);
> 
> Why ASSERT_ARG and not just ASSERT?

No idea. I copied it from the function above it. I switched both to just use ASSERT.

[Tony Gentilcore]

Comment on attachment 81749 [details]
Patch

Overall the patch looks good, but it looks like the patch should be bounced up against http://html5.org/tools/web-apps-tracker?from=5816&to=5817 to make sure naming and details are as close as possible.


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

> Source/WebCore/dom/ScriptElement.cpp:61
> +    , m_willExecuteInOrder(false)

Since the spec says "Initially, script elements must have this flag set. It is unset...", I think we should default it to true and invert the condition in insertedIntoDocument to unset it instead of setting it.

> Source/WebCore/dom/ScriptElement.h:94
> +    bool m_willExecuteInOrder;

I realize you wrote this patch before the spec update. But now that the spec is updated and calls this "force-async", I think we should use similar names so that future code readers can tie it to the spec easier. I'd suggestion m_forceAsync.

> Source/WebCore/html/HTMLScriptElement.h:72
> +    bool m_forceAsync;

I don't think this should be HTMLScriptElement-specific. The spec talks of the force-async flag for scripts created by the XML parser.

[James Simonsen]

Created attachment 83424 [details]
Patch

[James Simonsen]

(In reply to comment #63)
> (From update of attachment 81749 [details])
> Overall the patch looks good, but it looks like the patch should be bounced up against http://html5.org/tools/web-apps-tracker?from=5816&to=5817 to make sure naming and details are as close as possible.

I've updated this to merge cleanly with the recent ScriptElement changes. It should be easier to follow now that ScriptElement more closely follows the spec. cq- to let the ScriptElement bake a little longer before we add new behavior to it.

[WebKit Review Bot]

Attachment 83424 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/dom/ScriptElement.cpp:101:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Simonsen]

Created attachment 84344 [details]
Patch

[Tony Gentilcore]

Comment on attachment 84344 [details]
Patch

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

> LayoutTests/http/tests/misc/script-async-load-execute-in-order-expected.txt:1
> +Attempts to check that async=false enforces execution order dynamically inserted scripts. See: http://wiki.whatwg.org/wiki/Dynamic_Script_Execution_Order

s/Attempts to check that/Checks that/

Also, let's point to the spec instead of this wiki.

> Source/WebCore/dom/ScriptElement.cpp:65
> +    , m_willExecuteInOrder(false)

Is this bool really necessary? Seems like we theoretically should be able to get by w/ just m_forceAsync.

> Source/WebCore/dom/ScriptElement.cpp:304
> +        m_element->document()->scriptRunner()->resume();

It doesn't seem right to call resume here. Yes, the implementation does what you want. But it is confusing because it was never suspended.

> Source/WebCore/dom/ScriptRunner.cpp:99
> +    for (; numInOrderScriptsToExecute < m_scriptsToExecuteInOrder.size() && m_scriptsToExecuteInOrder[numInOrderScriptsToExecute].cachedScript()->isLoaded(); ++numInOrderScriptsToExecute)

The other loop makes it a point to copy out the vector first in case during the course of script execution something new is queued up. Shouldn't we do that here as well?

In any case, it would be nice to add a test that has a async=false script which in the course of its execution queues up another async=false script.

> Source/WebCore/dom/ScriptRunner.h:49
> +    void queueScriptForInOrderExecution(ScriptElement*, CachedResourceHandle<CachedScript>);

Would be nice if these two methods had more synergy in their naming.

Maybe executeScriptSoon() and executeScriptInOrderSoon() or queueScriptForExecution() and queueScriptForInOrderExecution().

[Kyle Simpson]

It would appear that Bug #51760 has either been landed in Webkit nightlies, or was picked up early by Chrome... because LABjs is now breaking in Chrome nightlies, with symptoms clearly tied to 51760. I was under the impression that 51760 and *this* bug would be landed together so as to minimize breakage for LABjs.

Can you advise?

[James Simonsen]

(In reply to comment #68)
> (From update of attachment 84344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84344&action=review
> 
> > LayoutTests/http/tests/misc/script-async-load-execute-in-order-expected.txt:1
> > +Attempts to check that async=false enforces execution order dynamically inserted scripts. See: http://wiki.whatwg.org/wiki/Dynamic_Script_Execution_Order
> 
> s/Attempts to check that/Checks that/

Done.

> Also, let's point to the spec instead of this wiki.

Done.

> > Source/WebCore/dom/ScriptElement.cpp:65
> > +    , m_willExecuteInOrder(false)
> 
> Is this bool really necessary? Seems like we theoretically should be able to get by w/ just m_forceAsync.

Yeah, unfortunately it's necessary. The page could manipulate the async attribute after the script is requested, but before it executes. In such a case, we need to honor the decision made during prepareScript(), not the current state of the attributes.

Sounds like something worthy of a test, so I added it to the http test.

> > Source/WebCore/dom/ScriptElement.cpp:304
> > +        m_element->document()->scriptRunner()->resume();
> 
> It doesn't seem right to call resume here. Yes, the implementation does what you want. But it is confusing because it was never suspended.

Done. Long term, we might want to make script runner listen for the callbacks directly instead of the current callback->element->runner->element.

> > Source/WebCore/dom/ScriptRunner.cpp:99
> > +    for (; numInOrderScriptsToExecute < m_scriptsToExecuteInOrder.size() && m_scriptsToExecuteInOrder[numInOrderScriptsToExecute].cachedScript()->isLoaded(); ++numInOrderScriptsToExecute)
> 
> The other loop makes it a point to copy out the vector first in case during the course of script execution something new is queued up. Shouldn't we do that here as well?

We do. Everything is moved to "scripts" before we start executing.

> In any case, it would be nice to add a test that has a async=false script which in the course of its execution queues up another async=false script.

Good idea. Done.

> > Source/WebCore/dom/ScriptRunner.h:49
> > +    void queueScriptForInOrderExecution(ScriptElement*, CachedResourceHandle<CachedScript>);
> 
> Would be nice if these two methods had more synergy in their naming.
> 
> Maybe executeScriptSoon() and executeScriptInOrderSoon() or queueScriptForExecution() and queueScriptForInOrderExecution().

Done. It's just one method with an enum arg now.

[James Simonsen]

Created attachment 84783 [details]
Patch

[Kyle Simpson]

(in reply to my own comment #69)

it is confirmed that the breaking change has landed in Webkit, albeit not by Bug #51760 as I assumed. Apparently the change was made in another changeset and the correlation was missed.

So, at this point, Webkit (and Chome) are broken on LABjs in their nightlies, which makes it kind of important that this patch land successfully fairly soon. Are we on a good track with that?

[Tony Gentilcore]

Comment on attachment 84783 [details]
Patch

Looks good now, thanks again. There's probably some follow-up refactoring that can be done now, but landing this first seems like the right approach.

[James Simonsen]

Created attachment 85399 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 85399 [details]
Patch for landing

Rejecting attachment 85399 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'ap..." exit_code: 2

Last 500 characters of output:
rce/WebCore/dom/AsyncScriptRunner.cpp'
patching file Source/WebCore/dom/ScriptRunner.cpp
rm 'Source/WebCore/dom/AsyncScriptRunner.h'
patching file Source/WebCore/dom/ScriptRunner.h
patching file Source/WebCore/html/HTMLScriptElement.cpp
patching file Source/WebCore/html/HTMLScriptElement.h
patching file Source/WebCore/html/HTMLScriptElement.idl
patching file Source/WebCore/page/PageGroupLoadDeferrer.cpp

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

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

[James Simonsen]

Committed r81198: <http://trac.webkit.org/changeset/81198>

[WebKit Review Bot]

http://trac.webkit.org/changeset/81198 might have broken WinCE Release (Build)