109671 - leak pres context of second window (and later windows)

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We leak the pres context of the second navigator window that's opened, and I
think all later windows.  (If one leaks a pres context and then focuses anything
inside that pres context, the document is leaked.  This means we leak the entire
XUL document if the window gets focus.)

Steps to reproduce:
 1) ./mozilla -P Mozilla
 2) Click in blank area of homepage
 3) Ctrl-N
[optional: click in blank area of homepage to see entire XUL document leak]
 4) X in corner of 2d window
 5) X in corner of 1st window

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The nsPresContext is being leaked from an imgRequestProxy.  The leaked reference
is put into the nsCOMPtr here:

nsCOMPtr_base::assign_assuming_AddRef(nsISupports *)+0x00000037
[/builds/cssorg/obj/debug/dist/bin/libxpcom.so +0x0013FD8F]
nsCOMPtr_base::assign_with_AddRef(nsISupports *)+0x0000003C
[/builds/cssorg/obj/debug/dist/bin/libxpcom.so +0x0013132C]
nsCOMPtr<nsISupports>::operator=(nsISupports *)+0x00000021
[/builds/cssorg/obj/debug/dist/bin/libxpcom.so +0x0013FAB5]
imgRequestProxy::Init(imgRequest *, nsILoadGroup *, imgIDecoderObserver *,
nsISupports *)+0x0000012A
[/builds/cssorg/obj/debug/dist/bin/components/libimglib2.so +0x0001D182]
imgLoader::CreateNewProxyForRequest(imgRequest *, nsILoadGroup *,
imgIDecoderObserver *, nsISupports *, unsigned int, imgIRequest *, imgIRequest
**)+0x00000143 [/builds/cssorg/obj/debug/dist/bin/components/libimglib2.so
+0x000194D7]
imgLoader::LoadImage(nsIURI *, nsILoadGroup *, imgIDecoderObserver *,
nsISupports *, unsigned int, nsISupports *, imgIRequest *, imgIRequest
**)+0x0000132C [/builds/cssorg/obj/debug/dist/bin/components/libimglib2.so
+0x00018CBC]
nsImageBoxFrame::UpdateImage(nsIPresContext *, int &)+0x00000564
[/builds/cssorg/obj/debug/dist/bin/components/libgklayout.so +0x0029F904]
nsImageBoxFrame::DidSetStyleContext(nsIPresContext *)+0x00000026
[/builds/cssorg/obj/debug/dist/bin/components/libgklayout.so +0x0029FC92]
nsIFrame::SetStyleContext(nsIPresContext *, nsIStyleContext *)+0x00000078
[/builds/cssorg/obj/debug/dist/bin/components/libgklayout.so +0x0034E1CC]
nsFrame::Init(nsIPresContext *, nsIContent *, nsIFrame *, nsIStyleContext *,
nsIFrame *)+0x000000FB
[/builds/cssorg/obj/debug/dist/bin/components/libgklayout.so +0x0017E803]
nsLeafBoxFrame::Init(nsIPresContext *, nsIContent *, nsIFrame *, nsIStyleContext
*, nsIFrame *)+0x00000031
[/builds/cssorg/obj/debug/dist/bin/components/libgklayout.so +0x002AA425]
nsImageBoxFrame::Init(nsIPresContext *, nsIContent *, nsIFrame *,
nsIStyleContext *, nsIFrame *)+0x00000105
[/builds/cssorg/obj/debug/dist/bin/components/libgklayout.so +0x0029F195]

This reminds me of bug 87040.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hrm.  And we're leaking |nsFrame|s

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The root of the leaked subtree of frames is an nsMenuPopupFrame that is a child
of an nsBoxFrame.  That nsBoxFrame has one other sibling, an nsPlaceHolderFrame.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could it be being leaked because the nsRootBoxFrame has a null mPopupSetFrame? 
It is null -- the question is whether the Destroy through the popup set the only
way Destroy is supposed to be called on popups.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The basic problem here seems to be ordering:

GetPopupSetFrame, this=0x8938a18, popupset=(nil).
###!!! ASSERTION: unexpected null pointer: 'popupSetFrame != 0', file
/builds/trunk/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp, line 5675
###!!! Break: at file
/builds/trunk/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp, line 5675
^GWEBSHELL+ = 5
SetPopupSetFrame, this=0x8938a18, popupset=0x89e1308.
GetPopupSetFrame, this=0x8938a18, popupset=0x89e1308.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: workaround to avoid leak

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for review

Comment 8

[David Hyatt]

sr=hyatt

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 58797 [details] [diff] [review]
patch for review

r=[e-mail]

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch seems to have speed up the window opening test on tinderbox (Txul)
significantly (~1790ms to ~1700ms).  Not sure why, but I'll take it.

Comment 11

[John Morrison]

Someone stop him!! He's outta control!

Comment 12

[John Morrison]

I have a test that opens and times 100 windows, some opened singly,
some in groups of three, and some 10 at a time. After running this 
test with dbaron's leak fix, task manager on win2k reports that I'm 
using 23.8MB (26.6MB peak). By comparison, without the fix, I use
44.3MB (46.0MB peak). Gee. A mere 20MB savings. Is that all :-]

The times also show that this is now, on average, about 9% faster 
through the first 40 windows, and because it no longer hits the same
pathological states near the end of the test, about 17% faster overall
through 100 windows.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

jrgm pointed out that the obvious explanation for the speedup is less time in
js_GC because there's less to mark.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in 2001-11-22 13:44 PDT, in case it wasn't mentioned already.

Comment 15

[Brendan Eich [:brendan]]

dbaron: we love you man!

/be