36639 - Leaks due to circular ownership model of form / form controls

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[Eric Pollmann]

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)

Comment 1

[Eric Pollmann]

Attachment: diffs, for review

Comment 2

[Eric Pollmann]

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.

Comment 3

[Eric Pollmann]

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!  :)

Comment 4

[Eric Pollmann]

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.

Comment 5

[Eric Pollmann]

See also bug 39894 and bug 39856, which are probably dups.

Comment 6

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

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

Comment 7

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

Attachment: revised diffs

Comment 8

[ekrock's old account (dead)]

nsbeta2 6/1-. We need to finish up this kind of internal infrastructure work.

Comment 9

[Eric Pollmann]

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.

Comment 10

[Eric Pollmann]

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.

Comment 11

[Nisheeth Ranjan]

*** Bug 39856 has been marked as a duplicate of this bug. ***

Comment 12

[Kevin McCluskey (gone)]

*** Bug 39894 has been marked as a duplicate of this bug. ***

Comment 13

[leger]

Putting on [nsbeta2+][6/15] radar.  only for crash fix

Comment 14

[Eric Pollmann]

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.

Comment 15

[Eric Pollmann]

Attachment: ref log of leaked form element

Comment 16

[Eric Pollmann]

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.

Comment 17

[Eric Pollmann]

This bug is only a leak now, removing nsbeta2+ as it was contingent on cleaning 
up possible stability problems and that is done.

Comment 18

[Eric Pollmann]

- 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,...

Comment 19

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

See bug 27739 (?).  Does undoing that help?

Comment 20

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

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

Comment 21

[Eric Pollmann]

Yup, that fixes the leak.  I take it this is a known problem then?

Comment 22

[Eric Pollmann]

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!

Comment 23

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

(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...

Comment 24

[Eric Pollmann]

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

Comment 25

[bsharma]

Updating QA contact.

Comment 26

[jerone]

verified build 20010812303 os:win95,mac8.6,winNT