431414 - Review MailNews uses of idl functions that return xpcom allocated arrays [..., array, ...] for memory leaks

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect, P2)

Description

[Mark Banner (:standard8)]

This is a tracking bug that I am raising to track instances where we call functions such as:

nsIMsgDatabase.idl
void getNewList(out unsigned long count, [array, size_is(count)] out nsMsgKey newKeys);

from c++ to ensure that we don't leak the array, or elements of the array. 

Leaking the elements of the array are commonly caused by calling NS_Free (or some "free" form) on the out parameter rather than NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY or NS_FREE_XPCOM_ISUPPORTS_POINTER_ARRAY.

I've already found three instances where we leak memory due to this and some of these appear to be on regularly used functions.

I may turn this bug into a bug that just fixes all the instances where we're leaking - I'll have to see how big the patch gets.

Suggesting this goes on the blocking (or at least wanted) alpha 2 list - it is something that I think should be relatively easy to get some gains with.

Comment 1

[Mark Banner (:standard8)]

Attachment: Fix memory leaks

This fixes the memory leaks I have found by going through:

http://mxr.mozilla.org/seamonkey/search?string=array&find=%5C.idl%24&findi=&filter=&hitlimit=&tree=seamonkey

and checking the c++ callers of those functions. The only ones I didn't check were the callers of the functions in nsICollation because they are intertwined with mork, and DB Views etc, and it would make much better sense to use a tool to check those.

So of the leaks I found, nsDogbertProfileMigrator will obviously leak but typically that'll be a one-off case.

I'm not sure if nsLDAPAutoCompleteSession would really leak its mSearchAttrs or not, but I added the check just in case we start re-using it in strange ways.

The worst ones I found doing this check are probably nsMsgDBFolder::ClearNewMessages and nsMsgDBFolder::CallFilterPlugins. Each of these will leak a PRUint32 (4 bytes typically) per new message received on any protocol. So depending on how many messages a person get, how bad this leak is will vary. If you get a 1000 new posts/messages a day, then you're probably looking at an 8k leak. Not very significant, but its a start to clearing up mailnews.

Comment 2

[[e-mail]]

Comment on attachment 318566 [details] [diff] [review]
Fix memory leaks

>+    // Ensure any old data is freed if necessary.
>+    if (mSearchAttrs) {
>+        NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(mSearchAttrsSize, mSearchAttrs);
>+    }
Nit: ought to set mSearchAttrs to nsnull in case GetAttributes fails.

>+      nsMemory::Free(newMessageKeys);
Nit: NS_Free (and below)

>+  nsMemory::Free(newKeys);
>+  newKeys = 0;
Nit: newKeys doesn't get used again, so no need to clear it this time!

Comment 3

[Mark Banner (:standard8)]

Attachment: Fix memory leaks v2

Fixes Neil's comments, carrying forward his sr.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 319371 [details] [diff] [review]
Fix memory leaks v2

Dan seems busy at the moment, and there's more important patches than this one that I want him to look at...

Comment 5

[Mark Banner (:standard8)]

Patch checked in, I don't believe there are any more leaks of this type, if there are, we'll cover them in separate bugs.