Leak with <fieldset>, <legend>

RESOLVED FIXED in mozilla2.0b8

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jruderman, Assigned: khuey)

Tracking

(Blocks 1 bug, {memory-leak, regression, testcase})

Trunk
mozilla2.0b8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(2 attachments)

(Reporter)

Description

9 years ago
Posted file testcase
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?
(Reporter)

Comment 2

9 years ago
Yes.  There's no leak on the mozilla-1.9.2 branch.
Keywords: regression
Mounir: might be unrelated, but why was nsHTMLFieldSetElement changed from NS_DECL_ISUPPORTS_INHERITED to NS_DECL_CYCLE_COLLECTING_ISUPPORTS?
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)
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → khuey
Blocks: 613021
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?
Blocks: 595449
No longer blocks: 613021
(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.
Pushed:
http://hg.mozilla.org/mozilla-central/rev/e56aa52a47fe
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.