Closed Bug 503481 Opened 15 years ago Closed 15 years ago

Implement async attribute of script element

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a1
Tracking Flags:
Tracking Status
status1.9.2 --- beta3-fixed

People

(Reporter: smaug, Assigned: sicking)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

Fix review comments and add more tests
27.97 KB, patch
sicking
: review+
sicking
: superreview+
Details | Diff | Splinter Review
Branch patch
27.98 KB, patch
beltzner
: approval1.9.2+
Details | Diff | Splinter Review 
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.
Reminder: If we ever implement this, async scripts should not run code for permitting document.write() (upcoming code in nsIScriptElement per my current thinking).
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.
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.
Attached patch Patch to fix (obsolete) — — Splinter Review 
This implements the async attribute.
Assignee: nobody → jonas
Status: NEW → ASSIGNED
Attachment #411027 - Flags: superreview?(jst)
Attachment #411027 - Flags: review?(mrbkap)
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.
Attachment #411027 - Flags: review?(Olli.Pettay)
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+.
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?
(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)?
(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.
(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
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 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).
Attachment #411027 - Flags: review?(Olli.Pettay) → review+
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.
Attachment #411027 - Flags: superreview?(jst) → superreview+
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.
Attachment #411027 - Attachment is obsolete: true
Attachment #411325 - Flags: superreview+
Attachment #411325 - Flags: review+
Attachment #411027 - Flags: review?(mrbkap)
Checked in to trunk

http://hg.mozilla.org/mozilla-central/rev/53689357009f
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 527623
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.
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
Depends on: 527870
status1.9.2: --- → final-fixed
Keywords: dev-doc-needed
Summary: Investigate if async attribute of script element should be implemented → Implement async attribute of script element
Target Milestone: --- → mozilla1.9.3a1
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.
I updated the Firefox3.6 for devs page (it's a wiki, anyone can). I don't have access to edit the release notes.
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.
Flags: needinfo?(cigafuriz)
Restrict Comments: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: