nsMsgNewsFolder::UpdateSummaryFromNNTPInfo leaks nsMsgKeySet



18 years ago
10 years ago


(Reporter: dbaron, Assigned: hwaara)




Firefox Tracking Flags

(Not tracked)



(3 attachments)

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


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


18 years ago
OS: Linux → All

Comment 1

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

Comment 2

18 years ago
Created attachment 26807 [details] [diff] [review]
fix v1

Comment 3

18 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 

* 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

18 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;"

Comment 6

18 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

18 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

please supply a new patch.

Comment 9

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

Comment 10

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

Comment 11

18 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)

please check in without the newlines, as bienvenu suggests.

Comment 13

18 years ago
fix checked in according to bienvenu and modeline.
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 14

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