765471 - Mail Compact exhausts disk space, and stays full until response to ""unable to compact mail folders" dialog

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Alexander Stohr]

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:13.0) Gecko/20100101 Firefox/13.0
Build ID: 20120601045813

Steps to reproduce:

having firefox run on Windows Vista.
received a bigger bunch of mails.
inbox might contain some 25k of messages, some other folders have even up to 32k.
the mail client window was simply open.


Actual results:

triggerd by an unknown event (timer or network reception activity)
the mail compression started probably on one of the bigger folders.
signs of the mail clients operation were visible on the footer bar.
after some relatively short time the operation stopped with the
message (for me in german, rogh translation:) "unable to compress mail folders"
that made the task bar button blink and a modal dialog box pop up
with an "OK" button inside that halts all operation of this tree of windows.
at this moment the disk space (that previousely had som 1,5 GB) was suddenly
exhausted and it stayed this was until the button was pressed.
then the disc space was nearly imedeately freed.


Expected results:

when the dialog is up the disc space should already be freed before pressing OK.
it damages the system when its hold at the peak disc usage.
the order of freeing and confirmation should be changed.
the state of maximum disc usage should stay as short as possible.

maybe an estimation if disc space is sufficient should be done before compression
and thus any likely to fail scenario should not lead to initiation of the process
or to optional (query for and selection by the user) operation.

If possible copression process shall be changed so that it does not need so much extra space.

i am not sure if compression only failed for a single folder or if it cheased in the middle of compressing a set of folders - thus dropping the action even for folders that would easily be done with success. - please check

Comment 1

[Jo Hermans]

bug 759360, bug 749983, ...

Comment 2

[:aceman]

Each folder should be compacted (compressed) separatelly. The compaction essentially copies all the messages in the folder except the deleted ones into a new file and only if that succeeds (enough disk space), the original file is removed and the new version is used. I don't know how hard it is to determine beforehand how much space is needed for the new file. Could the msf data be used in some way?

I think the first thing what you suggest (free the space immediately upon failure, while the dialog is still up) should be doable. It may be quite common that the user initiates the compaction and walk away from the computer (as it is long and slows it down) so the dialog may be up for a long time. And TB is keeping the disk full which may break other programs (unnecessarily).

I don't think this is a dupe of any of the mentioned bugs.

Comment 3

[Wayne Mery (:wsmwk)]

closest match is bug 398151

Comment 4

[:aceman]

But that is also something else: crash and dataloss inside TB.

In the bug here TB operates fine, the compact fails and it rolls back gracefully, but only after confirming the dialog.

Comment 5

[:aceman]

Johannes, you could be interested in this bug (and be able to reproduce it easily with full disk). And it looks like it could be easy, just rearranging the order of the code.

Comment 6

[Johannes Buchner]

Attachment: bug765471-compact-fills-disk-alert

aceman, Did you mean something along these lines?

Comment 7

[:aceman]

Comment on attachment 8337658 [details] [diff] [review]
bug765471-compact-fills-disk-alert

Yes, that looks like the idea. But I haven't run tested it.

Comment 8

[[e-mail]]

Comment on attachment 8337658 [details] [diff] [review]
bug765471-compact-fills-disk-alert

Wouldn't it be better to remove m_status and just call CleanupTempFilesAfterError in all the right places?

Also, is there a reason you didn't call CleanupTempFilesAfterError in OnDataAvailable?

Comment 9

[Johannes Buchner]

For your first question you are probably looking for someone else to answer. 
For your second: I was under the impression that OnStopRequest is called after OnDataAvailable and handles its failure.

Comment 10

[[e-mail]]

OK, well I agree the code looks reasonable, but I'm not sure how to test it.

Comment 11

[:aceman]

I will test it later today (I have a 4GB mail folder so it is possible to fill the disk to leave a bit less than 4GB free, without causing problems).

Comment 12

[Johannes Buchner]

A method of testing is to create a file filesystem:

$ truncate myfile -s 50M
$ mkreiserfs -f myfile
$ mkdir -p mydir
$ mount -o loop myfile mydir

and then put your TB profile (or a new one) into it.
You can fill this filesystem easily to trigger bugs.

I have this script to manage such sub-filesystems: https://github.com/JohannesBuchner/sparse-reiserfs-manager

Comment 13

[:aceman]

OK, I run tested the patch and it works as expected. But after the dialog is dismissed, the status bar still shows "Compacting folder X...", it is not cleared away. Maybe this is the case even before the patch. Are you able to track that down while you are there? If not, it will not be a problem.

Comment 14

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #13)
> OK, I run tested the patch and it works as expected. But after the dialog is
> dismissed, the status bar still shows "Compacting folder X...", it is not
> cleared away. Maybe this is the case even before the patch. (Very likely) Are you able to
> track that down while you are there? If not, it will not be a problem.

Comment 15

[Johannes Buchner]

Sorry, I have stowed away my Mozilla development folder to free disk space for other projects. I'm not working on it currently, but I can't see why the patch would introduce that issue.

Comment 16

[[e-mail]]

(In reply to Johannes Buchner from comment #9)
> For your first question you are probably looking for someone else to answer.

OK, maybe I can persuade aceman or someone to remove all the references to m_status and call CleanupTempFilesAfterError() in a separate bug.

> For your second: I was under the impression that OnStopRequest is called
> after OnDataAvailable and handles its failure.

The offline store compaction code would appear to have a similar bug; I guess we'll have to fix that separately too now.

Comment 17

[:aceman]

(In reply to [e-mail] from comment #16)
> (In reply to Johannes Buchner from comment #9)
> > For your first question you are probably looking for someone else to answer.
> 
> OK, maybe I can persuade aceman or someone to remove all the references to
> m_status and call CleanupTempFilesAfterError() in a separate bug.

If you file it and add some more context we may look at it :)

> > For your second: I was under the impression that OnStopRequest is called
> > after OnDataAvailable and handles its failure.
> 
> The offline store compaction code would appear to have a similar bug; I
> guess we'll have to fix that separately too now.

So is this patch ready to go in?

Comment 18

[[e-mail]]

(In reply to aceman from comment #17)
> (In reply to comment #16)
> > OK, maybe I can persuade aceman or someone to remove all the references to
> > m_status and call CleanupTempFilesAfterError() in a separate bug.
> 
> If you file it and add some more context we may look at it :)

What context do you need? nsFolderCompactState currently has a member variable m_status which is used to clean up the temp files after an error. However it only does this once the compactor is destroyed, when as this bug shows the temp files should be deleted immediately. This also applies to nsOfflineStoreCompactState but as that inherits m_status it will need to be fixed at the same time. This will then have the effect of porting this bug to cover offline store compaction.

> > > For your second: I was under the impression that OnStopRequest is called
> > > after OnDataAvailable and handles its failure.
> > 
> > The offline store compaction code would appear to have a similar bug; I
> > guess we'll have to fix that separately too now.
> 
> So is this patch ready to go in?

Sure.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/b9bef7723091

Comment 20

[Johannes Buchner]

Yay!