398880 - getElementsByClassName('') leaks an nsVoidArray

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

getElementsByClassName('') leaks an nsVoidArray.  I have a testcase and a patch.

I found this leak with one of my own tools, but it looks like the test suite file
http://lxr.mozilla.org/mozilla/source/content/base/test/test_bug357450.js should have caught this.  Is nobody running mochitests with trace-refcnt?  (See bug 397724 for doing so on a Tinderbox machine.)

Comment 1

[Jesse Ruderman]

Attachment: patch

Comment 2

[Robert Sayre]

(In reply to comment #0)
> Is nobody running mochitests with trace-refcnt?  (See
> bug 397724 for doing so on a Tinderbox machine.)

I've done it, but last time I did so, we were leaking an nsVoidArray on every shutdown so I probably missed this.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 283870 [details] [diff] [review]
patch

+    delete classes;
+    classes = nsnull;

You don't need to null out classes here, but sr=jst either way.

Comment 4

[Jesse Ruderman]

I'm pretty sure I have to null out |classes|, since it can be deleted (again) in the next |if| block.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 283870 [details] [diff] [review]
patch

Oh, duh, you're right of course.

Comment 6

[Reed Loden [:reed]]

Comment on attachment 283870 [details] [diff] [review]
patch

This is a blocker. No need for approval once the tree reopens, unless you want to try to get approval for beta.

Comment 7

[Jesse Ruderman]

Patch landed.

Marking as "in-testsuite+" because this is covered by an existing mochitest, as mentioned in comment 0.  Bug 397724 will get all those tests running with leak-checking enabled.