Closed Bug 581240 Opened 14 years ago Closed 10 years ago

onload doesn't fire, document.body is null with iframe[scrolling="no" src="empty-page"], when its 'display' changes from none quickly followed by dynamically document.write'ing a <script src=>

Categories

(Core :: General, defect, P1)

Product:
Core
Component:
General
Version:
1.9.2 Branch
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
blocking2.0 --- .x+
blocking1.9.2 --- needed
status1.9.2 --- wanted
blocking1.9.1 --- -
status1.9.1 --- unaffected

People

(Reporter: asqueella, Assigned: bzbarsky)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files)

testcase
623 bytes, text/html
Details
581240.htm
659 bytes, text/html
Details 
<script>//<!--
function onload() {
    document.getElementById("preview").style.display = "";
    document.getElementById("preview").innerHTML = ('<iframe src="data:text/html," scrolling="no"></iframe>');
    var iframe = document.getElementById("preview").firstChild;
    var doc = iframe.contentDocument;
    doc.write("<script src='data:text/javascript,'></script><script>function onload() {alert('PASS')}</" + "script>");
    doc.close();
}

//-->
</script>
<body>
    You should see an alert saying "PASS". Firefox 3.6.7, however, doesn't show the alert.
    <div id="preview" style="display:none">
    </div>
</body>

This regressed in 3.6.7, more specifically:
works: 2010-06-11-03-mozilla-1.9.2.app
fails: 2010-06-12-03-mozilla-1.9.2.app

pointing to http://hg.mozilla.org/releases/mozilla-1.9.2/pushloghtml?fromchange=9716c797df5b&tochange=5a4db3e2a4e4

...of which I guess bug 550882 is indirectly responsible, since removing scrolling=no makes the testcase fail in older 1.9.2 builds. Trunk handles both the case with scrolling=no and the case without the attribute correctly.

This is reduced from a real (wiki) app. The code in the app shows dynamically-generated preview when the user switches in-page tabs.
Attached file testcase — 

    
    

    
  
With a testcase that doesn't have scrolling="no", this got fixed on m-c in this range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fc3befa99ab7&tochange=981132d2f5d6

Sadly, I bet that it was fixed by lazy frame construction.

That same testcase originally broke on m-c in this range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bd35ca5e194a&tochange=b5359ab6a52c

I'm guessing that bug 477880 was responsible somehow.... not sure how yet.
Blocks: 477880

    
    

    
  
OK.  I have verified, via local backout, that bug 477880 was in fact the thing that caused it.

Still looking into how this _used_ to work, but with it broken it looks like there are two requests in flight in the subframe: the document request and the src script load.  The latter gets taken out of the loadgroup when Close() is called.  The former is taken out when the script finishes loading, which is _before_ the second script runs.  And its the second script that sets up the load lister.  So we fire onload before the listener is set up, which is what loses.

For some reason, our onload blocker is not in the loadgroup.  Looking into why.

    
    

    
  
Oh, this used to work because nsDocument::EndLoad fired and the onload blocker _was_ in the loadgroup at that point.

    
    

    
  
OK, here's the issue.  We're adding the onload blocker for the document (the one from BeginLoad()) while the loadgroup is canceling, so it never gets added.  The relevant stack:

#0  nsLoadGroup::AddRequest (this=0x1dfb14b0, request=0x1df20590, ctxt=0x0) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/netwerk/base/src/nsLoadGroup.cpp:522
#1  0x1a38fc18 in nsDocument::BlockOnload (this=0x16c2200) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/content/base/src/nsDocument.cpp:7095
#2  0x1a388e27 in nsDocument::AsyncBlockOnload (this=0x16c2200) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/content/base/src/nsDocument.cpp:7068
#3  0x1a3a8cfa in nsRunnableMethod<nsDocument, void>::Run (this=0x1d720d20) at nsThreadUtils.h:282
#4  0x1a35be6c in nsContentUtils::RemoveScriptBlocker () at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/content/base/src/nsContentUtils.cpp:4494
#5  0x1a0740a3 in nsAutoScriptBlocker::~nsAutoScriptBlocker (this=0xbfffa6db) at nsContentUtils.h:1676
#6  0x1a0e94bc in PresShell::FlushPendingNotifications (this=0x1d71f910, aType=Flush_Layout) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/layout/base/nsPresShell.cpp:4880
#7  0x1a38aff4 in nsDocument::FlushPendingNotifications (this=0x1726a00, aType=Flush_Layout) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/content/base/src/nsDocument.cpp:6398
#8  0x1a38afb5 in nsDocument::FlushPendingNotifications (this=0x16c2200, aType=Flush_Layout) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/content/base/src/nsDocument.cpp:6392
#9  0x00d506d3 in nsDocLoader::DocLoaderIsEmpty (this=0x1df326d0, aFlushLayout=1) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/uriloader/base/nsDocLoader.cpp:756
#10 0x00d50fd1 in nsDocLoader::OnStopRequest (this=0x1df326d0, aRequest=0x1df22050, aCtxt=0x0, aStatus=2152398850) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/uriloader/base/nsDocLoader.cpp:697
#11 0x11829c6e in nsLoadGroup::RemoveRequest (this=0x1dfb14b0, request=0x1df22050, ctxt=0x0, aStatus=2152398850) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/netwerk/base/src/nsLoadGroup.cpp:680
#12 0x1182a755 in nsLoadGroup::Cancel (this=0x1dfb14b0, status=2152398850) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/netwerk/base/src/nsLoadGroup.cpp:331
#13 0x00d509cf in nsDocLoader::Stop (this=0x1df326d0) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/uriloader/base/nsDocLoader.cpp:328
#14 0x00d281b3 in nsDocShell::Stop (this=0x1df326d0) at nsDocShell.h:256
#15 0x00d01b9a in nsDocShell::Stop (this=0x1df326d0, aStopFlags=1) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/docshell/base/nsDocShell.cpp:3968
#16 0x1a52bd4b in nsHTMLDocument::OpenCommon (this=0x16c2200, aContentType=@0xbfffad04, aReplace=0) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/content/html/document/src/nsHTMLDocument.cpp:1892

    
    

    
  
Or more precisely we add an earlier blocker that way.... and ignore the failure to add to the loadgroup.  Then when BeginLoad happens the block count is 2 so we don't try to add the blocker again.

The scrollbars matter because the thing that's blocking load that early is the xbl binding stuff for the scrollbars.

    
    

    
  
I'm really not sure yet how to fix this on the document end....  Ideally, loadgroup would just deal with new loads being added while it's canceling or something.
blocking1.9.1: --- → -
blocking1.9.2: ? → .9+

          status1.9.1:
          --- → unaffected

          status1.9.2:
          ? → wanted

    
  
Blocks: 581336

    
    

    
  
Nickolay, was the wiki app in question "Confluence"?

    
    

    
  
Yeah.

    
    

    
  
Perfect, thanks.

    
  
No longer blocks: 581336

    
  
Assignee: nobody → bzbarsky

    
  
Priority: -- → P1

    
    

    
  
I'm going to punt this to 1.9.2.10. We want a fix for this as it is a regression, but we wouldn't hold up the security fixes for it.
blocking1.9.2: .9+ → .10+

    
  
blocking2.0: --- → ?

    
  
blocking2.0: ? → betaN+

    
  
blocking1.9.2: .11+ → needed

    
  
blocking2.0: betaN+ → .x

    
    

    
  
This works now. Well, the old test case fails but if we write 

window.onload = function(){ ...

instead of 

function onload(){ ...

it works. (Will attach new version).
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: