Closed Bug 461555 Opened 16 years ago Closed 16 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.1b3

People

(Reporter: stephend, Assigned: sicking)

References

(
URL
)

Details

(Keywords: regression, testcase, verified1.9.1)

Attachments

(7 files, 1 obsolete file)

Logfile
326.58 KB, text/plain
Details
javascript file for testcase
58 bytes, text/javascript
Details
testcase
254 bytes, text/html
Details
extended testcase
466 bytes, text/html
Details
Minimal testcase
275 bytes, text/html
Details
Fire DOMContentLoaded after deferred scripts
14.94 KB, patch
mrbkap
: review+
mrbkap
: superreview+
Details | Diff | Splinter Review
Fix
3.06 KB, patch
mrbkap
: review+
mrbkap
: superreview+
Details | Diff | Splinter Review 
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">
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.
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.

    
    

    
  

      
      
      
      

        Attached file
          
        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...

    
  
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?

    
    

    
  
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")

    
    

    
  

      
      
      
      

        Attached file
          
        extended testcase
        — 
      

    


  
This shows the difference with IE8.
Basically, the page uses swfobject, which sets innerHTML, and somehow that makes the difference.

    
    

    
  

      
      
      
      

        Attached file
          
        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.

    
    

    
  
I also wonder if this is the same bug that seems to prevent http://www.ford.com from loading sometimes as well?
Target Milestone: --- → mozilla1.9.1b3

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch to fix (obsolete)
        — 
          — Splinter Review
      

    


  
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)

        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.

    
  
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

    
    

    
  
Progress on this, Jonas?

    
    

    
  
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.

        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.

    
    

    
  


  
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)

        Attachment #358783 -
        Flags: superreview?(mrbkap)

        Attachment #358783 -
        Flags: superreview+

        Attachment #358783 -
        Flags: review?(mrbkap)

        Attachment #358783 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix
        — 
          — Splinter Review
      

    


  
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)

        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
Closed: 16 years ago
Resolution: --- → FIXED

    
    

    
  
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+
No longer depends on: 494874

    
    

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

    
    

    
  
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)

    
    

    
  
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).

    
    

    
  
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).
Component: DOM → DOM: Core & HTML

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: