Last Comment Bug 503481 - Implement async attribute of script element
: Implement async attribute of script element
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86 All
: -- normal with 2 votes (vote)
: mozilla1.9.3a1
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on: 527623 527870
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-10 04:24 PDT by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2014-11-19 21:56 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta3-fixed


Attachments
Patch to fix (20.32 KB, patch)
2009-11-07 21:52 PST, Jonas Sicking (:sicking) PTO Until July 5th
bugs: review+
jst: superreview+
Details | Diff | Review
Fix review comments and add more tests (27.97 KB, patch)
2009-11-09 17:00 PST, Jonas Sicking (:sicking) PTO Until July 5th
jonas: review+
jonas: superreview+
Details | Diff | Review
Branch patch (27.98 KB, patch)
2009-11-09 18:32 PST, Jonas Sicking (:sicking) PTO Until July 5th
mbeltzner: approval1.9.2+
Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-07-10 04:24:37 PDT
HTML5 defines async attribute for script element.
I don't know which browsers implement it, but
at least webkit doesn't seem to
https://bugs.webkit.org/show_bug.cgi?id=20710

I don't know the reasons for async attribute, so before implementing it, 
one should make sure it is actually useful and well-enough defined.
Comment 1 Henri Sivonen (:hsivonen) 2009-07-10 04:27:31 PDT
Reminder: If we ever implement this, async scripts should not run code for permitting document.write() (upcoming code in nsIScriptElement per my current thinking).
Comment 2 bnkuhn 2009-09-29 20:21:43 PDT
The async attribute would be extremely useful for things like Google Analytics.  Scripts like ga.js are independent of other scripts.  Ideally it should run as soon as it can, but without blocking other scripts from executing.  There are ways (hacks) to accomplish this in other browsers, but in Firefox (and Opera) it can only be downloaded asynchronously.  Firefox preserves script execution order no matter which script finishes downloading first.  So, if ga.js takes a long time to download, the scripts that come after it can't execute even though they may be ready to do so.  We could prevent this if Firefox supported the async attribute.
Comment 3 Henri Sivonen (:hsivonen) 2009-10-09 01:43:30 PDT
About comment 1, see bitrotted patch in bug 503473.

Another note to implementor: When the async script starts/finishes running, please don't call methods on the parser or sink. Doing the usual parser-inserted script thing would confuse the speculative parsing part of the HTML5 parser.
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-07 21:52:32 PST
Created attachment 411027 [details] [diff] [review]
Patch to fix

This implements the async attribute.
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-07 22:18:15 PST
Comment on attachment 411027 [details] [diff] [review]
Patch to fix

Smaug, feel free to take the review if blake doesn't get to it first. Was gonna try to pitch this on tuesday so would be great to get this reviewed soon.
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-11-09 02:46:28 PST
Comment on attachment 411027 [details] [diff] [review]
Patch to fix

>   /**
>    * Is the script deferred. Currently only supported by HTML scripts.
>    */
>   virtual PRBool GetScriptDeferred() = 0;
> 
>+  /**
>+   * Is the script async. Currently only supported by HTML scripts.
>+   */
>+  virtual PRBool GetScriptAsync() = 0;
>+
Could you add some more documentation that when the script is deferred and when
it is async.

>diff --git a/content/base/test/test_bug503481.html b/content/base/test/test_bug503481.html
>new file mode 100644
>--- /dev/null
>+++ b/content/base/test/test_bug503481.html
>@@ -0,0 +1,62 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=503481
>+-->
>+
>+<head>
>+  <title>Test for Bug 503481</title>
>+  <script src="/MochiKit/packed.js"></script>
>+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>+</head>
>+<body onload="done();">
>+
>+<a href="https://bugzilla.mozilla.org/show_bug.cgi?id=503481"
>+   target="_blank" >Mozilla Bug 503481</a>
>+
>+<p id="display"></p>
>+
>+<script>
>+SimpleTest.waitForExplicitFinish();
>+function done() {
>+  is(firstRan, true, "first has run");
>+  is(secondRan, true, "second has run");
>+  is(thirdRan, true, "third has run");
>+  SimpleTest.finish();
>+}
>+var reqs = [];
>+function unblock(s) {
>+  xhr = new XMLHttpRequest();
>+  xhr.open("GET", "file_bug503481.sjs?unblock=" + s);
>+  xhr.send();
>+  reqs.push(xhr);
>+}
>+var firstRan = false, secondRan = false, thirdRan = false;
>+function runFirst() { firstRan = true; }
>+function runSecond() {
>+  is(thirdRan, true, "should have run third already");
>+  secondRan = true;
>+}
>+function runThird() {
>+  is(secondRan, false, "shouldn't have unblocked second yet");
>+  thirdRan = true;
>+  unblock("B");
>+}
>+</script>
>+<script async src="file_bug503481.sjs?blockOn=A&body=runFirst();"></script>
>+<script>
>+is(firstRan, false, "First async script shouldn't have run");
>+unblock("A");
>+</script>
>+
>+<script async src="file_bug503481.sjs?blockOn=B&body=runSecond();"></script>
>+<script async src="file_bug503481.sjs?blockOn=C&body=runThird();"></script>
>+<script>
>+is(secondRan, false, "Second async script shouldn't have run");
>+is(thirdRan, false, "Third async script shouldn't have run");
>+unblock("C");
>+</script>
>+
>+</body>
>+</html>

Add some tests for <script async>, i.e. no src attribute. Also, add tests
for cases when there are both async and defer attributes.

I couldn't yet find what HTML5 tries to say about
parser-inserted + document.write handling.
I hope the patch does what HTML5 defines, and HTML5 defines something reasonable,
but I need to figure that out before giving r+.
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-11-09 02:53:39 PST
How does the patch handle document.write if the script is in cache, so that
it is loaded immediately/fast? What happens if the script is loading slowly?
Comment 8 Henri Sivonen (:hsivonen) 2009-11-09 04:40:36 PST
(In reply to comment #7)
> How does the patch handle document.write if the script is in cache, so that
> it is loaded immediately/fast? 

Per HTML5, document.write() should blow away the document when called from an async script, because there's not supposed to be any way to have the insertion point set to not 'undefined' when an async script is executed.

With this patch, do async script cause all the usual calls back to nsIParser (Block/Unblock/ContinueInterruptedParsing)?
Comment 9 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-11-09 04:50:01 PST
(In reply to comment #8)
> Per HTML5, document.write() should blow away the document when called from an
> async script, because there's not supposed to be any way to have the insertion
> point set to not 'undefined' when an async script is executed.
Except if async script is executed because of sync XHR.
So we probably need to temporarily clear insertion point while
executing async script.
Comment 10 Henri Sivonen (:hsivonen) 2009-11-09 04:57:09 PST
(In reply to comment #9)
> (In reply to comment #8)
> > Per HTML5, document.write() should blow away the document when called from an
> > async script, because there's not supposed to be any way to have the insertion
> > point set to not 'undefined' when an async script is executed.
> Except if async script is executed because of sync XHR.
> So we probably need to temporarily clear insertion point while
> executing async script.

As mentioned on IRC, there's a spec bug about this:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=7926
http://lists.w3.org/Archives/Public/public-html/2009Oct/0424.html
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-09 09:33:32 PST
We don't in general do document.write according to the HTML5 spec. We don't have the concept of insertion points or anything like that. So for example a document.write from inside a timeout will either blow away the document, or simply add to the current one, depending on if we've fired DOMContentLoaded or not.

I could have attempted to implement insertion points in just the case of async scripts in this patch, however I'd rather keep this patch safer and implement insertion points separately.
Comment 12 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-11-09 13:03:31 PST
Comment on attachment 411027 [details] [diff] [review]
Patch to fix

Ok.
Could you still make sure we handle dynamic changes to async and defer attribute in a reasonable way (and according to the HTML5, though the draft isn't totally clear with those).
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-09 15:08:06 PST
Comment on attachment 411027 [details] [diff] [review]
Patch to fix

Looks like the new nsIDOMNSHTMLScriptElement.idl file is missing from the patch, and nsIScriptElement's IID should be bumped. The rest looks good!

sr=jst with that fixed.
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-09 17:00:20 PST
Created attachment 411325 [details] [diff] [review]
Fix review comments and add more tests

Fixes review comments from jst and adds more tests. Also fixes one bug that I found, I had forgotten to add the new interface to the interface table for nsHTMLScriptElement.
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-09 18:27:12 PST
Checked in to trunk

http://hg.mozilla.org/mozilla-central/rev/53689357009f
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-09 18:32:22 PST
Created attachment 411347 [details] [diff] [review]
Branch patch
Comment 17 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-10 11:32:37 PST
Comment on attachment 411347 [details] [diff] [review]
Branch patch

a192=beltzner

As per today's meeting, let's get this baking on 192 ASAP and make sure our friends who plan to use it are using some form of nightly build.
Comment 18 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-11 00:58:44 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/de2d52fb0fb9

Landed on the 1.9.2 branch. Including bug 527870 and 527623.

I ended up disabling one of the tests. I strongly suspect that the speculative parsing code sometimes end up fetching a script-url twice, (though only evaluating it once). This is normally fine but tripped up my test since it was written to test precise timing and fetching interactions.

I'm fairly certain that this wasn't caused by my patch since the script that was sometimes fetched twice was a non-async script and so shouldn't be affected by my patch.

We currently don't have any automated tests on speculative parsing which also makes me more ok with blaming the existing code. Also, under normal circumstances the failure wouldn't be detected by pages since just performing an extra GET (which probably often just hits our http cache) won't be noticed.

I'll make sure to file a bug on getting speculative parser tests which should detect this problem
Comment 19 Eric Shepherd [:sheppy] 2009-11-11 14:12:31 PST
Added async to the attribute list here:

https://developer.mozilla.org/en/html/element/script
Comment 20 Emil Ivanov 2010-01-31 15:32:43 PST
I see on the release notes that this is supported, but I don't see it in https://developer.mozilla.org/En/Firefox_3.6_for_developers

Also I think it will be better if the release notes links to https://developer.mozilla.org/en/html/element/script instead to this bug. It will be easier to find the information.
Comment 21 Jonas Sicking (:sicking) PTO Until July 5th 2010-01-31 19:23:51 PST
I updated the Firefox3.6 for devs page (it's a wiki, anyone can). I don't have access to edit the release notes.
Comment 22 mazhar md 2014-11-19 21:56:02 PST
As per my research the updated async added at developer.mozilla.org promising.

But HTML 5 handling the document.write() flow was still confusing me. I see it as working similar to last version.
w3.org listed a similar kind of issues here 

http://lists.w3.org/Archives/Public/html-tidy/2014OctDec/0001.html

Where the html tidy throws some warnings.

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