1. Run Firefox (debug build) with XPCOM_MEM_LEAK_LOG=2 2. Load the testcase 3. Quit Firefox Result: trace-refcnt reports leaks including 1 nsDocument, 2 nsGlobalWindow
Hmm. Is this a regression from 3.6?
Yes. There's no leak on the mozilla-1.9.2 branch.
Mounir: might be unrelated, but why was nsHTMLFieldSetElement changed from NS_DECL_ISUPPORTS_INHERITED to NS_DECL_CYCLE_COLLECTING_ISUPPORTS?
That's not unrelated.
Really? AFAIK all it does is add an unused mRefCnt member to nsHTMLFieldSetElement, we're still calling the base class' AddRef/Release. I don't understand how that would cause a leak.
Comment 5 is bogus; not sure how I got a run without it leaking, but it is still leaking now even with that fixed.
Comment on attachment 492621 [details] [diff] [review] Make nsHTMLFieldSetElement participate in cycle collection properly. The real problem here is that nsHTMLFieldSetElement doesn't do cycle collection properly. The cycle collector logic in unlink and traverse is correct, but it never gets called because the QI implementation is wrong. QIing an nsHTMLFieldSetElement gets you the cycle collection helper for a superclass (nsGenericElement?), so once you've constructed a cycle that goes through nsHTMLFieldSetElement::mElements we leak. It'd be nice to have an analysis for classes that implement cycle collection logic but don't directly QI to a cycle collection helper.
Attachment #492621 - Flags: review?(peterv)
Assignee: nobody → khuey
Status: NEW → ASSIGNED
(In reply to comment #4) > Mounir: might be unrelated, but why was nsHTMLFieldSetElement changed from > NS_DECL_ISUPPORTS_INHERITED to NS_DECL_CYCLE_COLLECTING_ISUPPORTS?
(In reply to comment #4) > Mounir: might be unrelated, but why was nsHTMLFieldSetElement changed from > NS_DECL_ISUPPORTS_INHERITED to NS_DECL_CYCLE_COLLECTING_ISUPPORTS? It has been done with this changeset: http://hg.mozilla.org/mozilla-central/rev/8b47f3cabf9f Given that the changeset moved the declaration part of nsHTMLFieldSetElement to a header, it's hard to understand why exactly I did this change. I guess/hope I didn't meant to do that given that I was told in bug 595449 that it was wrong.
FWIW, I would bet it happened because patches for bug 595449 were before patches for bug 557087 in my queue and when I applied patches for bug 557087, the comment fix (for NS_DECL_CYCLE_COLLECTING_ISUPPORTS) was probably overwritten and I probably didn't notice given that it was moving the code to another file. So, stupid mistake, sorry :(
Attachment #492621 - Flags: review?(peterv) → review+
(In reply to comment #9) > It'd be nice to have an analysis for classes that implement cycle collection > logic but don't directly QI to a cycle collection helper. Filed Bug 614238 with a runtime implementation.
blocking2.0: ? → final+
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.