404470 - [FIX]Leak with XUL in iframe

Status: VERIFIED FIXED

(Core :: XUL, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

On about 80% of loads, this testcase triggers a bunch of assertions and makes DOMWindows leak.

###!!! ASSERTION: initial containing block already created: 'nsnull == mInitialContainingBlock', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9000

###!!! ASSERTION: Popup set is already defined! Only 1 allowed.: 'Not Reached', file /Users/jruderman/trunk/mozilla/layout/xul/base/src/nsRootBoxFrame.cpp, line 298

###!!! ASSERTION: Unexpected PopupSetFrame: 'rootBox->GetPopupSetFrame() == newFrame', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 6204

###!!! ASSERTION: Unexpected child of document element containing block: 'mDocElementContainingBlock->GetFirstChild(nsnull) == nsnull', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9028

###!!! ASSERTION: already have a child frame: 'mFrames.IsEmpty()', file /Users/jruderman/trunk/mozilla/layout/xul/base/src/nsRootBoxFrame.cpp, line 165
JavaScript error: , line 0: uncaught exception: Permission denied to get property XPCComponents.classes

###!!! ASSERTION: Some objects allocated with AllocateFrame were not freed: 'mFrameCount == 0', file /Users/jruderman/trunk/mozilla/layout/base/nsPresShell.cpp, line 673

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

we'd better get this fixed

Comment 2

[Boris Zbarsky [:bzbarsky]]

The issue is most likely that nsXULDocument::StartLayout calls InitialReflow on the presshell unconditionally.  If it's already been called (e.g. by nsSubDocumentFrame::Init calling nsSubDocumentFrame::ShowDocShell calling nsDocShell::SetVisibility calling DocumentViewerImpl::Show which does InitPresentationStuff(PR_TRUE)), then we'll get these asserts.

So it's a timing issue: if the style change that causes a frame reconstruct (the body display change) comes through after the XUL has started loading but before it's finished loading (e.g. while it's waiting for the stylesheet), we'll get this problem.

I suppose we could skip the InitialReflow call if it's already happened (as the HTML sink does).  But we should double-check the BeginObservingDocument() mess.  I _think_ it would always have been called in that case and the presshell would be observing the document, but worth checking.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I have reproduced this with the testcase but *very* infrequently. I let it run for hundreds of reloads and I've only seen the assertions fire twice.

I also got one of these once:
###!!! ASSERTION: Shallow unbind won't clear document and binding parent on kids!: 'aDeep || (!GetCurrentDoc() && !GetBindingParent())', file /Users/roc/mozilla-trunk/content/base/src/nsGenericElement.cpp, line 2086

Boris, do you want to take this bug?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that should reliably reproduce this

I can try to look at this, but I'm pretty crunched for time right now...  I'm also not sure there's a sane way to fix this while giving correct behavior in all cases without digging deep into the tree-building code that the XUL prototype walk does.  :(

I'm pretty sure I can get to this for beta3.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it's pretty clear you're the right person to fix this if you possibly can. Send some bugs my way in return :-)

Comment 6

[Jesse Ruderman]

Sweet, this is the last known bug that triggers the assertion added in bug 334514.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

The major change here is that when doing Show() on a viewer for an already-existing document we'll only do the initial reflow if the document is ready for it.  Documents start our ready by default, and become unready when StartDocumentLoad is called.  After that, either the sink or the caller of StartDocumentLoad() sets the document as ready again.

The existing BeginObservingDocument() calls are all not needed, since document viewer calls it.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed 2008-01-20 10:02.  Checked in my testcase as a crashtest.

Comment 9

[Carsten Book [:Tomcat]]

verified fixed using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050621 Firefox/3.0pre - no leak/assertion on testcase

--> Verified fixed