Closed Bug 199809 Opened 18 years ago Closed 13 years ago

Memory leak in notification handler in nsMsgSendLater

Categories

(MailNews Core :: Composition, defect, P2)

Other
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: ch.ey, Assigned: standard8)

Details

Attachments

(1 file)

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:
.
Assignee: mscott → dmose
Status: UNCONFIRMED → NEW
Ever confirmed: true
Product: MailNews → Core
Component: MailNews: Backend → Address Book
Product: Core → Thunderbird
Target Milestone: --- → Thunderbird1.1
Component: Address Book → MailNews: LDAP Integration
Product: Thunderbird → Core
Target Milestone: Thunderbird1.1 → ---
Assignee: dmose → sspitzer
Component: MailNews: LDAP Integration → MailNews: Composition
QA Contact: esther
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
QA Contact: composition
Product: Core → MailNews Core
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.
Assignee: nobody → bugzilla
Priority: -- → P2
Target Milestone: --- → Thunderbird 3.0b2
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.
Attachment #350758 - Flags: superreview?(bienvenu)
Attachment #350758 - Flags: review?(neil)
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 ;-)
Attachment #350758 - Flags: review?(neil) → review+
Attachment #350758 - Flags: superreview?(bienvenu) → superreview+
Checked in with nits addressed: http://hg.mozilla.org/comm-central/rev/5063d38688e8
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.