Memory leak (n * 60 bytes) when tags are migrated from version 1 to 2

RESOLVED FIXED in mozilla1.9

Status

MailNews Core
Backend
--
minor
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

({memory-leak})

Trunk
mozilla1.9
memory-leak

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

10 years ago
When we call GetAllKeys from within nsMsgTagService::MigrateLabelsToTags, we're freeing the tag array with NS_Free, we should be using NS_FREE_XPCOM_ISUPPORTS_POINTER_ARRAY. So we're leaking nsMsgTag items (60 bytes a piece).

I think its very unlikely we'll hit this normally (as 1.8 branch is on labels which skips the tags v1->v2 migration bit), but it is still wrong and should be fixed.

To reproduce, set mailnews.tags.version to 1 and start SM/TB with XPCOM_MEM_BLOAT_LOG=1 set, open up the tags preference pane and then exit. Find there is nsMsgTag in the bloat log, being leaked 5 times (300 bytes) - 10 were created.

We're also not null-checking pointers on entry into the GetAllKeys function.
(Assignee)

Comment 1

10 years ago
Created attachment 318471 [details] [diff] [review]
The fix
Attachment #318471 - Flags: superreview?(neil)
Attachment #318471 - Flags: review?(neil)
(Assignee)

Updated

10 years ago
Depends on: 431414
(Assignee)

Updated

10 years ago
Blocks: 431414
No longer depends on: 431414
(Assignee)

Comment 2

10 years ago
Created attachment 318562 [details] [diff] [review]
[checked in] The fix v2

Fixes an addition leak case (although probably unlikely to hit it much) where we run out of memory for tags, and then return without freeing the child prefs array.
Attachment #318471 - Attachment is obsolete: true
Attachment #318562 - Flags: superreview?(neil)
Attachment #318562 - Flags: review?(neil)
Attachment #318471 - Flags: superreview?(neil)
Attachment #318471 - Flags: review?(neil)

Comment 3

10 years ago
Comment on attachment 318562 [details] [diff] [review]
[checked in] The fix v2

Do you want to do the one on line 415 too? (Note: tricky!)
Attachment #318562 - Flags: superreview?(neil)
Attachment #318562 - Flags: superreview+
Attachment #318562 - Flags: review?(neil)
Attachment #318562 - Flags: review+
(Assignee)

Comment 4

10 years ago
Created attachment 319774 [details] [diff] [review]
Fix the other leak

Fixes the one on line 415. Note you may need "The fix v2" applied before applying this one. I also created a temporary variable for the tagArray so that we don't have to worry about nulling it out if the function fails, and it makes it a bit tidier (not so much de-referencing).
Attachment #319774 - Flags: superreview?(neil)
Attachment #319774 - Flags: review?(neil)

Comment 5

10 years ago
Comment on attachment 319774 [details] [diff] [review]
Fix the other leak

>+              NS_FREE_XPCOM_ISUPPORTS_POINTER_ARRAY(currentTagIndex, tagArray);
Don't you have to free the preferences here too?
(Assignee)

Comment 6

10 years ago
Created attachment 319777 [details] [diff] [review]
Fix the other leak v2

Correct, we would have to free the preference ones as well.
Attachment #319774 - Attachment is obsolete: true
Attachment #319777 - Flags: superreview?(neil)
Attachment #319777 - Flags: review?(neil)
Attachment #319774 - Flags: superreview?(neil)
Attachment #319774 - Flags: review?(neil)

Comment 7

10 years ago
Comment on attachment 319777 [details] [diff] [review]
Fix the other leak v2

>-  // return the number of tags
>-  // (the idl's size_is(count) parameter ensures that the array is cut accordingly)
I think you should try to retain this comment (or something like it).
Attachment #319777 - Flags: superreview?(neil)
Attachment #319777 - Flags: superreview+
Attachment #319777 - Flags: review?(neil)
Attachment #319777 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #318562 - Attachment description: The fix v2 → [checked in] The fix v2
(Assignee)

Comment 8

10 years ago
Created attachment 320159 [details] [diff] [review]
[checked in] Fix the other leak v3 (unbitrotted)

Unbitrotted version, this is what I'm checking in.
Attachment #319777 - Attachment is obsolete: true
Attachment #320159 - Flags: superreview+
Attachment #320159 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #320159 - Attachment description: Fix the other leak v3 (unbitrotted) → [checked in] Fix the other leak v3 (unbitrotted)
(Assignee)

Comment 9

10 years ago
Both patches checked in, this in now fixed.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Target Milestone: --- → mozilla1.9
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.