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)
Core
Layout: Form Controls
Tracking
()
VERIFIED
FIXED
M17
People
(Reporter: pollmann, Assigned: pollmann)
References
Details
(Keywords: memory-leak, top100)
Attachments
(3 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details |
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•25 years ago
|
Status: NEW → ASSIGNED
Whiteboard: fix in hand
Target Milestone: --- → M16
Assignee | ||
Comment 1•25 years ago
|
||
Assignee | ||
Comment 2•25 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•25 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•25 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•24 years ago
|
||
Comment 6•24 years ago
|
||
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•24 years ago
|
||
Comment 8•24 years ago
|
||
nsbeta2 6/1-. We need to finish up this kind of internal infrastructure work.
Keywords: nsbeta2
Assignee | ||
Comment 9•24 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•24 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•24 years ago
|
||
*** Bug 39856 has been marked as a duplicate of this bug. ***
Comment 12•24 years ago
|
||
*** Bug 39894 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•24 years ago
|
Comment 13•24 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•24 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•24 years ago
|
||
Assignee | ||
Comment 16•24 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•24 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•24 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,...
Comment 19•24 years ago
|
||
See bug 27739 (?). Does undoing that help?
Comment 20•24 years ago
|
||
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•24 years ago
|
||
Yup, that fixes the leak. I take it this is a known problem then?
Assignee | ||
Comment 22•24 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
Closed: 24 years ago
Resolution: --- → FIXED
Comment 23•24 years ago
|
||
(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•24 years ago
|
||
Annotated and CC'd myself on the general bug so I can be annoying if it gets
forgotten. ;)
Comment 26•23 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.
Description
•