Memory leak of 40 bytes from 1 block allocated in nsImportModuleList::AddModule

VERIFIED FIXED

Status

SeaMonkey
MailNews: Address Book & Contacts
--
major
VERIFIED FIXED
16 years ago
14 years ago

People

(Reporter: stephend@netscape.com (gone - use stephen.donner@gmail.com instead), Assigned: Cavin Song)

Tracking

({memory-leak})

Trunk
x86
Windows 2000
memory-leak

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

752 bytes, patch
Cavin Song
: review+
(not reading, please use seth@sspitzer.org instead)
: superreview+
Details | Diff | Splinter Review
Build ID: latest win32 trunk under Purify.

Summary: nsImportModuleList::AddModule(nsID const&,char const*,WORD const*,WORD
const*)

Steps to Reproduce:

1. With an empty addressbook, do: File | Import.
2. Click on Address Book, then Next.
3. Click on the LDIF (.csv and .txt) choice.
4. When the dialog comes up choose an LDIF file (1446 entries).
5. Click Finish when done.

[W] MLK: Memory leak of 40 bytes from 1 block allocated in
nsImportModuleList::AddModule(nsID const&,char const*,WORD const*,WORD const*)
        Distribution of leaked blocks
        Allocation location
            new(UINT)      [MSVCRT.DLL]
            nsImportModuleList::AddModule(nsID const&,char const*,WORD
const*,WORD const*) [nsImportService.cpp:770]
                {
                    if (m_pList == nsnull) {
                        m_alloc = 10;
             =>         m_pList = new ImportModuleDesc *[m_alloc];
                        m_count = 0;
                        memset( m_pList, 0, sizeof( ImportModuleDesc *) * m_alloc);
                    }
            nsImportService::LoadModuleInfo(char const*,char const*)
[nsImportService.cpp:677]
                        theDescription.Assign(NS_LITERAL_STRING("Unknown
description"));
                
                    // call the module to get the info we need
             =>     m_pModules->AddModule( clsId, pSupports, theTitle.get(),
theDescription.get());
                
                    module->Release();
                
            nsImportService::DoDiscover(void) [nsImportService.cpp:626]
                        nsXPIDLCString    supportsStr;
                        rv = catMan->GetCategoryEntry( "mailnewsimport",
contractIdStr, getter_Copies( supportsStr));
                        if (NS_SUCCEEDED( rv)) {
             =>             LoadModuleInfo( contractIdStr, supportsStr);
                        }
                        rv = e->GetNext( getter_AddRefs( contractid));
                    }
            nsImportService::GetModuleCount(char const*,int *)
[nsImportService.cpp:416]
                    if (! _retval)
                        return NS_ERROR_NULL_POINTER;
                
             =>     DoDiscover();
                
                    if (m_pModules != nsnull) {
                        ImportModuleDesc *    pDesc;
            XPTC_InvokeByIndex [xptcinvoke.cpp:105]
           
XPCWrappedNative::CallMethod(XPCCallContext&,CallMode::XPCWrappedNative)
[xpcwrappednative.cpp:1998]
            XPC_WN_CallMethod(JSContext *,JSObject *,UINT,long *,long *)
[xpcwrappednativejsops.cpp:1266]
            js_Invoke      [jsinterp.c:788]
            js_Interpret   [jsinterp.c:2745]
            js_Invoke      [jsinterp.c:805]
            js_InternalInvoke [jsinterp.c:880]
            JS_CallFunctionValue [jsapi.c:3380]
            nsJSContext::CallEventHandler(void *,void *,UINT,void *,int *,int)
[nsJSEnvironment.cpp:1016]
            nsJSEventListener::HandleEvent(nsIDOMEvent *)
[nsJSEventListener.cpp:180]
            nsEventListenerManager::HandleEventSubType(nsListenerStruct
*,nsIDOMEvent *,nsIDOMEventTarget *,UINT,UINT) [nsEventListenerManager.cpp:1217]
            nsEventListenerManager::HandleEvent(nsIPresContext *,nsEvent
*,nsIDOMEvent * *,nsIDOMEventTarget *,UINT,nsEventStatus *)
[nsEventListenerManager.cpp:1890]
            GlobalWindowImpl::HandleDOMEvent(nsIPresContext *,nsEvent
*,nsIDOMEvent * *,UINT,nsEventStatus *) [nsGlobalWindow.cpp:684]
            DocumentViewerImpl::LoadComplete(UINT) [nsDocumentViewer.obj:1298]
            nsDocShell::EndPageLoad(nsIWebProgress *,nsIChannel *,UINT)
[nsDocShell.cpp:3734]
Keywords: mlk
QA Contact: nbaca → stephend
Summary: nsImportModuleList::AddModule(nsID const&,char const*,WORD const*,WORD const*) → Memory leak of 40 bytes from 1 block allocated in nsImportModuleList::AddModule

Comment 1

16 years ago
Created attachment 70904 [details] [diff] [review]
Possible patch.

|m_pList| is declared as ImportModuleDesc **, doing |delete [] m_pList| does
not destroy the original objects that are copied to a new location on the line
above. This occurs if we need more than 10 ImportModuleDesc objects.

This file seems to contain tabs. I used space in the patch.

Comment 2

16 years ago
Created attachment 70925 [details] [diff] [review]
Patch, second attempt.

ClearList() resets m_alloc among other things. Silly mistake, this patch should
behave more correctly.
Keywords: patch, review

Comment 3

16 years ago
There's a fix attached.  Naving, Bienvenu, can you have a look at it?

Comment 4

16 years ago
Created attachment 74438 [details] [diff] [review]
Patch, third attempt.

This one is hard to get right. The previous patches are wrong. 

According to bug #126855 ClearList() uses nsMemory::Free() wrongly. My previous
patch copied that behaviour which is bad. This is updated to use delete []
instead.
(Assignee)

Comment 5

16 years ago
Comment on attachment 74438 [details] [diff] [review]
Patch, third attempt.

r=cavin.
Attachment #74438 - Flags: review+
Attachment #70904 - Attachment is obsolete: true
Attachment #70925 - Attachment is obsolete: true
Comment on attachment 74438 [details] [diff] [review]
Patch, third attempt.

looks good.

on a related note, should we change

nsMemory::Free( m_pList);
in ClearList() to be 

delete [] m_pList;
Attachment #74438 - Flags: superreview+

Comment 7

16 years ago
That is bug 126855 I believe, which is a dupe of bug 143035 - the patch hanging
there fixes the issue mentioned as far as I can see. Since this has both r and
sr I'd appreciate if someone would check it in. 
(Assignee)

Comment 8

16 years ago
> nsMemory::Free( m_pList);
> in ClearList() to be 
>
> delete [] m_pList;
>
Yes, the above fix is covered by bug 143035. So, I'll just check in the attached
patch.
(Assignee)

Comment 9

16 years ago
Fix landed on the trunk.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

16 years ago
Thanks to Peter A Jonsson for the patch.
Verified fixed with http://rocknroll/users/nbaca/publish/MachV/574.ldif and the 
latest trunk running under Purify on Win2k.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.