Last Comment Bug 461555 - document.write in <script defer> should not blank and replace the page
: document.write in <script defer> should not blank and replace the page
Status: VERIFIED FIXED
: regression, testcase, verified1.9.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P1 critical with 2 votes (vote)
: mozilla1.9.1b3
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
:
:
Mentors:
http://www.merck.com
: 457679 (view as bug list)
Depends on: 478889 494874
Blocks: 28293 461724 469751 469769
  Show dependency treegraph
 
Reported: 2008-10-24 11:30 PDT by Stephen Donner [:stephend]
Modified: 2012-04-25 11:51 PDT (History)
24 users (show)
jonas: blocking1.9.1+
jonas: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Logfile (326.58 KB, text/plain)
2008-10-25 01:03 PDT, Stephen Donner [:stephend]
no flags Details
javascript file for testcase (58 bytes, text/javascript)
2008-10-30 11:53 PDT, Sander
no flags Details
testcase (254 bytes, text/html)
2008-10-30 12:00 PDT, Sander
no flags Details
extended testcase (466 bytes, text/html)
2008-10-30 15:31 PDT, Sander
no flags Details
Minimal testcase (275 bytes, text/html)
2008-10-30 16:29 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details
Patch to fix (20.19 KB, patch)
2008-12-05 16:33 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
mrbkap: review+
mrbkap: superreview+
Details | Diff | Splinter Review
Fire DOMContentLoaded after deferred scripts (14.94 KB, patch)
2009-01-25 16:43 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
mrbkap: review+
mrbkap: superreview+
Details | Diff | Splinter Review
Fix (3.06 KB, patch)
2009-01-26 18:48 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
mrbkap: review+
mrbkap: superreview+
Details | Diff | Splinter Review

Description Stephen Donner [:stephend] 2008-10-24 11:30:27 PDT
Summary: Merck.com renders, then loads a blank page and appears to never finish loading

This works fine in 3.0.x

Steps to Reproduce:

1. Using trunk, load http://www.merck.com

Expected Results:

Merck.com finishes loading

Actual Results:

Initially Merck.com renders, but then flashes to a blank page and never finishes loading

Error: syntax error
Source File: http://www.merck.com/screenresolution.html?width=1920&height=1200
Line: 1, Column: 62
Source Code:
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
Comment 1 Olli Pettay [:smaug] 2008-10-24 12:10:03 PDT
Is it possible that this happens because of some UA sniffing.
Or if not, what is the regression range?
Comment 2 Stephen Donner [:stephend] 2008-10-24 13:46:50 PDT
I set |general.useragent.override| to Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3, and it still doesn't work.

Sadly, I don't really have time right now to find a regression range, but maybe later tonight.

I do have an HTTP log, but not sure if that would be helpful.
Comment 3 Tyler Downer [:Tyler] 2008-10-24 15:21:26 PDT
I am seeing this with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081024 Minefield/3.1b2pre. Funny thing is, it says "Stopped" in the toolbar, and yet the working animation is still going. I am also getting a lot of 
Error: return not in function
Source File: javascript:%20return%20false;
Line: 1, Column: 1
Source Code:
 return false;

In the Error console, with some other junk. Can provide the rest if wanted.
Comment 4 Stephen Donner [:stephend] 2008-10-25 00:59:46 PDT
Based on the following snippet, I'm not sure this is in the correct component, but hopefully biesi or someone else would know.

0[725140]: nsHttpResponseHead::MustValidate ??
0[725140]: no mandatory validation requirement
0[725140]: Not validating based on expiration time
0[725140]: CheckCache [this=2598f10 doValidation=0]
0[725140]: nsHttpChannel::ReadFromCache [this=2598f10] Using cached copy of: http://www.merck.com/scripts/jquery121.js
0[725140]: nsHttpConnectionMgr::RescheduleTransaction [trans=33eed30 10]
0[725140]: STS dispatch [19f5c60]
0[725140]: nsHttpChannel::Cancel [this=25986a0 status=804b0002]
0[725140]: nsHttpConnectionMgr::CancelTransaction [trans=33eed30 reason=804b0002]
0[725140]: STS dispatch [19f5e80]
0[725140]: nsHttpConnectionMgr::RescheduleTransaction [trans=33ed3e0 10]
0[725140]: STS dispatch [19f5fc0]
0[725140]: nsHttpChannel::Cancel [this=2598340 status=804b0002]
0[725140]: nsHttpConnectionMgr::CancelTransaction [trans=33ed3e0 reason=804b0002]
...

There are a bunch of repeating STS dispatch -> nsHttpChannel:Cancel -> nsHttpConnectionMgr:CancelTransaction calls.
Comment 5 Stephen Donner [:stephend] 2008-10-25 01:03:31 PDT
Created attachment 344726 [details]
Logfile
Comment 6 Christian :Biesinger (don't email me, ping me on IRC) 2008-10-25 05:35:18 PDT
I'm not 100% sure, but I believe this is due to a document.write() call after onload.
Comment 7 Stephen Donner [:stephend] 2008-10-25 13:55:25 PDT
Fails: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/2008080504 Minefield/3.1a2pre
Works: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/2008080403 Minefield/3.1a2pre works
Comment 8 Stephen Donner [:stephend] 2008-10-25 14:19:32 PDT
Jonas, could this be fallout from bug 28293?
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-10-25 16:26:49 PDT
It sounds plausible given the description of what happens. But I don't see any elements with the 'defer' attribute set. There might be one hidden deep in there somewhere though.
Comment 10 Christopher Yeleighton 2008-10-28 12:07:13 PDT
The page <http://www.merck.com> is invalid (67 Errors, 33 warnings)
Comment 11 Tyler Downer [:Tyler] 2008-10-28 16:26:02 PDT
Christopher, I think we are agreed about that. The question is, what id causing this, and why, and should it?
Comment 12 Christopher Yeleighton 2008-10-28 16:34:05 PDT
I think the reason is that Merck.com has got sloppy Web developers.  If anyone at Merck takes time to clean up that mess and Firefox still blanks, it would be an interesting thing to investigate.  But examining that stinking dead corpse of a HTML page?  I am not volunteering.
Comment 13 Tyler Downer [:Tyler] 2008-10-28 17:01:06 PDT
@Christopher, first, I know that the Merck website is a mess. However, the question is not, "Does Merck need to get better devs", but "Should, based on that Code, Firefox be showing a blank page".
Comment 14 Christopher Yeleighton 2008-10-28 18:20:58 PDT
The script <http://www.merck.com/scripts/screenres.js> is called early, its actual purpose is unknown.  It seems to do nothing except that it makes an XMLHttpRequest to the page the trunk version ends up displaying.  I cannot see any connection to the DEFER functionality here.
HTH.
Comment 15 Tyler Downer [:Tyler] 2008-10-28 18:25:45 PDT
Maybe someone should volunteer to contact the Merck developers to ask them on what their take is on the situation. Maybe Stephen since he was the reporter, and presumably was using the site. This could end up being moved over to Tech Evangelism if necessary.
Comment 16 Sander 2008-10-30 11:53:19 PDT
Created attachment 345534 [details]
javascript file for testcase
Comment 17 Sander 2008-10-30 12:00:15 PDT
Created attachment 345535 [details]
testcase

So I created this testcase from the HTML I get when I do "save page". Which looks distinctly different from what I see with just view source. E.g. view source doesn't show any deferred script elements, but there's four of them in the saved source.
Still, I'm guessing that for the testcase itself it doesn't matter...
Comment 18 Christopher Yeleighton 2008-10-30 12:12:48 PDT
(In reply to comment #17)
> Created an attachment (id=345535) [details]
> testcase
> 

Does the page stop loading when the deferred script:
a) tries to write a META element into the HEAD?
b) tries to write a DIV element into the BODY?
Comment 19 Sander 2008-10-30 13:18:06 PDT
Christopher: No. Neither of those changes causes the page to stop loading.
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-10-30 14:08:53 PDT
Sander: Thanks for the testcase! This helps tremendously.

However it looks like IE8 is doing the same thing that we are doing on the testcase. Any idea why merck.com works while FF3.1 doesn't?
Comment 21 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-10-30 14:51:24 PDT
(sorry, that should of course say, "why IE works but FF3.1 doesn't")
Comment 22 Sander 2008-10-30 15:31:29 PDT
Created attachment 345593 [details]
extended testcase

This shows the difference with IE8.
Basically, the page uses swfobject, which sets innerHTML, and somehow that makes the difference.
Comment 23 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-10-30 16:29:57 PDT
Created attachment 345613 [details]
Minimal testcase

Wow, that is freaky. Attaching a minimal singlefile testcase.

Talked with jst and what we'll do is to move execution of deferred scripts to before we drop the parser so that document.write always result in things getting appended to the end, rather than replace the page.

Thanks a ton for creating the testcase Sander! Saves me a lot of time.
Comment 24 Christopher Yeleighton 2008-10-31 01:45:17 PDT
In any case, I would expect document.write to a closed document to silently fail, not reopen, IMHO.  The current Firefox behavior <https://developer.mozilla.org/en/DOM/document.write> is an extension to the standard where this case is undefined.
Comment 25 Olli Pettay [:smaug] 2008-10-31 01:48:42 PDT
(In reply to comment #24)
See
http://www.whatwg.org/specs/web-apps/current-work/#dom-document-write-html
Comment 26 Stephen Donner [:stephend] 2008-12-02 16:35:18 PST
I also wonder if this is the same bug that seems to prevent http://www.ford.com from loading sometimes as well?
Comment 27 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-12-05 16:33:14 PST
Created attachment 351633 [details] [diff] [review]
Patch to fix

This should do it.

The idea is to delay calling DidBuildModel until all deferred scripts have finished running. I also decided to defer it until all pending scripts have run, so that if someone does something like

<script>
  s = document.createElement('script');
  s.src = "data:text/plain,document.write('foo');";
  document.body.appendChild(s);
</script>

there won't be a race condition which might blow away the whole document.

One concern with the patch is, is it safe for the parser to run scripts at the point when ReadyToCallDidBuildModel is called?
Comment 28 Blake Kaplan (:mrbkap) 2008-12-10 17:03:15 PST
(In reply to comment #27)
> One concern with the patch is, is it safe for the parser to run scripts at the
> point when ReadyToCallDidBuildModel is called?

I think it's safe for the parser, but it appears that we started firing DOMContentLoaded asynchronously for bug 344305, comment 18.
Comment 29 Nochum Sossonko [:Natch] 2008-12-30 15:39:10 PST
*** Bug 457679 has been marked as a duplicate of this bug. ***
Comment 30 Damon Sicore (:damons) 2009-01-09 15:36:35 PST
Progress on this, Jonas?
Comment 31 Jose Fandos 2009-01-13 13:30:56 PST
I've created bug 473361. It's been mentioned that it could be this bug, but in myfax.com.
Comment 32 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-01-13 23:38:33 PST
Blake: I talked with bz and it seems like the better fix for bug 344305 is to improve SetupNewViewer instead since there are more places where stopping a channel can cause scripts to execute.
Comment 33 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-01-14 18:26:42 PST
This was backed out for failing unit tests.
Comment 35 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-01-23 16:24:19 PST
Still leaks :(

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1232748032.1232753994.16137.gz
Comment 36 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-01-25 16:43:44 PST
Created attachment 358783 [details] [diff] [review]
Fire DOMContentLoaded after deferred scripts

This fires DOMContentLoaded at the same place it used to, after the called to EndLoad. This means that it'll fire after all deferred scripts have executed.
Comment 37 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-01-26 18:48:21 PST
Created attachment 358981 [details] [diff] [review]
Fix

Fix on top of previous one. This should fix mochitest failures as well as leaks.
Comment 38 Blake Kaplan (:mrbkap) 2009-01-26 18:49:40 PST
Comment on attachment 358981 [details] [diff] [review]
Fix

Add a comment about the mInternalState check and r+sr=me.
Comment 39 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-01-27 01:35:35 PST
This is IN! Woot!

http://hg.mozilla.org/mozilla-central/rev/d524c8d6a4fb (main patch)
http://hg.mozilla.org/mozilla-central/rev/e43a978789a9 (fix additional problem)
Comment 40 Stephen Donner [:stephend] 2009-01-28 09:08:18 PST
Verified FIXED on 1.9.1: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b3pre) Gecko/20090128 Shiretoko/3.1b3pre

Replacing fixed1.9.1 keyword with verified1.9.1.
Comment 41 Stephen Donner [:stephend] 2009-01-28 09:12:13 PST
Also verified as fixed on trunk:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090128 Minefield/3.2a1pre
Comment 42 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-01-28 10:20:03 PST
Thanks for finding and verifying this Stephen!
Comment 43 Boris Zbarsky [:bz] (still a bit busy) 2009-02-17 08:38:30 PST
It looks like this fix broke deferred scripts in XHTML (as in, the document won't parse at all).  See bug 478889.
Comment 44 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-03-01 12:22:25 PST
Tests are in the patch
Comment 45 James [silentj] 2009-11-09 01:53:59 PST
This bug is back on the latest trunk builds... manifesting itself on dabs.com causing the homepage to redirect to verisign....
Comment 46 Jesse Ruderman 2009-11-09 11:49:18 PST
Probably because of bug 518104.  Given our experience with this bug, I'm surprised the spec says what it does.
Comment 47 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-11-09 11:53:14 PST
Can you please file a new bug. It would be great to have a minimized testcase. We are behaving fairly close to IE now (though not exactly like IE since IE is crazy)
Comment 48 Jesse Ruderman 2009-11-09 12:08:38 PST
dabs.com works fine for me using Firefox trunk on Mac, fwiw.
Comment 49 Nochum Sossonko [:Natch] 2009-11-09 12:10:46 PST
You might have to enable html5 (html5.enable) but it certainly doesn't work for me (page gets "blanked" after a little while of loading).
Comment 50 Jesse Ruderman 2009-11-09 12:13:34 PST
Yes, it gets blanked if I set html5.enable to true.
Comment 51 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-11-09 12:23:36 PST
That's probably a wholly unrelated to this bug then. Please file a separate bug.
Comment 52 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2012-04-17 23:58:00 PDT
sicking, what's the reason for not setting readyState to "interactive" when the parser is aborted? I see no spec-based justification for going straight from "loading" to "complete".
Comment 53 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-04-25 11:51:46 PDT
"loading" means that we're still downloading the main document and will eventually fire both "DOMContentLoaded" as well as "load" events.

"interactive" means that we're still downloading resources that the document depends on and will eventually fire a "load" event.

"complete" means that no more downloading is happening and that no more events will be fired.

Out of these I think "complete" seems to be the best fit as to what state we're in after aborting. Possibly we could mint a new "aborted" and/or "error" state but I haven't thought through if that would add more confusion than it would solve.

Either way it seems like this bug is a bad place to discuss this. Maybe start a whatwg thread? (though I think you might have already done so).

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