199809 - Memory leak in notification handler in nsMsgSendLater

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect, P2)

Description

[Christian Eyrich]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:1.4a) Gecko/20030329
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:1.4a) Gecko/20030329

nsMsgSendLater::AddListener adds Listeners to the list. There's also a function
to remove them -> nsMsgSendLater::RemoveListener, but isn't used.

The destructor of nsMsgSendLater doesn't free the array. But there are values
assigned to it when entering de destructor. So it seems that the memory chunks
get lost.

Maybe it's enough to add a call RemoveListener(nsnull) to a slightly changed
RemoveListener.

NS_IMETHODIMP
nsMsgSendLater::RemoveListener(nsIMsgSendLaterListener *aListener)
{
  PRInt32 i;
  for (i=0; i<mListenerArrayCount; i++)
    if (mListenerArray[i] == aListener || !aListener)
    {
      NS_RELEASE(mListenerArray[i]);
      mListenerArray[i] = nsnull;
      if(aListener)
        return NS_OK;
    }

  return NS_ERROR_INVALID_ARG;
}

Or something like this - just a thought.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

.

Comment 2

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

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 3

[Mark Banner (:standard8)]

The nsMsgSendLater code is wrong on multiple counts (should use an array helper class, should free memory etc). I'm fairly sure this is just going to leak when we send unsent messages, but in any case, its not a good idea. I'll also need to look to see if we're getting refcount cycles or not.

Comment 4

[Mark Banner (:standard8)]

Attachment: Fix the leak and provide unit test

This fixes the leak by moving us to an nsTObserverArray (which is the correct thing to use in this instance, rather than a handle-ourselves "dynamic" array) and provides a unit test for the send later function.

Comment 5

[[e-mail]]

Comment on attachment 350758 [details] [diff] [review]
Fix the leak and provide unit test

>+void
>+nsMsgSendLater::NotifyListenersOnStopSending(nsresult aStatus,
>+                                             const PRUnichar *aMsg, 

>+  void NotifyListenersOnStopSending(nsresult aStatus, const PRUnichar *aMsg, 
Nit: fix the whitespace while you're here :-)

>+    server.stop();
>+  
And don't make matters worse ;-)

Comment 6

[Mark Banner (:standard8)]

Checked in with nits addressed: http://hg.mozilla.org/comm-central/rev/5063d38688e8