Memory leak in wallet_GetLine

RESOLVED FIXED

Status

()

Toolkit
Form Manager
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: Philip K. Warren, Assigned: Philip K. Warren)

Tracking

({mlk})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

13 years ago
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)
(Assignee)

Comment 1

13 years ago
Created attachment 166131 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Attachment #166131 - Flags: superreview?(darin)
Attachment #166131 - Flags: review?(cbiesinger)
(Assignee)

Updated

13 years ago
Keywords: mlk

Comment 2

13 years ago
Comment on attachment 166131 [details] [diff] [review]
Patch v1

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

sr=darin
Attachment #166131 - Flags: superreview?(darin) → superreview+
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?
Attachment #166131 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 4

13 years ago
(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.
(Assignee)

Comment 5

13 years ago
Darin has filed Bug 270248 to convert more of the wallet code over to using 
the string classes.
Status: NEW → ASSIGNED
(Assignee)

Comment 6

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

9 years ago
Component: Form Manager → Form Manager
Product: Core → Toolkit
QA Contact: form.manager
You need to log in before you can comment on or make changes to this bug.