Closed Bug 462955 Opened 11 years ago Closed 11 years ago
News TUnit tests have +/- the same leaks
[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
A log example
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
[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 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-
Av1, plus the fix. (In reply to comment #7) An observer is not needed here, is it ?
Target Milestone: --- → mozilla1.9.1b2
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.)
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
Resolution: --- → DUPLICATE
Duplicate of bug: 438922
You could have used this bug. Anyway. V.Duplicate
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.