Several IMAP tests have reported leak (including nsRDFResource)

RESOLVED FIXED in Thunderbird 3.0b3

Status

MailNews Core
Networking: IMAP
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: standard8, Assigned: Bienvenu)

Tracking

({memory-leak})

Trunk
Thunderbird 3.0b3
memory-leak
Bug Flags:
wanted-thunderbird3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 383250 [details]
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.
Flags: wanted-thunderbird3+
(Reporter)

Updated

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

Comment 1

9 years ago
taking
Assignee: nobody → bienvenu
Status: NEW → ASSIGNED
(Assignee)

Comment 2

9 years ago
Created attachment 384182 [details] [diff] [review]
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...
Attachment #384182 - Flags: review?(bugzilla)
(Assignee)

Comment 3

9 years ago
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.
(Reporter)

Comment 4

9 years ago
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).
Attachment #384182 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 5

9 years ago
very odd - I wasn't seeing that before, but now I do see it when I do a check-one.
(Assignee)

Comment 6

9 years ago
Created attachment 384238 [details] [diff] [review]
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.
Attachment #384182 - Attachment is obsolete: true
Attachment #384238 - Flags: review?(bugzilla)
(Assignee)

Comment 7

9 years ago
(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).
(Reporter)

Comment 8

9 years ago
(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.
(Reporter)

Comment 9

9 years ago
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.
Attachment #384238 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 10

9 years ago
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...
(Assignee)

Updated

9 years ago
Attachment #384238 - Attachment description: add fix for hang in folder listener imap test → add fix for hang in folder listener imap test - checked in.
(Assignee)

Comment 11

9 years ago
Created attachment 384344 [details] [diff] [review]
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.
Attachment #384344 - Flags: superreview?(bugzilla)
Attachment #384344 - Flags: review?(bugzilla)
(Reporter)

Updated

9 years ago
Attachment #384344 - Flags: superreview?(bugzilla)
Attachment #384344 - Flags: superreview+
Attachment #384344 - Flags: review?(bugzilla)
Attachment #384344 - Flags: review+
(Reporter)

Comment 12

9 years ago
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.
(Assignee)

Comment 13

9 years ago
I've added a comment as to why we're remembering if the db was open or not before the function is called...
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b3
Flags: in-testsuite+
(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.
You need to log in before you can comment on or make changes to this bug.