Closed
Bug 393357
Opened 17 years ago
Closed 17 years ago
nsDocument::mRadioGroups leaks its members
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9beta1
People
(Reporter: sayrer, Assigned: sharparrow1)
References
()
Details
(Keywords: memory-leak)
Attachments
(2 files, 1 obsolete file)
7.49 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
sicking
:
approval1.9+
|
Details | Diff | Splinter Review |
4.81 KB,
patch
|
Details | Diff | Splinter Review |
It should be an nsObjectHashtable rather than an nsHashtable, and we never call Reset().
Reporter | ||
Updated•17 years ago
|
OS: Linux → All
Hardware: PC → All
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).
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.
Attachment #277937 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 3•17 years ago
|
||
Fixed version.
Attachment #277937 -
Attachment is obsolete: true
Attachment #277958 -
Flags: review?(bzbarsky)
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
Comment 5•17 years ago
|
||
Comment on attachment 277958 [details] [diff] [review] Patch v2 Looks good.
Attachment #277958 -
Flags: superreview+
Attachment #277958 -
Flags: review?(bzbarsky)
Attachment #277958 -
Flags: review+
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.
Attachment #277958 -
Flags: approval1.9?
Attachment #277958 -
Flags: approval1.9? → approval1.9+
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?)
Updated•17 years ago
|
Flags: blocking1.9?
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.
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
Updated•17 years ago
|
Flags: in-testsuite?
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•