event handling on inputs leaks containing XUL document

VERIFIED FIXED in M17

Status

()

Core
Editor
P3
major
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: Chris Waterson, Assigned: dbaron)

Tracking

({memory-leak})

Trunk
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta2+])

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
To reproduce, turn on your bloat log,

1. Start mozilla
2. Move the mouse over the URL bar
3. Close the window

You'll see about 700-800Kb of leaks. If you type in the URL bar, this goes up
over 1Mb.

dbaron and I looked at this for a few hours today. It turns out that ender lite
is not cleaning up its script objects. It needs to SetDocument(nsnull) on the
anonymous content, we think.

hyatt suggested that this should just be re-written in XBL, which will
automagically take care of this.
(Reporter)

Comment 1

18 years ago
Also, I tried a stupid hack in nsGfxTextControlFrame2::Destroy() that didn't
work:

cvs server: Diffing .
Index: nsGfxTextControlFrame2.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/forms/src/nsGfxTextControlFrame2.cpp,v
retrieving revision 1.37
diff -u -r1.37 nsGfxTextControlFrame2.cpp
--- nsGfxTextControlFrame2.cpp	2000/06/16 06:09:33	1.37
+++ nsGfxTextControlFrame2.cpp	2000/06/17 00:09:21
@@ -752,6 +752,11 @@
     }
   }
 
+  nsCOMPtr<nsIDOMElement> root;
+  mEditor->GetRootElement(getter_AddRefs(root));
+  nsCOMPtr<nsIContent> rootContent = do_QueryInterface(root);
+  rootContent->SetDocument(nsnull, PR_TRUE, PR_FALSE);
+
   mSelCon = 0;
   mEditor = 0;
   
I stepped through rootcontent->SetDocument() in the debugger, and it turns out
that none of the children had script objects! So maybe the analysis that dbaron
and I came up with is wrong...
Keywords: mlk

Comment 2

18 years ago
Are you sure this isn't bug 42530? 42530 is a nasty thing that could have lots of 
ramifications, it seems. I wouldn't trust any leak data until that one is nailed.
(Reporter)

Comment 3

18 years ago
*** Bug 42934 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 4

18 years ago
Nominating this for nsbeta2.  Here is a table of leaks I think this causes.
(Although not all the leaks in the mail numbers may be caused by this, the
incremental amount per compose window almost definitely is, since the
nsXULDocument leaked went from 3 to 4 when going from 1 to 2 compose windows.)
Any windows opened are immediately closed, in reverse order (except the first
mail compose window was closed and then the second opened).

 657K  Open one browser window, mouse over URLbar
 665K  Ditto, but type in URL bar too.
 960K  Open two browser windows (2d via File | New ...),mouseover both URLbars
1342K  Same for 3 windows

1586K  Open browser window, mail window, and one mail compose window
1874K  Open browser window, mail window, and 2 mail compose windows
Keywords: nsbeta2
Summary: mouseover URL bar leaks 700Kb → event handling on inputs leaks containing XUL document

Comment 5

18 years ago
setting to m17
Target Milestone: --- → M17

Comment 6

18 years ago
Putting on [nsbeta2+] radar for beta2 fix. 
Whiteboard: [nsbeta2+]

Comment 7

18 years ago
so what do I do now then? wait for xbl magic? if so should this be on hyatts 
plate?  I dont mind doing the work, I am just worried that this bug is sitting 
here with no one paying attention to it.
Status: NEW → ASSIGNED
(Assignee)

Comment 8

18 years ago
This leak was not fixed by Brendan's fix for bug 42530.  It still exists today.

Comment 9

18 years ago
Sending over to hyatt per Composer Beta2 bug review today.
Assignee: mjudge → hyatt
Status: ASSIGNED → NEW

Comment 10

18 years ago
Dan has agreed to take this.  Isn't he nice?
Assignee: hyatt → danm
(Assignee)

Comment 11

18 years ago
I think the reason waterson's fix didn't work is that the code he wrote gets 
executed after the rest of the unrooting.  The unrooting is normally triggered 
by nsDocument::SetScriptGlobalObject(nsnull), which calls SetDocument(nsnull, 
PR_TRUE, PR_TRUE) on all the children and then sets the global object to null.  
nsGenericElement::SetDocument won't be able to remove the root if the global 
object is null because it needs the global object to get the script context.  At 
least that's how it gets it right now.
(Assignee)

Comment 12

18 years ago
Created attachment 11389 [details] [diff] [review]
fix for leak
(Assignee)

Comment 13

18 years ago
I'll take this bug since I have a fix for the leak.  If you want to rewrite this 
stuff in XBL for other reasons, that should probably be a separate bug.  I'll 
try to check this in later today.  (I clobbered and am waiting for the build to 
finish in the hopes that my failure to start was a dependencies problem.)
Assignee: danm → dbaron
(Assignee)

Comment 14

18 years ago
Fix checked in, 2000-07-14 16:20-0700.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 15

18 years ago
verified in 7/17 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.