MLK: leaks in addrbook upon autocompletion and sending mail

VERIFIED FIXED

Status

SeaMonkey
MailNews: Address Book & Contacts
VERIFIED FIXED
16 years ago
13 years ago

People

(Reporter: Navin Gupta, Assigned: Navin Gupta)

Tracking

({mlk})

Trunk
x86
Windows NT

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

1.87 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

16 years ago
stack 1
        [W] MLK: Memory leak of 24 bytes from 2 blocks allocated in PR_Malloc
            Distribution of leaked blocks
            Allocation location
            malloc         [dbgheap.c:129]
            PR_Malloc      [prmem.c:406]
            nsMemoryImpl::Alloc(UINT) [nsMemoryImpl.cpp:320]
            NS_IMETHODIMP_(void *)
            nsMemoryImpl::Alloc(PRSize size)
            {
         =>     void* result = MALLOC1(size);
                if (! result) {
                    // Request an asynchronous flush
                    FlushMemory(NS_LITERAL_STRING("alloc-failure").get(), 
PR_FALSE);
            nsMemory::Alloc(UINT) [nsMemory.cpp:81]
                if (!ENSURE_ALLOCATOR)
                    return nsnull;
            
         =>    return gMemory->Alloc(size);
            }
            
            NS_EXPORT void*
            HashtableToPropertyPtrArrays::Convert(nsHashtable&,UINT *,char * * 
*,WORD * * *) [nsAbUtils.cpp:236]
                if (!(*propertyNameArray))
                    return NS_ERROR_OUT_OF_MEMORY;
                *propertyValueArray =
         =>         NS_STATIC_CAST(PRUnichar**, nsMemory::Alloc (sizeof 
(PRUnichar* ) * (*propertiesSize)));
                if (!(*propertyValueArray))
                {
                    nsMemory::Free (*propertyNameArray);
            nsAbBSDirectory::GetChildNodes(nsIEnumerator * *) 
[nsAbBSDirectory.cpp:240]
                        HashtableToPropertyPtrArrays::Convert (propertySet,
                            factoryPropertyNames.GetSizeAddr (),
                            factoryPropertyNames.GetArrayAddr (),
         =>                 factoryPropertyValues.GetArrayAddr ());
                        *(factoryPropertyNames.GetSizeAddr ()) = 
factoryPropertyNames.GetSize ();
            
                        // Create the directories
            nsAbAutoCompleteSession::SearchDirectory(char 
const*,nsAbAutoCompleteSearchString *,nsIAutoCompleteResults *,int) 
[nsAbAutoCompleteSession.cpp:495]
                    return rv;
            
                nsCOMPtr<nsIEnumerator> subDirectories;
         =>     if 
(NS_SUCCEEDED(directory->GetChildNodes(getter_AddRefs(subDirectories))) && 
subDirectories)
                {
                    nsCOMPtr<nsISupports> item;
                    if (NS_SUCCEEDED(subDirectories->First()))
            nsAbAutoCompleteSession::OnStartLookup(WORD 
const*,nsIAutoCompleteResults *,nsIAutoCompleteListener *) 
[nsAbAutoCompleteSession.cpp:628]
            XPTC_InvokeByIndex [xptcinvoke.cpp:105]
            
XPCWrappedNative::CallMethod(XPCCallContext&,CallMode::XPCWrappedNative) 
[xpcwrappednative.cpp:2009]

stack 2

[W] MLK: Memory leak of 24 bytes from 2 blocks allocated in PR_Malloc
        Distribution of leaked blocks
        Allocation location
            malloc         [dbgheap.c:129]
            PR_Malloc      [prmem.c:406]
            nsMemoryImpl::Alloc(UINT) [nsMemoryImpl.cpp:320]
            nsMemory::Alloc(UINT) [nsMemory.cpp:81]
            HashtableToPropertyPtrArrays::Convert(nsHashtable&,UINT *,char * * 
*,WORD * * *) [nsAbUtils.cpp:232]
            nsAbBSDirectory::GetChildNodes(nsIEnumerator * *) 
[nsAbBSDirectory.cpp:240]
                            HashtableToPropertyPtrArrays::Convert (propertySet,
                                factoryPropertyNames.GetSizeAddr (),
                                factoryPropertyNames.GetArrayAddr (),
             =>                 factoryPropertyValues.GetArrayAddr ());
                            *(factoryPropertyNames.GetSizeAddr ()) = 
factoryPropertyNames.GetSize ();
                
                            // Create the directories
            nsAbAutoCompleteSession::SearchDirectory(char 
const*,nsAbAutoCompleteSearchString *,nsIAutoCompleteResults *,int) 
[nsAbAutoCompleteSession.cpp:495]
                        return rv;
                
                    nsCOMPtr<nsIEnumerator> subDirectories;
             =>     if 
(NS_SUCCEEDED(directory->GetChildNodes(getter_AddRefs(subDirectories))) && 
subDirectories)
                    {
                        nsCOMPtr<nsISupports> item;
                        if (NS_SUCCEEDED(subDirectories->First()))
            nsAbAutoCompleteSession::OnStartLookup(WORD 
const*,nsIAutoCompleteResults *,nsIAutoCompleteListener *) 
[nsAbAutoCompleteSession.cpp:628]
                    if (NS_SUCCEEDED(rv))
                          if (NS_FAILED(SearchPreviousResults(&searchStrings, 
previousSearchResult, results)))
                          {
             =>               rv = SearchDirectory(kAllDirectoryRoot, 
&searchStrings, results, PR_TRUE);
                          }
                
                    AutoCompleteStatus status = nsIAutoCompleteStatus::failed;
            XPTC_InvokeByIndex [xptcinvoke.cpp:105]
            
XPCWrappedNative::CallMethod(XPCCallContext&,CallMode::XPCWrappedNative) 
[xpcwrappednative.cpp:2009]

similar stack trace on sending message
(Assignee)

Updated

16 years ago
Keywords: mlk
QA Contact: nbaca → stephend
Summary: MLK leaks in addrbook upon autocompletion and sending mail → MLK: leaks in addrbook upon autocompletion and sending mail
(Assignee)

Comment 1

16 years ago
same leaks on sending message
   [W] MLK: Memory leak of 24 bytes from 2 blocks allocated in PR_Malloc
                Distribution of leaked blocks
                Allocation location
                malloc         [dbgheap.c:129]
                PR_Malloc      [prmem.c:406]
                nsMemoryImpl::Alloc(UINT) [nsMemoryImpl.cpp:320]
                nsMemory::Alloc(UINT) [nsMemory.cpp:81]
                HashtableToPropertyPtrArrays::Convert(nsHashtable&,UINT *,char * 
* *,WORD * * *) [nsAbUtils.cpp:236]
                    if (!(*propertyNameArray))
                        return NS_ERROR_OUT_OF_MEMORY;
                    *propertyValueArray =
             =>         NS_STATIC_CAST(PRUnichar**, nsMemory::Alloc (sizeof 
(PRUnichar* ) * (*propertiesSize)));
                    if (!(*propertyValueArray))
                    {
                        nsMemory::Free (*propertyNameArray);
                nsAbBSDirectory::GetChildNodes(nsIEnumerator * *) 
[nsAbBSDirectory.cpp:240]
                            HashtableToPropertyPtrArrays::Convert (propertySet,
                                factoryPropertyNames.GetSizeAddr (),
                                factoryPropertyNames.GetArrayAddr (),
             =>                 factoryPropertyValues.GetArrayAddr ());
                            *(factoryPropertyNames.GetSizeAddr ()) = 
factoryPropertyNames.GetSize ();
                
                            // Create the directories
                nsMsgCompose::GetABDirectories(char const*,nsISupportsArray 
*,int) [nsMsgCompose.cpp:3214]
                      return rv;
                
                  nsCOMPtr<nsIEnumerator> subDirectories;
             =>   if 
(NS_SUCCEEDED(directory->GetChildNodes(getter_AddRefs(subDirectories))) && 
subDirectories)
                  {
                    nsCOMPtr<nsISupports> item;
                    if (NS_SUCCEEDED(subDirectories->First()))
                nsMsgCompose::CheckAndPopulateRecipients(int,int,WORD * *,UINT 
*) [nsMsgCompose.cpp:3436]
                  if (NS_SUCCEEDED(rv) && addrbookDirArray)
                  {
                    nsString dirPath;
             =>     GetABDirectories(kAllDirectoryRoot, addrbookDirArray, 
PR_TRUE);
                    PRUint32 nbrRecipients;
                
                    PRUint32 nbrAddressbook;
                XPTC_InvokeByIndex [xptcinvoke.cpp:105]
                
XPCWrappedNative::CallMethod(XPCCallContext&,CallMode::XPCWrappedNative) 
[xpcwrappednative.cpp:2009]
            [W] MLK: Memory leak of 24 bytes from 2 blocks allocated in 
PR_Malloc
                Distribution of leaked blocks
                Allocation location
                malloc         [dbgheap.c:129]
                PR_Malloc      [prmem.c:406]
                nsMemoryImpl::Alloc(UINT) [nsMemoryImpl.cpp:320]
                nsMemory::Alloc(UINT) [nsMemory.cpp:81]
                HashtableToPropertyPtrArrays::Convert(nsHashtable&,UINT *,char * 
* *,WORD * * *) [nsAbUtils.cpp:232]
                    
                    
                        *propertyNameArray =
                 =>         NS_STATIC_CAST(char**, nsMemory::Alloc (sizeof 
(char* ) * (*propertiesSize)));
                        if (!(*propertyNameArray))
                            return NS_ERROR_OUT_OF_MEMORY;
                        *propertyValueArray =
                nsAbBSDirectory::GetChildNodes(nsIEnumerator * *) 
[nsAbBSDirectory.cpp:240]
                                HashtableToPropertyPtrArrays::Convert 
(propertySet,
                                    factoryPropertyNames.GetSizeAddr (),
                                    factoryPropertyNames.GetArrayAddr (),
                 =>                 factoryPropertyValues.GetArrayAddr ());
                                *(factoryPropertyNames.GetSizeAddr ()) = 
factoryPropertyNames.GetSize ();
                    
                                // Create the directories
                nsMsgCompose::GetABDirectories(char const*,nsISupportsArray 
*,int) [nsMsgCompose.cpp:3214]
                          return rv;
                    
                      nsCOMPtr<nsIEnumerator> subDirectories;
                 =>   if 
(NS_SUCCEEDED(directory->GetChildNodes(getter_AddRefs(subDirectories))) && 
subDirectories)
                      {
                        nsCOMPtr<nsISupports> item;
                        if (NS_SUCCEEDED(subDirectories->First()))
                nsMsgCompose::CheckAndPopulateRecipients(int,int,WORD * *,UINT 
*) [nsMsgCompose.cpp:3436]
                      if (NS_SUCCEEDED(rv) && addrbookDirArray)
                      {
                        nsString dirPath;
                 =>     GetABDirectories(kAllDirectoryRoot, addrbookDirArray, 
PR_TRUE);
                        PRUint32 nbrRecipients;
                    
                        PRUint32 nbrAddressbook;
                XPTC_InvokeByIndex [xptcinvoke.cpp:105]
                
XPCWrappedNative::CallMethod(XPCCallContext&,CallMode::XPCWrappedNative) 
[xpcwrappednative.cpp:2009]
(Assignee)

Comment 2

16 years ago
Created attachment 62782 [details] [diff] [review]
proposed fix

free mArray when mFreeElements is false. I have tested it and do some more
tests
I'm not sure this is correct.

who is creating the those objects with false, for the freeElements() arg?  

I'm not sure that in all cases, we want to free the memory, we might own it.

can you investigate what the callers are expecting?  perhaps some caller is
doing the wrong thing.
see related bug #116983.  most of those helper classes are duplicates of code
already in the tree, and should be removed.
(Assignee)

Updated

16 years ago
Depends on: 116983
I think naving's patch is correct, I misunderstood.

in one scenario, we want to delete the strings in the array, and the array.
in another scenario, we want to just delete the array.

but it looks like even when the caller sets it it so we don't delete the 
strings (and just the array), we still copy the strings, so we're going to leak 
them.

I'm reviewing how these helper classes work, and our usage of them now.

sorry for the delay, navin.

can you do the investigation?

Look at the callers carefully, to see if we can avoid the copy.
If the copy is always necessary, we can simplify the code to always free.

If not, your fix should be all we need.

also check if anyone is really trying to those helper classes in a non-copy 
way, they might still be copying.  (perhaps someone else has changed the string 
foo code and force a copy, which would explain the leak.)

But if you determine that we have to copy, we can simplify those helper 
classes, so that the callers always copy, and the classes always free.
(Assignee)

Comment 7

16 years ago
Created attachment 63879 [details] [diff] [review]
proposed fix

The fix is to free the array when we are not doing a copy of elements. I have 
investigated and we can use PR_FALSE because copy in these cases is wasteful
because we are just going between native and non-native type data structures.

seth, please review. 

I have also tested the actions affected by this change and that are 
addressbook quick search and composing and sending mail and they work fine.
Attachment #62782 - Attachment is obsolete: true

Comment 8

16 years ago
Comment on attachment 63879 [details] [diff] [review]
proposed fix

r=dmose@netscape.com if you use NS_STATIC_CAST instead of c-style casts in
nsAbUtils.cpp.
Attachment #63879 - Flags: review+
Comment on attachment 63879 [details] [diff] [review]
proposed fix

sr=sspitzer, once you address the static cast issue.
Attachment #63879 - Flags: superreview+
(Assignee)

Comment 10

16 years ago
here is what I did to use static_cast 
NS_STATIC_CAST(PRUnichar*, (*array[i]).get());

and it doesn't compile

F:\trunk\mozilla\mailnews\addrbook\src\nsAbUtils.cpp(71) : error C2440: 'static_
cast' : cannot convert from 'const char *' to 'char *'
        Conversion loses qualifiers
F:\trunk\mozilla\mailnews\addrbook\src\nsAbUtils.cpp(124) : error C2440: 'static
_cast' : cannot convert from 'const unsigned short *' to 'unsigned short *'
        Conversion loses qualifiers

any ideas?
try casting like this?

NS_STATIC_CAST(const PRUnichar *, (*array[i]).get());

(Assignee)

Comment 12

16 years ago
that will not work because (*returnPropertiesArray)[i] is char*. 

(*returnPropertiesArray)[i] = NS_STATIC_CAST(const char*, (*array[i]).get());

(Assignee)

Updated

16 years ago
No longer depends on: 116983
(Assignee)

Comment 13

16 years ago
Created attachment 63913 [details] [diff] [review]
proposed fix

NS_CONST_CAST() works, tested it. there are three/four instances when we 
don't do a copy- all of them in Search() and the use is to convert from
nsCStringArray to charPtrArrayGuard and then back to nsCStringArray. we
are not stomping it.
Attachment #63879 - Attachment is obsolete: true
(Assignee)

Comment 14

16 years ago
fixed
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
I tried to reproduce this (after the fix) and then expanded all of my PR_Malloc
stacks, saved to a .txt file, and grepped the logfile for "ab".  All instances
came up with XPCNativeScriptableSharedMap::GetNewOrUsed, so the addressbook
leaks seem to be gone.

Verified FIXED with the latest trunk, Windows 2000 under Purify.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.