613027 - Leak with <fieldset>, <legend>

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  Is this a regression from 3.6?

Comment 2

[Jesse Ruderman]

Yes.  There's no leak on the mozilla-1.9.2 branch.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

j'accuse http://hg.mozilla.org/mozilla-central/rev/8b47f3cabf9f

Comment 4

[Peter Van der Beken [:peterv]]

Mounir: might be unrelated, but why was nsHTMLFieldSetElement changed from NS_DECL_ISUPPORTS_INHERITED to NS_DECL_CYCLE_COLLECTING_ISUPPORTS?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That's not unrelated.

Comment 6

[Peter Van der Beken [:peterv]]

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 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Make nsHTMLFieldSetElement participate in cycle collection properly.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 10

[Mounir Lamouri (:mounir)]

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

Comment 11

[Mounir Lamouri (:mounir)]

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

Comment 12

[Mounir Lamouri (:mounir)]

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

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 14

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/e56aa52a47fe