114211 - Request MSF files be closed when not in use (pop3 filter only!)

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect, P2)

Description

[David A. Cobb]

Machines will go down - Windoze more often than some.  Mozilla keeps as many as
60 or more .MSF files open after a transfer-filter operation.  Because they are
open, if the system crashes they are all found to be damaged on reboot.  The
disk scan reports a SizeError in the allocation for a very large number of the
.MSF files; I have found there is no way to "fix" the files - they need to be
deleted.  The disk scan then discovers over a Meg of file space marked as InUse
but not allocated to any file.  
My recommendation would be to close the files after the transfer-filter
operation completes -- this will update the directory allocations and flush the
cache, then re-open as needed to satisfy the user's operations.  Damage is much
less likely when the files are not being extended - rewrite in place is probably
not a big vulnerability.

Comment 1

[Boris Zbarsky [:bzbarsky]]

confirming, but the performance impact should be investigated...

Comment 2

[scottputterman]

reassigning to bienvenu. This sounds like the conversation we had last week
where it looked like we were closing everything down. 

Comment 3

[David :Bienvenu]

it's not dataloss, since those files don't have any information that can't be
recreated. Also, the files aren't corrupt - the data in them has been written
out to disk.

Comment 4

[David A. Cobb]

I won't argue the first point - although I don't agree.
However, if the filesystem structures aren't updated the file is damned well
corrupt, whether the data bits happen to be in some unreachable sector of the
disk or not!
Obviously, some files are open and thus at risk all the time.  The fault is that
there are far far too many.  It's not like this was a real operating system,
after all; the application needs to minimize its exposure to the inevitable badness.

Comment 5

[David :Bienvenu]

Sorry if I wasn't clear, the data is recreated automatically by mozilla, because
all the info is stored either on the server or in your local mailbox (I'm
guessing you're a pop3 user). I'm not being argumentative, but dataloss has a
serious meaning and we shouldn't cheapen it by using it where it doesn't apply.

We attempt to close db's when not in use, but unfortunately there is client code
that leaks objects that prevent the db from being closed. We're trying to track
down those leaks but it's not trivial.

Comment 6

[David :Bienvenu]

right now, we're only closing db's after you read a folder, and after you search
a folder, but not after messages get moved into target folders.  I can add code
to do that.

Comment 7

[David :Bienvenu]

actually, I think there's some sort of leak in the copy code that's holding the
db's open. I thought it might have been the undo code, but now I think it might
be the move copy state, or the js.

Comment 8

[Henrik Gemal]

closing mail/news (leaving the browser open) should release the .msf files, rite?

Comment 9

[David :Bienvenu]

I'm a little torn about how to fix this. I could just close db's of folders that
get messages moved/copied into them. That would be fairly easy and would keep
most db's closed. Or, I could do something more general and close db's after
they've been inactive for a certain period of time (e.g., 5 minutes). This would
avoid the overhead of opening and closing db's a lot, and would also allow us to
close the Sent and Trash folder db's occasionally (we'd keep the inbox open
because it's used for check for new mail).  The more general approach is a bit
harder to code (and risky until they fix the timer implementation!) but the
easier approach has the disadvantage of potentially causing us to open and close
db's frequently. For example, we'd have to keep the trash db out of this
entirely because otherwise we'd open and close the trash db on every message
delete, and trash folders can get quite large.

Comment 10

[David A. Cobb]

RE: "I'm a little torn about how to fix this. I could just close db's of folders
that get messages moved/copied into them. That would be fairly easy and would keep
most db's closed. . . . ."
1) The trash folder is slightly less important because it won't often matter if
the database file gets stomped.
2) There are cycles of a sort in the "usual" usage.  Specifically the periodic
download from the server will probably open as many DB's as the system will allow.
When you complete the download, close all open DB's.  That makes them safer.
When the user selects a folder in FolderPane, open that DB; it would be
vulnerable but only one or two DB's would be.  It is somewhat less likely that
the user is ploughing through more than one or two folders at a time.

Comment 11

[David :Bienvenu]

Attachment: diffs to close imap folder when destination of move/copy

Comment 12

[David :Bienvenu]

Attachment: close destination local mail folder db after move/copy

this patch is in ClearCopyState, and closes the local mail folder db after move
copy, if it's not the inbox or trash.

Comment 13

[David :Bienvenu]

Cavin, can I get a review? thx.

Comment 14

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

Comment on attachment 73201 [details] [diff] [review]
diffs to close imap folder when destination of move/copy

sr=sspitzer

do we also want to keep the sent folder open?

Comment 15

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

Comment on attachment 73203 [details] [diff] [review]
close destination local mail folder db after move/copy

1)

instead of the CID, please use NS_MSGMAILSESSION_CONTRACTID

2) how about:

if (NS_SUCCEEDED(rv) && session) {
  PRBool folderOpen;
  session->
}

one less nested if.  do we need to initialize folderOpen?

Comment 16

[David :Bienvenu]

my feeling is no for the sent folder. My reasoning was that message composition
and sending is relatively heavy-weight. I don't think that this code will cause
the sent folder db to get closed anyway, for the imap case, since that runs a
different kind of url. I think it might close it for the local mail case.

Comment 17

[David :Bienvenu]

Attachment: new patch for local mail incorporating sr comments

I also fixed the imap code to use the contract id.

Comment 18

[Cavin Song]

Comment on attachment 73201 [details] [diff] [review]
diffs to close imap folder when destination of move/copy

r=cavin.

Comment 19

[Cavin Song]

Comment on attachment 73221 [details] [diff] [review]
new patch for local mail incorporating sr comments

r=cavin.

Comment 20

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

Comment on attachment 73221 [details] [diff] [review]
new patch for local mail incorporating sr comments

sr=sspitzer

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 73221 [details] [diff] [review]
new patch for local mail incorporating sr comments

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

Comment 22

[David :Bienvenu]

this patch fixes the imap case, and fixes the local case of moving messages
between local folders, but it doesn't fix the case of pop filters moving
messages into local folders, because that doesn't go through the move/copy code.
Unfortunately, I suspect the pop case is what this bug was filed for. To fix
that, I'd need to add some code to nsPop3Sink::EndMailDelivery() that closes any
open db's on that mail account (other than inbox and trash). I'll still check
this patch in, but I'll attach a new patch for the pop filter case.

Comment 23

[David A. Cobb]

I can confirm <[e-mail]>'s judgement that the POP+filters case is
critical.  A download session with filters active can access nearly every folder
defined - stressing resource use and leaving the entire mail system vulnerable.
 We shouldn't remain in such a vulnerable position.
IMHO, the logical point at which to do this is when closing the server session.

Comment 24

[Michael Buckland]

Discussed in Mail News bug mtg with Engineering QA Mktng PjM.  Decided to minus
this bug. 

Comment 25

[Asa Dotzler [:asa]]

has the approved patch landed? if so can you obsolete that patch so it doesn't
look like it's waiting to land? thanks.

Comment 26

[David :Bienvenu]

Comment on attachment 73221 [details] [diff] [review]
new patch for local mail incorporating sr comments

obsoleting (because it's checked in)

Comment 27

[David :Bienvenu]

leaving open for the pop3 filter case.

Comment 28

[:aceman]

Are the db files closed on profile switch?

David, could you check if you can help closing some more files as needed for 
bug 205821 ?

Comment 29

[David :Bienvenu]

it doesn't look like the db files are closed on profile switch but I doubt that
has anything to do with bug 205821 because we're not using the db files after
the profile is switched. It's not good to leave them open, but it's not going to
cause any actual harm that I can see besides some memory bloat. Both cases of
.msf files left open should be fixed at some point, though.

Comment 30

[David :Bienvenu]

Attachment: proposed fix

after getting new pop3 mail, close the db's of any folders that had new mail
filtered into them, if the folder is not open in the ui...also cleaned up a
little bit the code that marks summary files as valid after copying local
folders...

Comment 31

[David :Bienvenu]

this breaks new status on messages filtered to local folders, because that is
stored in the db, and we're closing the db now. So, we need to store the new set
in the folder object, and propagate it to the db when we open the db.  Reopening
this bug for that fix.

Comment 32

[David :Bienvenu]

Attachment: fix for newness regression.

now we save a new set in the folder object, and if we open a folder db, and the
folder knows there are new messages, we apply the new set to the db. I need to
run with this for a little bit to make sure it doesn't break anything...

Comment 33

[David :Bienvenu]

Attachment: make view use new flag

the view code needs to go through code that checks the new set in the db in
order for the view code to display the new flag correctly.

Comment 34

[:aceman]

There is a problem, that if you download new mail, don't read it, restart
Mozilla, the new mail indicators (green arrows on folders) are lost. May your
last 3 comments have something to do with this? 

Comment 35

[David :Bienvenu]

re comment 34, that has always been that way since Netscape 4.x. New is defined
as "having a new message arrive in the folder during the current app session".
Once you shut down, no messages are new anymore.

Comment 36

[David A. Cobb]

I'll admit I've never fully adapted to the "Object Oriented" paradigm.  Just
another dinosaur and all that.  However, re: comment#31, et.seq., I find it hard
to understand why the "Newness" indicator isn't simply a property ( attribute? )
of the Folder Object -- and the database file completely inside the containment
of the Object.  

But, of course, I think in different languages.  Sigh!

Comment 37

[:aceman]

Comment 35: This sounds like that is actually the correct behaviour (loose the
flag). Anyway, I wonder if you actually didn't change it now, so that the
indicator will preserve across sessions. You took it out of the db file into
some folder object storage (maybe panacea.dat), so that it is immediatelly
accessible. That's how I understand it. I may be wrong.


Comment 36: looks like this is exactly what David did now :) But even an
attribute of an 'object' must be stored somewhere physically, and there are
problems how to retrieve it from that storage.

Comment 38

[David :Bienvenu]

right, to be clear, having the new flag not persist across sessions is not a
technical issue or limitation of our implementation; it's the way we intended it
to work. I didn't change as much as some of the comments have speculated. We've
always had a new flag for the folders, and we've always stored the new set
non-persistently in the db object. All I've done is add a non-persistent new set
in the folder object that is then copied to the db's non-peristent set. I may
have introduced a regression in the new flag handling, but I don't think it's
one that will span sessions.

Comment 39

[David A. Cobb]

re: Comment#36, Comment#37:

Well, of course it needs be somewhere.  Point is, only the code that realizes
the object needs know where and how.  I don't know much about panacea (except it
isn't one), but anyone outside the folder abstraction that has his fingers on
the flag needs to have his fingers cut off.