Closed Bug 462955 Opened 11 years ago Closed 11 years ago

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

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED DUPLICATE of bug 438922
Thunderbird 3.0b3

People

(Reporter: sgautherie, Assigned: sgautherie)

References

()

Details

(Keywords: memory-leak)

Attachments

(3 obsolete files)

[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
Attached file test_maildb.js.log (obsolete) —
A log example
Depends on: 456414
I'm testing with test_mailboxes.js:
the leak is triggered by |do_import_script("../mailnews/test/resources/mailDirService.js");| in head_server.js.
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
Attached patch (Av1) simple nits and doc (obsolete) — Splinter Review
[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.
(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-
Attached patch (Bv1) Unregister the provider (obsolete) — Splinter Review
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)
Blocks: mlkTests
Flags: wanted-thunderbird3?
Target Milestone: --- → mozilla1.9.1b2
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-
(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.)
Depends on: 469523
No longer depends on: 456414
Depends on: 485583
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
Closed: 11 years ago
Flags: wanted-thunderbird3?
Resolution: --- → DUPLICATE
Duplicate of bug: 438922
No longer blocks: 465376
No longer blocks: 426615
No longer depends on: 485583
You could have used this bug. Anyway.

V.Duplicate
No longer blocks: mlkTests
Status: RESOLVED → VERIFIED
No longer depends on: 438922, 469523
Hardware: x86 → All
Target Milestone: mozilla1.9.1b2 → Thunderbird 3.0b3
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.