543062 - scripts loaded using document.write aren't loaded in parallel

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P2)

Description

[Steve Souders]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6

Newer browsers (IE8, Saf4, Chr2+, and FF 3.5+) support downloading scripts in parallel when they're loaded the normal HTML way: script src="main.js".

Many pieces of 3rd party content (ads, widgets, analytics) use document.write to load scripts: document.write('<script src="main.js" etc'). If two scripts are loaded this way, FF 3.6 does NOT download them in parallel, whereas the other browsers mentioned above do.

Here's a test case: http://stevesouders.com/cuzillion/?c0=hj1wfff2_0_f&c1=hj1wfff2_0_f

I emailed Jonas about this and he asked me to create this bug and assign it to him.

Reproducible: Always

Steps to Reproduce:
1. go to http://stevesouders.com/cuzillion/?c0=hj1wfff2_0_f&c1=hj1wfff2_0_f
2. notice that the load time of the two scripts are 2 seconds apart - if they had been downloaded in parallel the times would be nearly equal
3. you can also watch in Net Panel and see the HTTP requests going off sequentially
Actual Results:  
two scripts are downloaded sequentially

Expected Results:  
they are downloaded in parallel

Comment 1

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

Seems like this also effects the HTML5 parser.

Comment 2

[Henri Sivonen (:hsivonen)]

So this bug is asking for the pending document.written stream to be speculatively scanned for script URLs, right?

Comment 3

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

So doesn't

document.write("<script src=A></s" + "cript><script src=B></s" + "cript>");

insert both scripts into the DOM before the doc.write returns? Or do we just parse up to the first one and then resuming parsing once the first script has downloaded and executed?

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to comment #3)
> So doesn't
> 
> document.write("<script src=A></s" + "cript><script src=B></s" + "cript>");
> 
> insert both scripts into the DOM before the doc.write returns? Or do we just
> parse up to the first one and then resuming parsing once the first script has
> downloaded and executed?

Per HTML5, the whole string goes into the input stream, then the parser runs to the end of the first end tag, blocks and document.write returns. The second script is parsed when the first script has been executed.

Comment 5

[Henri Sivonen (:hsivonen)]

I've recently seen JS code that browser sniffs and does unsafe stuff in Gecko while running a safe document.write()-based code path in WebKit and IE. I'm guessing people do that in order to work around this bug to get parallel downloads.

I'm wondering if it made sense to try to squeeze a fix for this bug in Firefox 4 by fixing this in a way that causes the tail of document.written content to be parsed twice. Fixing this in a way that avoids reparsing seems too risky.

We'd have a much nicer evang story for the scripts that now browser sniff if this bug was fixed.

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: First attempt at a fix

sicking, does this patch look like something that we could put in Firefox 4 (if I write a test)?

I realize this is a bit feature-y, but after noticing that some sites (Live.com calendar and OpenLayers) use HTML5-incompatible script loading in Gecko apparently in order to get parallel downloads, I think this should be treated as a perf regression fix in the situation where the sites put us on their cross-browser-compatible document.write code path because the old parallel load hack no longer works in Firefox 4.

Note that the tail of document.written content (the part that gets parsed asynchronously) gets parsed twice, but I think that's OK, because avoiding the duplicate parsing would be considerably more tricky than just doing what this patch does.

That is:
document.write("This is parsed once<script src='...'></script>This is parsed twice.");

Or:
document.write("This is parsed once<script src='...'></script>"); document.write("This is parsed twice.");

Comment 7

[Henri Sivonen (:hsivonen)]

FWIW, IE9 downloads multiple document.written scripts in parallel.

Comment 8

[Henri Sivonen (:hsivonen)]

Per discussion with jst, I'll pursue fixing this for Firefox 4.

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: Pre-parse document.written content that doesn't get parsed synchronously

Comment 10

[Henri Sivonen (:hsivonen)]

Nominating as a blocker due to the reason given in comment 5.

Comment 11

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

Comment on attachment 491173 [details] [diff] [review]
Pre-parse document.written content that doesn't get parsed synchronously

I can't really judge the risk here, so I'll leave the decision to others as to if we should take this patch or not.

The patch looks good as far as I can tell. The only thing is that the the "timeout" capability in the test isn't really needed. It's ok to have a test fail by timing out, that's why we have the timeout feature in the test runner.

So r=me with that removed (rerequest if you think it's needed)

Comment 12

[Johnny Stenback (:jst)]

I think we should take this optimization ASAP and get as much testing as we can on it. Blocking beta8 for that reason.

Comment 13

[Henri Sivonen (:hsivonen)]

Landed with the test's timeout capability removed:
http://hg.mozilla.org/mozilla-central/rev/fe9637495f97