All(!?) MailNews TUnit tests have +/- the same leaks

VERIFIED DUPLICATE of bug 438922

Status

MailNews Core
Backend
VERIFIED DUPLICATE of bug 438922
10 years ago
9 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

({memory-leak})

Trunk
Thunderbird 3.0b3
memory-leak

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 obsolete attachments)

(Assignee)

Description

10 years ago
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081103 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)

14 test_addbook
 3 test_bayes
 7 test_compose
 1 test_imap
 2 test_import
14 test_mailnewsbase
 1 test_mailnewsdb
 5 test_mailnewslocal
 3 test_news
(Assignee)

Comment 1

10 years ago
Created attachment 346160 [details]
test_maildb.js.log

A log example
(Assignee)

Updated

10 years ago
Depends on: 456414
(Assignee)

Comment 2

10 years ago
I'm testing with test_mailboxes.js:
the leak is triggered by |do_import_script("../mailnews/test/resources/mailDirService.js");| in head_server.js.
(Assignee)

Comment 3

10 years ago
The trigger is
|dirSvc.QueryInterface(Ci.nsIDirectoryService).registerProvider(provider);|

I verified that adding an |unregisterProvider()| call would fix this leak.
See
http://mxr.mozilla.org/mozilla-central/ident?i=unregisterProvider
Blocks: 426615
(Assignee)

Comment 4

10 years ago
Created attachment 346181 [details] [diff] [review]
(Av1) simple nits and doc

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081103 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)
Attachment #346160 - Attachment is obsolete: true
Attachment #346181 - Flags: review?(bugzilla)
Comment on attachment 346181 [details] [diff] [review]
(Av1) simple nits and doc

I see no unregisterProvider call. All I can see is a possibility of reducing leaks in some cases. Please explain what you are doing here and where you are planning on going.
(Assignee)

Comment 6

10 years ago
(In reply to comment #5)
> I see no unregisterProvider call.

Right, this first patch is cleanup only.

> Please explain [...] where you are planning on going.

I hoped you would add the unregisterProvider call in the way you like...

I could fix this myself if you gave me a hint of how you want it done:
*add a global var as a reminder of the provider object only.
*move the whole |provider| to be a global const.
*simply move it up from makeDirectoryService() to initializeDirServer(), and add a parameter to the latter, and maybe rename the function.
*...
Comment on attachment 346181 [details] [diff] [review]
(Av1) simple nits and doc

Standalone, I don't think we need this tidy up, the patch is to test code only and its clean enough that it works.

For the unregister provider that would probably require a solution like bug 438922 which is currently blocked by bug 384339.
Attachment #346181 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 8

10 years ago
Created attachment 348601 [details] [diff] [review]
(Bv1) Unregister the provider

Av1, plus the fix.

(In reply to comment #7)

An observer is not needed here, is it ?
Assignee: nobody → sgautherie.bz
Attachment #346181 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #348601 - Flags: review?(bugzilla)
(Assignee)

Updated

10 years ago
Blocks: 408905
Flags: wanted-thunderbird3?
Target Milestone: --- → mozilla1.9.1b2
(Assignee)

Updated

10 years ago
Blocks: 465376
Comment on attachment 348601 [details] [diff] [review]
(Bv1) Unregister the provider

It took me a while to work out why this works, which is that once we get a directory from nsIDirectoryService it caches it. Hence the next time we get it, it get it out the cache and won't try the provider.

This patch won't work now we have the extra option for the gloda tests.

I guess you could get that directory temporarily as well if you want to fix this before we fix bug 438922. Though you should also add comments explaining why we can do unregisterProvider in case something regresses or something causes weird going ons.

I also don't like the way the const provider has moved into the catch. Please keep it where it is, although you'll need to reindent it of course.
Attachment #348601 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 10

10 years ago
(In reply to comment #9)
> [...] once we get a directory from nsIDirectoryService it caches it.

Ah :-|

> This patch won't work now we have the extra option for the gloda tests.
> 
> I guess you could get that directory temporarily as well if you want to fix

I'm not sure what this is referring to ...

> this before we fix bug 438922.

... so let's wait for that bug for now.

> I also don't like the way the const provider has moved into the catch.

(Sure.)
(Assignee)

Updated

10 years ago
Depends on: 469523
No longer depends on: 456414
(Assignee)

Updated

9 years ago
Depends on: 485583
(Assignee)

Comment 11

9 years ago
Update status:
This can now be tracked on tinderboxes.
I'll soon post a patch for bug 384339...
OS: Windows 2000 → All
Serge, thanks for fixing bug 384339.

The plan is that I'll fix most of the generic leaks in bug 438922 and file bugs for the new ones. So I think we don't need this bug as well as bug 438922. Therefore I'll mark it as dupe. I'm just about to comment on bug 438922 with my plan for landing it.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: wanted-thunderbird3?
Resolution: --- → DUPLICATE
Duplicate of bug: 438922
(Assignee)

Updated

9 years ago
No longer blocks: 465376
(Assignee)

Updated

9 years ago
No longer blocks: 426615
(Assignee)

Updated

9 years ago
No longer depends on: 485583
(Assignee)

Comment 13

9 years ago
You could have used this bug. Anyway.

V.Duplicate
No longer blocks: 408905
Status: RESOLVED → VERIFIED
No longer depends on: 438922, 469523
Hardware: x86 → All
Target Milestone: mozilla1.9.1b2 → Thunderbird 3.0b3
(Assignee)

Comment 14

9 years ago
Comment on attachment 348601 [details] [diff] [review]
(Bv1) Unregister the provider


Abandoned, per comment 7 (and comment 9)
Attachment #348601 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.