Closed Bug 543062 Opened 10 years ago Closed 9 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: steve, Assigned: hsivonen)

Details

(Whiteboard: [wanted1.9.3][parity-ie])

Attachments

(1 file, 1 obsolete file)

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
Seems like this also effects the HTML5 parser.
Component: General → DOM
Product: Firefox → Core
QA Contact: general → general
Whiteboard: [wanted1.9.3]
So this bug is asking for the pending document.written stream to be speculatively scanned for script URLs, right?
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
Component: DOM → HTML: Parser
OS: Windows XP → All
QA Contact: general → parser
Hardware: x86 → All
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.
Attached patch First attempt at a fix (obsolete) — Splinter Review
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.");
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #490859 - Flags: feedback?(jonas)
FWIW, IE9 downloads multiple document.written scripts in parallel.
Whiteboard: [wanted1.9.3] → [wanted1.9.3][parity-ie]
Per discussion with jst, I'll pursue fixing this for Firefox 4.
Priority: -- → P2
Nominating as a blocker due to the reason given in comment 5.
blocking2.0: --- → ?
Attachment #491173 - Flags: review?(jonas)
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)
Attachment #491173 - Flags: review?(jonas) → review+
I think we should take this optimization ASAP and get as much testing as we can on it. Blocking beta8 for that reason.
blocking2.0: ? → beta8+
Landed with the test's timeout capability removed:
http://hg.mozilla.org/mozilla-central/rev/fe9637495f97
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.