Closed Bug 36639 Opened 25 years ago Closed 24 years ago

Leaks due to circular ownership model of form / form controls

Categories

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

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: pollmann, Assigned: pollmann)

References

Details

(Keywords: memory-leak, top100)

Attachments

(3 files)

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)
Status: NEW → ASSIGNED
Whiteboard: fix in hand
Target Milestone: --- → M16
Attached patch diffs, for review (deleted) — Splinter Review
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.
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! :)
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
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
Attached patch revised diffs (deleted) — Splinter Review
nsbeta2 6/1-. We need to finish up this kind of internal infrastructure work.
Keywords: nsbeta2
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.
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
*** Bug 39856 has been marked as a duplicate of this bug. ***
*** Bug 39894 has been marked as a duplicate of this bug. ***
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)
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)
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.
Attached file ref log of leaked form element (deleted) —
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.
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)
- 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
Yup, that fixes the leak. I take it this is a known problem then?
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
Closed: 24 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...
Annotated and CC'd myself on the general bug so I can be annoying if it gets forgotten. ;)
Updating QA contact.
QA Contact: ckritzer → bsharma
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.

Attachment

General

Creator:
Created:
Updated:
Size: