document.write in <script defer> should not blank and replace the page

VERIFIED FIXED in mozilla1.9.1b3

Status

()

Core
DOM
P1
critical
VERIFIED FIXED
9 years ago
5 years ago

People

(Reporter: stephend, Assigned: sicking)

Tracking

({regression, testcase, verified1.9.1})

Trunk
mozilla1.9.1b3
regression, testcase, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(7 attachments, 1 obsolete attachment)

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

9 years ago
Is it possible that this happens because of some UA sniffing.
Or if not, what is the regression range?
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.
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.
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.
Created attachment 344726 [details]
Logfile
Flags: blocking1.9.1?
I'm not 100% sure, but I believe this is due to a document.write() call after onload.
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
Jonas, could this be fallout from bug 28293?
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.
Blocks: 28293
The page <http://www.merck.com> is invalid (67 Errors, 33 warnings)
Christopher, I think we are agreed about that. The question is, what id causing this, and why, and should it?
Keywords: testcase-wanted
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.
@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".
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.
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

9 years ago
Created attachment 345534 [details]
javascript file for testcase

Comment 17

9 years ago
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...

Updated

9 years ago
Keywords: testcase-wanted → testcase
(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

9 years ago
Christopher: No. Neither of those changes causes the page to stop loading.
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?
(sorry, that should of course say, "why IE works but FF3.1 doesn't")

Comment 22

9 years ago
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.
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.
Assignee: nobody → jonas
Flags: blocking1.9.1? → blocking1.9.1+
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.
(In reply to comment #24)
See
http://www.whatwg.org/specs/web-apps/current-work/#dom-document-write-html
I also wonder if this is the same bug that seems to prevent http://www.ford.com from loading sometimes as well?
Priority: -- → P1

Updated

9 years ago
Target Milestone: --- → mozilla1.9.1b3
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?
Attachment #351633 - Flags: superreview?(mrbkap)
Attachment #351633 - Flags: review?(mrbkap)

Updated

9 years ago
Attachment #351633 - Attachment is patch: true
Attachment #351633 - Attachment mime type: application/octet-stream → text/plain
(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.
Blocks: 461724

Updated

9 years ago
Summary: Merck.com renders, then loads a blank page and appears to never finish loading → document.write in <script defer> should not blank and replace the page
Blocks: 469751

Updated

9 years ago
Blocks: 469769
Duplicate of this bug: 457679
Progress on this, Jonas?

Comment 31

9 years ago
I've created bug 473361. It's been mentioned that it could be this bug, but in myfax.com.
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.

Updated

9 years ago
Attachment #351633 - Flags: superreview?(mrbkap)
Attachment #351633 - Flags: superreview+
Attachment #351633 - Flags: review?(mrbkap)
Attachment #351633 - Flags: review+
This was backed out for failing unit tests.
Thanks ben!

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1231982992.1231986068.20508.gz&fulltext=1

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1231982992.1231987385.22582.gz&fulltext=1
Still leaks :(

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1232748032.1232753994.16137.gz
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.
Attachment #351633 - Attachment is obsolete: true
Attachment #358783 - Flags: superreview?(mrbkap)
Attachment #358783 - Flags: review?(mrbkap)

Updated

8 years ago
Attachment #358783 - Flags: superreview?(mrbkap)
Attachment #358783 - Flags: superreview+
Attachment #358783 - Flags: review?(mrbkap)
Attachment #358783 - Flags: review+
Created attachment 358981 [details] [diff] [review]
Fix

Fix on top of previous one. This should fix mochitest failures as well as leaks.
Attachment #358981 - Flags: superreview?(mrbkap)
Attachment #358981 - Flags: review?(mrbkap)

Updated

8 years ago
Attachment #358981 - Flags: superreview?(mrbkap)
Attachment #358981 - Flags: superreview+
Attachment #358981 - Flags: review?(mrbkap)
Attachment #358981 - Flags: review+
Comment on attachment 358981 [details] [diff] [review]
Fix

Add a comment about the mInternalState check and r+sr=me.
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)
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Keywords: fixed1.9.1
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.
Keywords: fixed1.9.1 → verified1.9.1
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
Status: RESOLVED → VERIFIED
Thanks for finding and verifying this Stephen!
Depends on: 478889
It looks like this fix broke deferred scripts in XHTML (as in, the document won't parse at all).  See bug 478889.
Flags: in-testsuite?
Tests are in the patch
Flags: in-testsuite? → in-testsuite+
Depends on: 494874
No longer depends on: 494874
Depends on: 494874

Comment 45

8 years ago
This bug is back on the latest trunk builds... manifesting itself on dabs.com causing the homepage to redirect to verisign....

Comment 46

8 years ago
Probably because of bug 518104.  Given our experience with this bug, I'm surprised the spec says what it does.
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

8 years ago
dabs.com works fine for me using Firefox trunk on Mac, fwiw.
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

8 years ago
Yes, it gets blanked if I set html5.enable to true.
That's probably a wholly unrelated to this bug then. Please file a separate bug.
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".
"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).
You need to log in before you can comment on or make changes to this bug.