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

Status: VERIFIED DUPLICATE of bug 438922

(MailNews Core :: Backend, defect)

Description

[Serge Gautherie (:sgautherie)]

[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

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: test_maildb.js.log

A log example

Comment 2

[Serge Gautherie (:sgautherie)]

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

Comment 3

[Serge Gautherie (:sgautherie)]

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

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (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)

Comment 5

[Mark Banner (:standard8)]

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.

Comment 6

[Serge Gautherie (:sgautherie)]

(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 7

[Mark Banner (:standard8)]

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.

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Unregister the provider

Av1, plus the fix.

(In reply to comment #7)

An observer is not needed here, is it ?

Comment 9

[Mark Banner (:standard8)]

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.

Comment 10

[Serge Gautherie (:sgautherie)]

(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.)

Comment 11

[Serge Gautherie (:sgautherie)]

Update status:
This can now be tracked on tinderboxes.
I'll soon post a patch for bug 384339...

Comment 12

[Mark Banner (:standard8)]

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.

Comment 13

[Serge Gautherie (:sgautherie)]

You could have used this bug. Anyway.

V.Duplicate

Comment 14

[Serge Gautherie (:sgautherie)]

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


Abandoned, per comment 7 (and comment 9)