Closed Bug 116972 Opened 23 years ago Closed 23 years ago

MLK: leaks in addrbook upon autocompletion and sending mail

Categories

(SeaMonkey :: MailNews: Address Book & Contacts, defect)

Product:
SeaMonkey
Component:
MailNews: Address Book & Contacts
Platform:
x86
Windows NT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: naving, Assigned: naving)

Details

(Keywords: memory-leak)

Attachments

(1 file, 2 obsolete files)

proposed fix
1.87 KB, patch
Details | Diff | Splinter Review 
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
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
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]
Attached patch proposed fix (obsolete) — — Splinter Review 
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.
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.
Attached patch proposed fix (obsolete) — — Splinter Review 
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 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+
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());


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

(*returnPropertiesArray)[i] = NS_STATIC_CAST(const char*, (*array[i]).get());
No longer depends on: 116983
Attached patch proposed fix — — Splinter Review 
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
fixed
Status: NEW → RESOLVED
Closed: 23 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.

Attachment

General

Creator:
Created:
Updated:
Size: