Leaks due to circular ownership model of form / form controls

VERIFIED FIXED in M17

Status

()

P3
normal
VERIFIED FIXED
19 years ago
17 years ago

People

(Reporter: pollmann, Assigned: pollmann)

Tracking

({memory-leak, top100})

Trunk
memory-leak, top100
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

19 years ago
This is a bug to track the cleanup of the circular ownership model that the form 
content element and the form control content elements currently have.  Nisheeth 
was recently bitten by this (bug 36362), and Chris, he and I came up with a 
reasonable solution - use weak references!

I have this completely implemented and working in my tree.  I'd like to run some 
memory leak tests before checking it into the tree, but I think this will make 
things a lot easier to grok.

(CC'ing danm because he also worked on this recently)
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
Whiteboard: fix in hand
Target Milestone: --- → M16
(Assignee)

Comment 1

19 years ago
Created attachment 7806 [details] [diff] [review]
diffs, for review
(Assignee)

Comment 2

19 years ago
According to XPCOM_MEM_LOG_LEAKS, this causes a few new leaks when visiting 
Viewer demo 8.  My guess is these were here all along, but the strange way we 
release things was causing them to be deleted anyway.  I'm looking in to this.

See bug 37076 for a test case.
(Assignee)

Comment 3

19 years ago
Running XPCOM_MEM_TRACE_REFCNT, even logging just the single class 
nsHTMLFormElement, took hours to load test 8 on my PII 450 256Mb.  But I just 
got the results back.  Time to go through the logs!  :)
(Assignee)

Comment 4

19 years ago
Since this is not causing any crashes or leaks as is, and i have not yet been 
able to trace down the leaks caused by the partial fix, moving off to M17.
Whiteboard: fix in hand → partial fix attached
Target Milestone: M16 → M17
(Assignee)

Comment 5

19 years ago
See also bug 39894 and bug 39856, which are probably dups.
The leaks were caused by not making the changes to nsHTMLLegendElement.cpp . 
Making similar changes there (hopefully I did it correctly...) fixes the form
content leaks on test8.  I also checked that the bugzilla query page does not
leak, and verified that this fix fixes the leaks on http://www.city.net/ and
http://www.cnn.com/ (the two bugs mentioned above).

I'll attach a revised patch.  The only differences between it and Eric
Pollmann's original patch are:
 * against a more current tree (from yesterday)
 * added changes to nsHTMLLegendElement.cpp
nsbeta2 6/1-. We need to finish up this kind of internal infrastructure work.
Keywords: nsbeta2
(Assignee)

Comment 9

19 years ago
David, great catch, thanks!  I made one additional change to 
nsHTMLLegendElement's destructor - to not NS_RELEASE mForm.  I'll run ref 
logging again Monday and see if it cleared up the leaks.
(Assignee)

Comment 10

19 years ago
I just tested with David's new patch, plus the change to nsHTMLLegendElement's
destructor.  The leaks are all fixed!  I'll get a review and approval for
checking this in.
Whiteboard: partial fix attached → fix attached

Comment 11

19 years ago
*** Bug 39856 has been marked as a duplicate of this bug. ***
*** Bug 39894 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

19 years ago
Keywords: mlk, top100
Summary: Ownership model of form / form control content is circular → Leaks due to circular ownership model of form / form controls
Whiteboard: fix attached → fix attached (bug caused crashes on cnn.com in past, dups)

Comment 13

19 years ago
Putting on [nsbeta2+][6/15] radar.  only for crash fix
Whiteboard: fix attached (bug caused crashes on cnn.com in past, dups) → [nsbeta2+][6/15] radar.fix attached (bug caused crashes on cnn.com in past, dups)
(Assignee)

Comment 14

19 years ago
Memory model for the form / form elements has been put back to a normal state.  
That fixes the potential stability problems associated with this bug.

However, this does not fix either the leak of the form elements at city.net or 
the leak of the form elements at cnn.com.

Bug 39894 speculates that the form elements may be leaked in demoteContainer.  
Interestingly though, this code path is only encountered on city.net, and not 
cnn.com, but both exhibit a leak of two form elements (and the associated 
nsFormControlList).  I'm attaching a ref count log, this indicates the problem 
may be in the mHiddenForm of a XUL document being ref counted incorrectly.
(Assignee)

Comment 15

19 years ago
Created attachment 9316 [details]
ref log of leaked form element
(Assignee)

Comment 16

19 years ago
Methinks the problem is that two nsXULDocuments are leaked - and they leak their 
respective mHiddenForm nsHTMLFormElements.

This leak is also reproducible by double clicking on the URL bar, typing any 
random string, then exiting the app - you don't need to visit cnn.com, city.net, 
or any other site for that matter.
(Assignee)

Comment 17

19 years ago
This bug is only a leak now, removing nsbeta2+ as it was contingent on cleaning 
up possible stability problems and that is done.
Whiteboard: [nsbeta2+][6/15] radar.fix attached (bug caused crashes on cnn.com in past, dups)
(Assignee)

Comment 18

19 years ago
- The leaked nsFormElements are members (mHiddenForm) of nsXULDocuments
- The leaked nsXULDocuments are held by nsXULEventListeners (DoKey, which I 
  imagine is  called when typing in the URL bar, Creates a "KeyBindingDocument"
  which is stored away in a hash table, mKeyBindingTable).
- I'm guessing the leaked nsXULEventListeners are caused by the leaked
  nsEventListenerManagers.
- The leaked nsEventListenerManagers are members of nsXULDocuments

All this is to say there appears to be a circularity in 
nsXULDocuments/nsXULKeyListerers/nsEventListenerManagers,...
See bug 27739 (?).  Does undoing that help?
By undoing it I mean:

Index: mozilla/rdf/content/src/nsXULKeyListener.cpp
===================================================================
RCS file: /cvsroot/mozilla/rdf/content/src/nsXULKeyListener.cpp,v
retrieving revision 1.71
diff -u -d -r1.71 nsXULKeyListener.cpp
--- mozilla/rdf/content/src/nsXULKeyListener.cpp        2000/05/16 11:34:59    
1.71
+++ mozilla/rdf/content/src/nsXULKeyListener.cpp        2000/05/25 01:36:29
@@ -363,7 +363,8 @@
   if (gRefCnt == 1) {
     mKeyBindingTable = new nsSupportsHashtable();
 
-#define LEAK_KEY_BINDINGS_TABLE_BUG_27739
+// comment this out, it fixes leak:
+//#define LEAK_KEY_BINDINGS_TABLE_BUG_27739
 #ifdef LEAK_KEY_BINDINGS_TABLE_BUG_27739
     // XXX This is a total hack to deal with bug 27739. At some point,
     // the keybindings table will go away, and we won't need to worry
(Assignee)

Comment 21

19 years ago
Yup, that fixes the leak.  I take it this is a known problem then?
(Assignee)

Comment 22

19 years ago
So the remaining FormElement leak is really a dup of bug 27739 / bug 24645.  I 
guess we should mark this one fixed and move on.  Please reopen if you disagree, 
thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
(responding to comment before the most recent)
It's known, but it might also be forgotten, since bug 27739 was marked a dup of
a general bug...
(Assignee)

Comment 24

19 years ago
Annotated and CC'd myself on the general bug so I can be annoying if it gets 
forgotten. ;)

Comment 25

18 years ago
Updating QA contact.
QA Contact: ckritzer → bsharma

Comment 26

17 years ago
verified build 20010812303 os:win95,mac8.6,winNT
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.