nsMsgNewsFolder::UpdateSummaryFromNNTPInfo leaks nsMsgKeySet

VERIFIED FIXED

Status

MailNews Core
Networking: NNTP
VERIFIED FIXED
17 years ago
9 years ago

People

(Reporter: dbaron, Assigned: Håkan Waara)

Tracking

({mlk})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

17 years ago
When I start the main mail window with "./mozilla -mail" and exit,
nsMsgNewsFolder::UpdateSummaryFromNNTPInfo leaks an nsMsgKeySet allocated at:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/news/src/nsNewsFolder.cpp&rev=1.159&mark=792#782

It looks like there just isn't any provision for deleting |set| if it was
created rather than obtained from |mDatabase|
(Reporter)

Updated

17 years ago
Keywords: mlk
(Assignee)

Updated

17 years ago
OS: Linux → All
(Assignee)

Comment 1

17 years ago
I discussed this with David Bienvenu, patch coming up.
Assignee: sspitzer → hwaara
(Assignee)

Comment 2

17 years ago
Created attachment 26807 [details] [diff] [review]
fix v1
(Assignee)

Comment 3

17 years ago
This patch fixes two problems:

* If /setStr/ was null, then we wouldn't go ahead and 
delete /newsrcHasChanged/. I don't know if this was a bug or a feature, but I 
think it's safe to check if /newsrcHasChanged/ exists even though /setStr/ is 
null.

* My patch also fixes so we delete /set/ if mDatabase was null. Because, as 
dbaron pointed out - if mDatabase is null, we execute code which takes /set/ 
and does ::Create on it. If we create a new object, we're responsible for 
deleteing it. So we have to do that at the end of this function if /set/ is non-
null. This was the leak.

I think this patch is ready for r= and sr=, dbaron, bienvenu?
Keywords: patch, review
adding myself back to the bug list.  (feel free to take my bugs, but please keep
me on the cc list.)

Comment 5

17 years ago
you don't need to check if set is null before calling the delete operator -
delete handles the null case. So, the change can just be "delete set;"
(Assignee)

Comment 6

17 years ago
Created attachment 26822 [details] [diff] [review]
patch v2, fixed bienvenu's concern, fixed up tabs (-uw) and added myself to contributors list.

Comment 7

17 years ago
r=bienvenu - I don't think the change with checking newsrchaschanged even if
setstr is null will have any affect but I don't think it causes any harm.
newsrcHasChanged can only be true if setStr is non-null. 

the only place we set it to true is wrapped with "if (setStr &&
nsCRT::strcmp(setStr, oldSet))" 

so your change adds an extra check (in the case that setStr is null) that will
never pass. 

the other part of the fix (adding "delete set;") is correct. thanks for catching
that. 

please supply a new patch.
(Assignee)

Comment 9

17 years ago
I was thinking that sometime someone might use newsrcHasChanged in another place
in that function too, and that day we'll probably also leak newsrcHasChanged if
it depends on setStr.
(Assignee)

Comment 10

17 years ago
Created attachment 26869 [details] [diff] [review]
patch v3, ready for sr=

Comment 11

17 years ago
sr=bienvenu (style point: there's no need for the extra blank lines you've
inserted - they make it harder to read the code because you can see less actual
code at once)
r=sspitzer.

please check in without the newlines, as bienvenu suggests.

Comment 13

17 years ago
fix checked in according to bienvenu and modeline.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

17 years ago
Can someone with a leak detector verify that this bug doesn't occur any longer, 
dbaron?
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.