498323 - Several IMAP tests have reported leak (including nsRDFResource)

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Mark Banner (:standard8)]

Attachment: Typical leak log

After part 2 of bug 438922 has landed to clean up the leak logs, several of the imap tests still leak. nsRDFResource and RDFServiceImpl are notable highlights.

Current failures are:

test_nsIMsgFolderListenerIMAP.js
test_imapContentLength.js
test_bug460636.js

Typical logs attached.

Comment 1

[David :Bienvenu]

taking

Comment 2

[David :Bienvenu]

Attachment: proposed fix

the tests are assuming the uri is user@localhost, but we're not creating the account with the correct name. This causes RDF to create new imap folders and use those, but those folders aren't hooked up to the server/account, and thus don't get cleaned up...

Comment 3

[David :Bienvenu]

test_imapContentLength.js seems like it might still leaking - preliminary investigation shows that it's the imap archive folder, of all folders, that's leaking - I think this gets instantiated because it's the default folder for the imap server.

It doesn't seem to leak when I do a check-one, but when I check all, that test leaks. Since that doesn't make a lot of sense, I might be a bit confused.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 384182 [details] [diff] [review]
proposed fix

With the patch applied, I'm seeing test_nsIMsgFolderListenerIMAP.js hanging part way through. I haven't had time to debug yet.

FYI the leak on test_compactOfflineStore.js is still always present, regardless of if I do check-one or xpcshell-tests (note check-interactive doesn't show leaks).

Comment 5

[David :Bienvenu]

very odd - I wasn't seeing that before, but now I do see it when I do a check-one.

Comment 6

[David :Bienvenu]

Attachment: add fix for hang in folder listener imap test - checked in.

the imap folder listener test was working around a bug caused by the bad account definition, and listening for the deletion of the inbox. Removing that work-around fixed the hang for me.

Comment 7

[David :Bienvenu]

(In reply to comment #4)

> FYI the leak on test_compactOfflineStore.js is still always present, regardless
> of if I do check-one or xpcshell-tests (note check-interactive doesn't show
> leaks).

Is that on the mac? On windows, at least with my debug build, it only leaks with xpcshell-tests. I suspect it has to do with the timing of shutting down the imap server/threads. I notice that the test_imapContentLength.js test doesn't leak and it cleans up the server on a timeout (though my first attempt at making test_compactOfflineStore.js hung).

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #7)
> (In reply to comment #4)
> 
> > FYI the leak on test_compactOfflineStore.js is still always present, regardless
> > of if I do check-one or xpcshell-tests (note check-interactive doesn't show
> > leaks).
> 
> Is that on the mac? On windows, at least with my debug build, it only leaks
> with xpcshell-tests. I suspect it has to do with the timing of shutting down
> the imap server/threads. I notice that the test_imapContentLength.js test
> doesn't leak and it cleans up the server on a timeout (though my first attempt
> at making test_compactOfflineStore.js hung).

This is on mac, but it is test_imapContentLength.js that is leaking, not what I said in my comment.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 384238 [details] [diff] [review]
add fix for hang in folder listener imap test - checked in.

r=Standard8. So this leaves us with the reported leaks covered by bug 498307, and the test_imapContentLength.js set.

We can move test_imapContentLength.js if you want, or we can try and fix it here.

Comment 10

[David :Bienvenu]

I'm not seeing the test_imapContentLength.js leak, though from the Tinderbox logs, it sure looks like the same kind of leak. I'd just leave this open - I'll try it on my mac at some point...

Comment 11

[David :Bienvenu]

Attachment: proposed fix

The leak appears to be caused by a combination of factors - we default to an imap Archives folder, and when imap folder discovery is done, we set the special flags on the special folders based on the prefs. Because of the nature of RDF, this gives us an Archives folder that's free floating - it has no parent, and is nobody's child. Setting the Archives flag on this folder causes us to open the db. We don't close the db at shutdown as we would normally because it's not in the folder hierarchy.  This fix makes it so we close the db right after opening it, in the case that we had a brand new profile (ReadDBFolderInfo does not have the value initialized from the folder cache like it would normally).

I'm not sure why the other imap tests don't run into this issue - maybe it's because this test doesn't do the performExpand() some of the other tests do.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 384344 [details] [diff] [review]
proposed fix

Hmm, I did mean to say I wonder if it is worth having a brief comment to explain why we're doing the ReadDBFolderInfo then the check for dbWasOpen.

Comment 13

[David :Bienvenu]

I've added a comment as to why we're remembering if the db was open or not before the function is called...

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> (In reply to comment #4)
> 
> > FYI the leak on test_compactOfflineStore.js is still always present, regardless
> > of if I do check-one or xpcshell-tests (note check-interactive doesn't show
> > leaks).
> 
> Is that on the mac? On windows, at least with my debug build, it only leaks
> with xpcshell-tests.

I filed bug 503762.