110661 - Leaking imap progress string

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[David :Bienvenu]

[W] MLK: Memory leak of 48 bytes from 1 block allocated in PR_Malloc
                    Distribution of leaked blocks
                    Allocation location
                    malloc         [dbgheap.c:129]
                    PR_Malloc      [prmem.c:54]
                    nsMemoryImpl::Alloc(UINT) [nsMemoryImpl.cpp:320]
                    nsMemory::Alloc(UINT) [nsMemoryImpl.cpp:556]
                    AllocateStringCopy(nsAString const&,WORD *)
[nsReadableUtils.cpp:191]
                    ToNewUnicode(nsAString const&) [nsReadableUtils.cpp:251]
                    nsStringBundle::GetStringFromID(int,WORD * *)
[nsStringBundle.cpp:378]
                    nsImapIncomingServer::GetImapStringByID(int,WORD * *)
[nsImapIncomingServer.cpp:2058]
                            if (m_stringBundle)
                            {
                                PRUnichar *ptrv = nsnull;
                     =>         res = m_stringBundle->GetStringFromID(aMsgId,
&ptrv);
                        
                                if (NS_FAILED(res))
                                {
                    nsImapMailFolder::ProgressStatus(nsIImapProtocol *,UINT,WORD
const*) [nsImapMailFolder.cpp:4516]
                          {
                            nsCOMPtr<nsIImapServerSink> serverSink =
do_QueryInterface(server);
                            if (serverSink)
                     =>       serverSink->GetImapStringByID(aMsgId, &progressMsg);
                          }
                          if (!progressMsg)
                            IMAPGetStringByID(aMsgId, &progressMsg);
                    ProgressStatusProxyEvent::HandleEvent(void)
[nsImapProxyEvent.cpp:1045]

Comment 1

[David :Bienvenu]

fix coming up.

Comment 2

[David :Bienvenu]

this leak happens when we have a formatted string to display status, e.g., when
you delete a message and it displays the "moving to folder %S" message

Comment 3

[David :Bienvenu]

Attachment: proposed fix (and fix for annoying auto compact assertion on biff)

need to free progress string before assigning it.

Comment 4

[Håkan Waara]

Comment on attachment 58280 [details] [diff] [review]
proposed fix (and fix for annoying auto compact assertion on biff)

You could make progressMsg a nsXPIDLString.

Either way, r=hwaara

Comment 5

[Navin Gupta]

nsXPIDLString will be more safe if someone add something here in future. All 
those TextFormatter constructs can be cleaned up to use FormatStringFrom.., 
probably we should have another bug for that. 

Comment 6

[David :Bienvenu]

You'd have to rewrite the code to use an nsXPIDLString, or I would have done it.

Comment 7

[David :Bienvenu]

Attachment: patch using nsXPIDLString

reviews, please. Again, like the other patch, I'm using the Length() == 0 check
instead of a null check because the null check will never be false for a local
variable.

Comment 8

[Håkan Waara]

Two things:

1. Maybe you should use .IsEmpty() instead of checking Length() for a 0 value?
2. Is there a reason for using .Adopt() instead of .Assign() (or just '=' the
value directly)?

Comment 9

[David :Bienvenu]

Attachment: new patch with IsEmpty

using '=' wont' compile, using .Assign doesn't run (probably because there's
already data in the string), and .Adopt both works, and has the advantage of
avoiding an extra copy and free.

Comment 10

[Håkan Waara]

Comment on attachment 58358 [details] [diff] [review]
new patch with IsEmpty

r=hwaara

Comment 11

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

Comment on attachment 58358 [details] [diff] [review]
new patch with IsEmpty

sr=sspitzer

Comment 12

[David :Bienvenu]

fix checked in.

Comment 13

[[e-mail] (gone - use [e-mail] instead)]

Verified FIXED.

m_stringBundle didn't show up in any of my expanded Purify logs on Windows 2000 
with the latest trunk build (IMAP).