Robert Sayre Reporter
	Description • 17 years ago

It should be an nsObjectHashtable rather than an nsHashtable, and we never call Reset().

	Eli Friedman Assignee
	Comment 1 • 17 years ago

I haven't actually checked that this fixes the leak, but I'm almost certain it does.  Also fixes up mRadioButtons so that the pointer is an owning pointer (which is a bit fragile, and really bad when combined with traversing the members of the array).

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 17 years ago

Comment on attachment 277937 [details] [diff] [review]
Patch

>Index: nsDocument.h
>+  NS_ENSURE_TRUE(mRadioGroups.Put(tmKey, radioGroup), NS_ERROR_OUT_OF_MEMORY);

That leaks radioGroup on failure.

>+    radioGroup->mRadioButtons.AppendObject(aRadio);
>     NS_IF_ADDREF(aRadio);

That leaks due to the extra addref, no?  Cycle collector will never find the cycle.

>+    if (radioGroup->mRadioButtons.RemoveObject(aRadio)) {
>       NS_IF_RELEASE(aRadio);

Nix the extra release.

	Eli Friedman Assignee
	Comment 3 • 17 years ago

Fixed version.

	Brendan Eich [:brendan]
	Comment 4 • 17 years ago

I wonder if we couldn't have some static leaky-pattern detection, if not static leak detection. Taras, not looking to overload you, just get a few brainwaves.

/be

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 17 years ago

Comment on attachment 277958 [details] [diff] [review]
Patch v2

Looks good.

	Eli Friedman Assignee
	Comment 6 • 17 years ago

Comment on attachment 277958 [details] [diff] [review]
Patch v2

Fixes a minor leak; low-risk because it doesn't significantly change the surrounding code.

	Jesse Ruderman
	Comment 7 • 17 years ago

I get a compilation error when I apply this patch to trunk:

nsDocument.cpp: In member function 'nsresult nsDocument::GetRadioGroup(const nsAString_internal&, nsRadioGroupStruct**)':
nsDocument.cpp:5045: error: conversion from 'nsRadioGroupStruct*' to non-scalar type 'nsAutoPtr<nsRadioGroupStruct>' requested

I'm on Mac and building debug.  (Related to bug 357004?)

	Peter Van der Beken [:peterv]
	Comment 8 • 17 years ago

This one compiles on OS X. All it changes is

+  nsAutoPtr<nsRadioGroupStruct> radioGroup(new nsRadioGroupStruct());

instead of

+  nsAutoPtr<nsRadioGroupStruct> radioGroup = new nsRadioGroupStruct();

I'll check this in later today since it has r/sr/a.