602838 - Make script-inserted external scripts that have .async=false execute in the insertion order

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Henri Sivonen (:hsivonen)]

Long version:
http://lists.w3.org/Archives/Public/public-html/2010Oct/0088.html

Short version:
Bug 591981 made Gecko run scripts in an HTML5-compliant way in order to avoid site breakage. The HTML5-compliant way is modeled on the IE/WebKit behavior, so it was reasonable to assume it to be Web-compatible.

However, the LABjs library and the "order" plug-in for RequireJS run a different code path for Gecko/Opera vs. WebKit/IE. To avoid breaking sites we should make the minimal necessary willful spec violation: Executing script-inserted scripts that don't have the async attribute set in the insertion order.

It might even make sense to try to squeeze this into beta7.

Patch coming up.

Comment 1

[Henri Sivonen (:hsivonen)]

Nominating as a blocker. See http://lists.w3.org/Archives/Public/public-html/2010Oct/0088.html for the details.

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: Untested fix

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Execute script-inserted external scripts that do not have the async attribute in their isertion order to make the Gecko-sniffed code path in LABjs and the "order" plug-in of RequireJS work.

Notes:
 * It's desirable to keep script-inserted external scripts with the async attribute truly async, since e.g. RequireJS, judging from code inspection, in its modes that explicitly don't preserve order already sets the async attribute due to opt out of Gecko's legacy ordered behavior in Firefox 3.6.
 * The belief that this patch actually addresses compatibility problem with deployed copies of LABjs and the "order" plug-in for RequireJS is based on http://blog.getify.com/2010/10/mozilla-labjs-part-2/
 * The belief that sites would break without this patch is based on the same blog post and on inspecting the code of the JS libraries in question.

I ran the obvious mochitests locally, and they passed. I'll send this to try still, shortly.

In case this gets r+ and even blocking beta7 on Friday Mountain View time, please feel free to treat this as "check-in needed". I most likely won't be able to land this until Monday AM European time.

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Execute script-inserted external scripts that do not have the async attribute in their isertion order to make the Gecko-sniffed code path in LABjs and the "order" plug-in of RequireJS work. v2

Oops. I had forgotten to update one assertion.

While I was at it, I made the test case better, too. It now also has an async script and tests that a script-inserted external script with async=true doesn't wait for pending non-async script-inserted external scripts.

Comment 5

[Henri Sivonen (:hsivonen)]

Build for testing: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-43289b001dcf/
(The debug builds have the bad assertion that's addressed in the new patch.)

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to comment #0)
> It might even make sense to try to squeeze this into beta7.

OTOH, maybe it's even good to ship beta7 without this patch in order to see the extent of the breakage in practice.

Comment 7

[Johnny Stenback (:jst)]

Blocking.

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Execute script-inserted external scripts that do not have the async attribute in their isertion order to make the Gecko-sniffed code path in LABjs and the "order" plug-in of RequireJS work. v3

Refreshing the patch so that it applies to trunk now that bug 597368 has landed.

Comment 9

[Henri Sivonen (:hsivonen)]

Comment on attachment 483424 [details] [diff] [review]
Execute script-inserted external scripts that do not have the async attribute in their isertion order to make the Gecko-sniffed code path in LABjs and the "order" plug-in of RequireJS work. v3

Canceling the review request for now, since it's not clear enough what the path forward is going to be, and I'm now guessing that even if this patch is going to be part of the fix, it's not going to be the entire fix.

Comment 10

[Henri Sivonen (:hsivonen)]

Attachment: part 1 - Execute in insertion order script-inserted external scripts that have the async DOM attribute reporting false

Rebasing on top of beta7 blockers.

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: part 2 - Make script-created scripts (incl. scripts that have lost their parser-insertedness) default to .async=true

Here's an implementation of the proposal to make .async default to true for script-created scripts.

This proposal doesn't enjoy consensus at the HTML WG at this point, but I wrote the patch already anyway to make it possible to go ahead with this plan on a short notice.

Considering the status of the Firefox 4.0 release cycle and the complexities of the alternative proposals, I think the proposal implemented by these two patches taken together makes most sense.

If beta exposure ends up revealing too much site compat trouble with both part 1 and part 2, the two-part arrangement makes it possible to back out part 2 from Firefox 4 while keeping both parts in Firefox 4.next.

Comment 12

[Kyle]

BTW, the discussion on this topic has moved from the W3C public-html email list to:

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

Comment 13

[Henri Sivonen (:hsivonen)]

Should we make this a beta 8 blocker? As long as the patches here haven't landed, we don't have a good evangelism story for the sites that sniff Firefox/Gecko and expect HTML5-incompliant order-preserving behavior. Beta 7 will ship with this problem, but it would be unfortunate if the next beta lacked a nice evang story, too.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Imo, yes.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 486327 [details] [diff] [review]
part 1 - Execute in insertion order script-inserted external scripts that have the async DOM attribute reporting false

r=me, though I don't really understand why we don't want DOM-inserted external scripts to block parser scripts. The only reason I can think of is that it'd be somewhat iffy to block parser-inserted inline scripts, but a bit inconsistent not to.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 486328 [details] [diff] [review]
part 2 - Make script-created scripts (incl. scripts that have lost their parser-insertedness) default to .async=true

>   void LoseParserInsertedness()
>   {
>     mFrozen = PR_FALSE;
>     mUri = nsnull;
>     mCreatorParser = nsnull;
>     mParserCreated = mozilla::dom::NOT_FROM_PARSER;
>+    PRBool async = PR_FALSE;
>+    nsCOMPtr<nsIDOMHTMLScriptElement> htmlScript = do_QueryInterface(this);
>+    if (htmlScript) {
>+      htmlScript->GetAsync(&async);
>+    }
>+    mForceAsync = !async;
>   }

I'm not convinced this is a good idea, but it does make some sense.

However couldn't you just set mForceAsync = PR_TRUE? If the attribute is changed in any way mForceAsync is set to false anyway?

Comment 17

[Henri Sivonen (:hsivonen)]

http://hg.mozilla.org/mozilla-central/rev/fce2fc592595
http://hg.mozilla.org/mozilla-central/rev/be59b0a650ad

Thanks for the r+!

(In reply to comment #16)
> Comment on attachment 486328 [details] [diff] [review]
> part 2 - Make script-created scripts (incl. scripts that have lost their
> parser-insertedness) default to .async=true
> 
> >   void LoseParserInsertedness()
> >   {
> >     mFrozen = PR_FALSE;
> >     mUri = nsnull;
> >     mCreatorParser = nsnull;
> >     mParserCreated = mozilla::dom::NOT_FROM_PARSER;
> >+    PRBool async = PR_FALSE;
> >+    nsCOMPtr<nsIDOMHTMLScriptElement> htmlScript = do_QueryInterface(this);
> >+    if (htmlScript) {
> >+      htmlScript->GetAsync(&async);
> >+    }
> >+    mForceAsync = !async;
> >   }
> 
> I'm not convinced this is a good idea, but it does make some sense.

This is needed to avoid changing the edge case where scripts lose their parser-insertedness. I think it would be wrong to simplify this away.

> However couldn't you just set mForceAsync = PR_TRUE? If the attribute is
> changed in any way mForceAsync is set to false anyway?

When I was developing the patch, I was pretty sure it was necessary to do it this way, but I'll double-check if removeAttribute causes the suitable internal method calls in this case and file a follow-up bug if this can be simplified. (I landed the patch anyway before double-checking so that people affected by beta 7 have a nightly to test workarounds with ASAP.)

(In reply to comment #15)
> Comment on attachment 486327 [details] [diff] [review]
> part 1 - Execute in insertion order script-inserted external scripts that have
> the async DOM attribute reporting false
> 
> r=me, though I don't really understand why we don't want DOM-inserted external
> scripts to block parser scripts. The only reason I can think of is that it'd be
> somewhat iffy to block parser-inserted inline scripts, but a bit inconsistent
> not to.

The main reason was to avoid setting us up for a perf disadvantage relative to WebKit and IE in case we end up having to back out the "part 2" patch before release. The secondary reason is that blocking parser-inserted scripts would have been a larger deviation from what the HTML5 spec says now and a deviation that LABjs doesn't need. The third reason is that if a Web authors wants to insert a script in a way that blocks the parser, there already exists a more compatible way: Writing a script tag using document.write(). When a more compatible way exists, it doesn't really make sense to have a non-feature-detectable Firefox-specific way that would potentially be a perf disadvantage compared to WebKit and IE.

Comment 18

[Henri Sivonen (:hsivonen)]

Documented:
https://developer.mozilla.org/en/Firefox_4_for_developers
https://developer.mozilla.org/En/HTML/Element/Script

Spec bug filed:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=11295

(Also, a blog post coming up once the nightlies have cycled.)