Closed Bug 581240 Opened 12 years ago Closed 8 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)

1.9.2 Branch
x86
All
defect

Tracking

()

RESOLVED WORKSFORME
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)

<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+
Blocks: 581336
Nickolay, was the wiki app in question "Confluence"?
Yeah.
Perfect, thanks.
No longer blocks: 581336
Duplicate of this bug: 581336
Duplicate of this bug: 582829
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+
Duplicate of this bug: 595952
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: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.