129312 - leaking msg hdrs again, so db's don't get closed

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P1)

Description

[David :Bienvenu]

In the past few days, we've started leaking msg hdrs again when switching
folders, so we leave the db's open and the memory starts to bloat. Not sure what
change caused this - a build from two weeks ago didn't have this problem, and I
think it was really in the past 5 days or so that this was broken.

Comment 1

[David :Bienvenu]

March 1 build is fine.

Comment 2

[David :Bienvenu]

March 4th build seems OK too. So it's just one of the few hundred checkins
between the 4th and 6th.

Comment 3

[David :Bienvenu]

03/05 build seems to have this problem, so it was probably a checkin between
03/04 and 03/05, most likely one of these:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=03%2F04%2F2002+08%3A00%3A00&maxdate=03%2F05%2F2002+08%3A00%3A00&cvsroot=%2Fcvsroot

Comment 4

[David :Bienvenu]

OK, this is caused by Sean's checkin of Neil's perfectly harmless-looking patch
to check the imap deleted flag for that .01% of the user base that use that
delete model:

return gDBView.hdrForFirstSelectedMessage.flags & MSG_FLAG_IMAP_DELETED;

this gets the header object into js, where it may or may not ever be garbage
collected. If I comment this out, we close the db's correctly again.

This was bug 82056. An easy way to fix this is to never allow the hdr into js,
and just add a method to get the flag for the first selected message. We don't
ever need to get the header anyway, since the view maintains the flags
separately for display purposes. I'll look into a fix for this.

Comment 5

[Sean Su]

If you add a method to get the flag for the first selected message, could you
also add a sister method that gets the flag for a given URI?  That would take
care of bug 28916 which helps out with bug 30560.

Comment 6

[Sean Su]

oops, I meant to say: That would take care of bug 128916 which helps out with
bug 30560.

Comment 7

[David :Bienvenu]

If I clear the message window by doing multiple selection, before selecting
another folder, the hdr is garbage collected. But if I don't, as near as I can
tell, the header is never released. I've also found that other callers of
hdrForFirstSelectedMessage (e.g., as used to init the mark menu) also cause this
leak. I'm worried that there's something very strange going on here, so I might
need to spend some time getting to the bottom of it instead of just fixing the
immediate cause of the problem.

Comment 8

[David :Bienvenu]

OK, I've found the root cause of this problem - it's all my fault. When I added
code to make sure we didn't crash trying to free headers that belong to db's
that have been forced closed, I cleared out the mdb row pointer in the header.
This worked fine if no one was holding onto the header after you switch folders,
but the imap delete menu code made it so that someone was always holding onto a
header. Fix upcoming, please review.

Comment 9

[David :Bienvenu]

Attachment: proposed fix

The fix is to only clear out the db row member variable in the msg hdr if the
db is actually going away, e.g., we're clearing out the cache from
::ForceClosed. Please review.

Comment 10

[David :Bienvenu]

Navin, can you review? thx.

Comment 11

[Navin Gupta]

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

r=naving

Comment 12

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 13

[(not reading, please use [e-mail] instead)]

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

sr=sspitzer

Comment 14

[scottputterman]

Is this the likely cause for why the mail tinderbox leak numbers went up? 
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey-Testerbox

Comment 15

[David :Bienvenu]

If you're talking about the change from 1.21MB to 1.22MB, that looks like it
happened around the time of darin's uri changes. This doesn't cause a leak
because we clean up open db's at shutdown time anyway.

Comment 16

[David :Bienvenu]

fixed

Comment 17

[[e-mail] (gone - use [e-mail] instead)]

using bienvenu's testcase in comment 7 with the latest trunk Windows 2000 build
under Purify, I don't see this leak.

Verified FIXED.