270215 - Memory leak in wallet_GetLine

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Philip K. Warren]

I found the following memory leak in a SeaMonkey trunk build using Valgrind:

==6989== 54742 bytes in 4705 blocks are definitely lost in loss record 307 of 312
==6989==    at 0x1B904A90: malloc (vg_replace_malloc.c:131)
==6989==    by 0x1BB36788: PR_Malloc (prmem.c:436)
==6989==    by 0x1BA0B71B: NS_Alloc_P (nsMemoryImpl.cpp:367)
==6989==    by 0x1B97E899: nsMemory::Alloc(unsigned) (nsMemory.h:68)
==6989==    by 0x1BA2AA0C: char* AllocateStringCopy<nsACString, char>(nsACString
const&, char*) (nsReadableUtils.cpp:310)
==6989==    by 0x1BA28AD7: ToNewCString(nsACString const&) (nsReadableUtils.cpp:355)
==6989==    by 0x1C0E0466: wallet_GetLine (wallet.cpp:1133)
==6989==    by 0x1C0E0E36: wallet_ReadFromFile(char const*, nsVoidArray*&, int,
PlacementType) (wallet.cpp:1308)
==6989==    by 0x1C0E3D0C: wallet_Initialize(int) (wallet.cpp:2332)
==6989==    by 0x1C0E8F9E: WLLT_OnSubmit (wallet.cpp:3693)

Comment 1

[Philip K. Warren]

Attachment: Patch v1

The problem is that the helpMac->item3 string is assigned to several times over
the course of the loop without being freed. To simplify the memory management
in this file, I have converted all of the helpMac members to nsCString's.

Comment 2

[Darin Fisher]

Comment on attachment 166131 [details] [diff] [review]
Patch v1

It might be good to turn sublist->item into a nsCString as well :)

sr=darin

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 166131 [details] [diff] [review]
Patch v1

+      wallet_WriteToList(helpMac->item1.get(), helpMac->item1.get(), dummy,
list, PR_FALSE, placement);

maybe WriteToList should also take string classes...

-    if (WALLET_NULL(helpMac->item3)) {

is WALLET_NULL maybe unused now?

Comment 4

[Philip K. Warren]

(In reply to comment #3)
> (From update of attachment 166131 [details] [diff] [review])
> +      wallet_WriteToList(helpMac->item1.get(), helpMac->item1.get(), dummy,
> list, PR_FALSE, placement);
> maybe WriteToList should also take string classes...

I would like to get a lot of this code using the string classes, but will do 
this in a separate defect. I just wanted to try and address the memory leak 
first.

> -    if (WALLET_NULL(helpMac->item3)) {
> is WALLET_NULL maybe unused now?

Unfortunately not. With the patch it is still used to two other places. It 
would be nice to eventually remove that and the WALLET_FREE and WALLET_FREEIF 
macros as well.

Comment 5

[Philip K. Warren]

Darin has filed Bug 270248 to convert more of the wallet code over to using 
the string classes.

Comment 6

[Philip K. Warren]

Fixed.

Checking in singsign.cpp;
/cvsroot/mozilla/extensions/wallet/src/singsign.cpp,v  <--  singsign.cpp
new revision: 1.250; previous revision: 1.249
done
Checking in wallet.cpp;
/cvsroot/mozilla/extensions/wallet/src/wallet.cpp,v  <--  wallet.cpp
new revision: 1.371; previous revision: 1.370
done
Checking in wallet.h;
/cvsroot/mozilla/extensions/wallet/src/wallet.h,v  <--  wallet.h
new revision: 1.49; previous revision: 1.48
done