69256 - nsMsgNewsFolder::UpdateSummaryFromNNTPInfo leaks nsMsgKeySet

Status: VERIFIED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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|

Comment 1

[Håkan Waara]

I discussed this with David Bienvenu, patch coming up.

Comment 2

[Håkan Waara]

Attachment: fix v1

Comment 3

[Håkan Waara]

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?

Comment 4

[(not reading, please use [e-mail] instead)]

adding myself back to the bug list.  (feel free to take my bugs, but please keep
me on the cc list.)

Comment 5

[David :Bienvenu]

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

[Håkan Waara]

Attachment: patch v2, fixed bienvenu's concern, fixed up tabs (-uw) and added myself to contributors list.

Comment 7

[David :Bienvenu]

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.

Comment 8

[(not reading, please use [e-mail] instead)]

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.

Comment 9

[Håkan Waara]

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

[Håkan Waara]

Attachment: patch v3, ready for sr=

Comment 11

[David :Bienvenu]

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)

Comment 12

[(not reading, please use [e-mail] instead)]

r=sspitzer.

please check in without the newlines, as bienvenu suggests.

Comment 13

[timeless]

fix checked in according to bienvenu and modeline.

Comment 14

[Håkan Waara]

Can someone with a leak detector verify that this bug doesn't occur any longer, 
dbaron?