175540 - [FIX]Mailnews use of arrays cleanup, part 1

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

This patch does a few things:

1)  Replaces most uses of nsISupportsArray::ElementAt with QueryElementAt, since
    the code just QIed the nsISupports to something else immediately
2)  Replaces some uses of nsISupportsArray with nsCOMArray where this made sense
3)  Fixes a few leaks

It doesn't eliminate all calls to ElementAt in mailnews, but it leaves a lot
fewer of them....

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: same patch, diff -w

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and is the "readonly attribute nsISupportsArray msgWindowsArray;" on
nsIMsgMailSession actually useful? It's certainly not used.... and if if did not
exist, mWindows could become an nsCOMArray easily as well (it still could; would
just make the attr getter slower).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: real -w patch

some stuff snuck in the first one that should not have.

Comment 5

[scottputterman]

Dan, when you get a chance, can you verify this bug?

Comment 6

[Boris Zbarsky [:bzbarsky]]

For that matter, Dan, could you review?  ;)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: update to tip

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: and the same, -w (FOR REVIEW ONLY)

Comment 9

[Alec Flett]

Comment on attachment 105700 [details] [diff] [review]
update to tip

as far as the NS_ASSERTION when you're about to leak, lets just add the extra
"break;" to that if() statement - its the right thing to do, even if it doesn't
necessarily leak - might as well exit the loop early, eh?


I'm trying to think of a nicer way to do an array of nsCOMArrays, but one isn't
occuring to me. 

sr=alecf with the break; added. Nice work, there is a lot of code cleanup here.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Adding the break; would change the behavior significantly -- we would be getting
the first match instead of the last match.  I'm not willing to make that change
without a much deeper knowledge of this code...

I can spin off a separate bug on that particular chunk of code if you want.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 105700 [details] [diff] [review]
update to tip

file a bug about the leak, and you've got r=dmose

Comment 12

[Boris Zbarsky [:bzbarsky]]

Bug 181212 filed.

Checked in, except for this change to mailnews/base/src/nsMsgFolderCache.cpp:

-    *result = (nsIMsgFolderCacheElement *) m_cacheElements->Get(&hashKey);
+    *result = (nsIMsgFolderCacheElement *) m_cacheElements->Get(&hashKey).get();

that part was not supposed to sneak in.  It didn't build anyway, in a clean
tree, so.. ;)