Last Comment Bug 602838 - Make script-inserted external scripts that have .async=false execute in the insertion order
: Make script-inserted external scripts that have .async=false execute in the i...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- major (vote)
: ---
Assigned To: Henri Sivonen (:hsivonen)
:
Mentors:
http://lists.w3.org/Archives/Public/p...
Depends on:
Blocks: 591981 609236 610369 620852
  Show dependency treegraph
 
Reported: 2010-10-08 05:45 PDT by Henri Sivonen (:hsivonen)
Modified: 2010-12-21 18:37 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta8+


Attachments
Untested fix (8.09 KB, patch)
2010-10-08 06:21 PDT, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter 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. (10.59 KB, patch)
2010-10-08 07:06 PDT, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter 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. v2 (11.62 KB, patch)
2010-10-11 03:19 PDT, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter 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 (11.64 KB, patch)
2010-10-15 02:53 PDT, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
part 1 - Execute in insertion order script-inserted external scripts that have the async DOM attribute reporting false (11.82 KB, patch)
2010-10-27 05:06 PDT, Henri Sivonen (:hsivonen)
jonas: review+
Details | Diff | Splinter Review
part 2 - Make script-created scripts (incl. scripts that have lost their parser-insertedness) default to .async=true (7.58 KB, patch)
2010-10-27 05:12 PDT, Henri Sivonen (:hsivonen)
jonas: review+
Details | Diff | Splinter Review

Description Henri Sivonen (:hsivonen) 2010-10-08 05:45:49 PDT
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) 2010-10-08 05:46:27 PDT
Nominating as a blocker. See http://lists.w3.org/Archives/Public/public-html/2010Oct/0088.html for the details.
Comment 2 Henri Sivonen (:hsivonen) 2010-10-08 06:21:30 PDT
Created attachment 481811 [details] [diff] [review]
Untested fix
Comment 3 Henri Sivonen (:hsivonen) 2010-10-08 07:06:34 PDT
Created attachment 481823 [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.

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) 2010-10-11 03:19:11 PDT
Created attachment 482207 [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. 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) 2010-10-11 10:54:21 PDT
Build for testing: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/hsivonen@iki.fi-43289b001dcf/
(The debug builds have the bad assertion that's addressed in the new patch.)
Comment 6 Henri Sivonen (:hsivonen) 2010-10-13 01:56:15 PDT
(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, jst@mozilla.com) 2010-10-13 19:28:54 PDT
Blocking.
Comment 8 Henri Sivonen (:hsivonen) 2010-10-15 02:53:41 PDT
Created 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

Refreshing the patch so that it applies to trunk now that bug 597368 has landed.
Comment 9 Henri Sivonen (:hsivonen) 2010-10-21 00:44:56 PDT
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) 2010-10-27 05:06:15 PDT
Created attachment 486327 [details] [diff] [review]
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) 2010-10-27 05:12:44 PDT
Created attachment 486328 [details] [diff] [review]
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 2010-10-31 11:52:31 PDT
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) 2010-11-10 01:09:44 PST
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 [:bz] 2010-11-10 01:23:48 PST
Imo, yes.
Comment 15 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-11-10 17:13:41 PST
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 2010-11-10 18:50:53 PST
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) 2010-11-11 00:05:08 PST
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) 2010-11-11 02:07:21 PST
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.)

Note You need to log in before you can comment on or make changes to this bug.