239455 - All pop3 email messages on server downloaded again if local hard disk drive runs out of space, causing duplicates [not enough, insufficient, too little hdd capacity]

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, defect)

Description

[Dave Miller [:justdave]]

Build ID: Mozilla Thunderbird 0.5 (20040208)

This was encountered in Thunderbird 0.5, but since there doesn't seem to be an
appropriate component for it there, I'm assuming this is code that's common to
both Thunderbird and Mozilla MailNews.  If I'm wrong, feel free to move it back.

I ran out of disk space on my iBook while Thunderbird was running.  Thunderbird
politely stopped checking mail since it didn't have anywhere to put the mail
when it downloaded it (or so it appeared anyway).  However, when I cleared up
the disk space issue, Thunderbird started downloading mail again.  The only
problem is it apparently tossed the UIDL information about the POP3 mailbox (I
have it set to leave mail on the server for 7 days), and it proceeded to
redownload the last 7 days worth of mail (3000 or so pieces), giving me
duplicates of a lot of things and resurrecting several dozen messages I had
locally deleted.

I've actually had this happen to me twice today (I've been fighting with the
disk space until I get a chance to move some things to another computer), so
it's reproducible.

(I have over 200 MB free as I type this, but OS X likes to allocate huge chunks
at once for swap, and that's what's killing me).

Comment 1

[Dave Miller [:justdave]]

The Bayes filter training may or may not have been lost at the same time,
fwiw...  Almost all of the duplicated spam that got downloaded during the above
wound up in my inbox again instead of getting flagged as junk, even though they
were all pieces that were specifically used for training previously.

Comment 2

[Christian Eyrich]

Yes, the file in question is popstate.dat in the server folder of your mail
directory.

net_pop3_write_state() creates the whole file from scratch after we're done (or
interrupted).
David, write to a temp and then only delete the old and rename the new one if
we're succeeded writing? Would be save for the old UIDL, but UIDL of in this
session already downloaded messages would be lost too.
But I guess with this file format we can't to more intelligent writing.

BTW, what happens if a statement like
  outFileStream << host->user;
runs into a full disk?

Comment 3

[Dave Miller [:justdave]]

as far as downloading messages goes, since the pop3 server does tell you how big
the messages are first before you start downloading, it would make sense to make
sure there's that much disk space free before beginning to download them. 
Perhaps making sure there is ((room for new messages about to be downloaded) +
(2 * sizeof(popstate.dat))) before begining a download would prevent this in all
but "something else is rapidly filling my drive" situations.  And in those
situations, ensuring that we don't overwrite the existing popstate.dat if
there's no space to do it would be better than nothing.  If we wind up
downloading the messages just from the last download session again, that's way
better than re-downloading everything on the server for someone who has "leave
on server" set to any number of days.

Comment 4

[David :Bienvenu]

we do preflight the pop3 download by the total of the message size and abort if
there's not enough disk space available. But, we don't do it for the uidl, as
Christian points out. Yeah, we could write to a temp file first and then copy it
over the original if successful. Usually, streaming calls won't fail because
everything is buffered - often, it's only the final flush/close that fails
because we're out of disk space. We could check the disk space available before
writing the uidl file...

Comment 5

[Christian Eyrich]

> we do preflight the pop3 download by the total of the message size and abort
> if there's not enough disk space available.

Oh, indeed. I didn't bargain for this.

But why did Dave lose his UIDL's? If the check shows there isn't enough space
left, we leave GetMsg(), go to POP3_ERROR_DONE and then write out the UIDL list.
Because the list hasn't changed in this case, the "new" list should fit in the
available space.

The only explanation would the "something else is rapidly filling the drive"
situation while we wrote out the list.

I still vote for some check or the copy-solution, but as far as I can see, this
bug shouln't have happened with the current code either.

Comment 6

[David :Bienvenu]

I don't think we account for the size of the uidl file in the calculation, and
there is a point at which we might have two copies of it on disk (while we're
rewriting the file ? ) - and as you say, if the disk is rapidly filling up,
we'll have problems. We don't reserve the space on disk for the pop3 download,
iirc - we just check before we start writing.

Comment 7

[Christian Eyrich]

Yes, the space isn't reserverd. But I don't think we've two copies on disk while
rewrite. We open popstate.dat with PR_TRUNCATE and write it from scratch.
Overwriting a 1.5 kB popstate.dat with a 1.5 kB popstate.dat should succeed in
any case (except the very rare and unlikely rapidly filling).

Dave, how was the situation you had? Wasn't there any byte free after Mozilla
gave up?
And did Mozilla even start downloading and then stopped after a few mails? Or
did it recognzie there isn't enough space before downloading the first one?

Comment 8

[Dave Miller [:justdave]]

(In reply to comment #7)
> Dave, how was the situation you had? Wasn't there any byte free after Mozilla
> gave up?
> And did Mozilla even start downloading and then stopped after a few mails? Or
> did it recognzie there isn't enough space before downloading the first one?

I honestly don't know.  I had walked away from the computer at the point when it
was out of space, and discovered everything stopped up when I got back.  At the
point when I got there there were 0 bytes free on the drive according to 'df',
although I only had the "your startup disk is almost full" dialog in the Finder
and not the "your startup disk is full, please choose an application to
force-quit" dialog.

Comment 9

[Tuukka Tolvanen (sp3000)]

*** Bug 281671 has been marked as a duplicate of this bug. ***

Comment 14

[Goudal Francois]

This problem is also true for NNTP.
I subscribed to several hierarchies on 2 newsgroups servers in my thunderbird and at some point, I closed thunderbird while the disk was out of space.
Then, when I started it again, on one of my 2 NNTP accounts, all the groups I had subscribed had disappeared.
The files corresponding to these groups are still there in my profile account but it seems that thunderbird has no idea of them anymore.
If I try to subscribe again to these groups, It seems that thunderbird don't retreive the old informations it had on them, like read articles.

Comment 15

[Magnus Melin [:mkmelin]]

Nominating this for tb3, possibly not a huge amount of work.

Comment 17

[David Ascher (:davida)]

A fine idea, but not a blocker.

Comment 19

[Joel Goguen]

Has this been fixed recently?  I've been trying to reproduce with a recent build, but I haven't been able to.  As expected, Shredder informs me that the disk is out of space and refuses to download anything until I free up some space.  When I do, only new messages are downloaded.

Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b4pre) Gecko/20090412 Shredder/3.0b3pre

Comment 20

[Ludovic Hirlimann [:Usul]]

(In reply to comment #19)
> Has this been fixed recently?  I've been trying to reproduce with a recent
> build, but I haven't been able to.  As expected, Shredder informs me that the
> disk is out of space and refuses to download anything until I free up some
> space.  When I do, only new messages are downloaded.
> 
> Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b4pre) Gecko/20090412
> Shredder/3.0b3pre

Thanks for testing.

Comment 21

[:aceman]

This happens to me with thunderbird 2.0.0.22. I have not tried with Shredder yet.
Would be nice if it was really fixed.

Comment 22

[galois641]

I've experienced this bug three times in the last few weeks, as my laptop is constantly running low on HD space. This bug has been with me as long as I can remember using Thunderbird. It'd be great if it could get fixed. How can I assist?

I'm using version 2.0.0.23 (20090812) for Linux.

P.S.: The Remove Duplicate Messages add-on makes it easier to clean up, but it's still very annoying, especially with messages that I manually sorted into folders.

Comment 24

[Louis Ingenthron]

Attachment: Screenshot of error in progress

Just confirmed this bug on Win7 Home Premium x64 SP1.  See attached screenshot where Thunderbird is downloading visibly duplicate messages after I resolved a full disk error.  This has happened twice and is easily reproducible and really annoying to fix.

Comment 25

[fabian]

Hi guy, I am facing the same problem running Thunderbird 17.0.2 in Ubuntu Linux 12.10, 64Bit. When the system runs out of hard drive memory Thunderbird starts to download all mails again. I am using a POP server.

Comment 26

[Wayne Mery (:wsmwk)]

reopening. It's not clear (to me) whether comment 19 was a sufficient test.

Comment 27

[persona]

I can confirm this bug on Linux running Thunderbird version 17.0.2 (64bit). This only happens with POP-Accounts.

Comment 28

[:aceman]

I think this is not fixed. The checks for free disk space before pop3 download are not sufficient. We may help this in bug 640371 (do not even start pop3 download if disk space is not enough for all the messages to be downloaded) and bug 789679, where we actually check free disk space on some more operations in TB. In all the checks, we only allow the operation if there still remains at least 4MB free disk space after the operation. So 4MB should be enough to rewrite the popstate.dat file.

However we still should check for free space before popstate.dat is rewritten.

Comment 31

[Thomas D. (:thomas8)]

Adding some searchwords for better retrievability.

Comment 32

[:aceman]

(In reply to :aceman from comment #28)
> However we still should check for free space before popstate.dat is
> rewritten.

Any proposals on what to check? Current size of popstate.dat*2 + 4MB of spare? Anything more clever?

What sizes of popstate.dat have you guys seen in the wild?

Comment 33

[Wayne Mery (:wsmwk)]

The largest I find recorded in my emails and bug reports is 1.8MB.  But bienvenu would have great historical knowledge here.

how about popstate.dat*3 + X MB, where X is what we are using elsewhere as "buffer against disaster size"?

Comment 34

[Johannes Buchner]

17.0.5 (64bit), ~50.000 POP emails, this bug is annoying. 
I use this addon to get rid of the havoc: https://addons.mozilla.org/en-us/thunderbird/addon/remove-duplicate-messages-alte/
But junk mail, manually removed before, can remain.

With the DiskSpaceAvailableInStore(file, spaceRequested) function proposed in https://bugzilla.mozilla.org/attachment.cgi?id=735319&action=diff the proposed criterion could be implemented.

I think writing the new content of popstate.dat to a temporary file and then moving it over would be saner, because then you can never enter a corrupt state. This bug may be more closely related to Bug 263142 than Bug 789679 (new dep?), and pehaps can be resolved together.

Comment 35

[Johannes Buchner]

Attachment: Write updated popstate in tempfile first, then copy over

This patch should prevent the bug, because move does not fail when the disk is full.

Comment 36

[Ludovic Hirlimann [:Usul]]

Comment on attachment 756011 [details] [diff] [review]
Write updated popstate in tempfile first, then copy over

You need to request reviews for patches. Setting it for you.

Comment 37

[:aceman]

Cool, thanks for the patch.

The patch you referenced (https://bugzilla.mozilla.org/attachment.cgi?id=735319&action=diff) was already checked in.
Also bug 640371 should help this case as it refuses to download any new messages if there is less than several MB of free disk space. Even if we rewrote popstate.dat in that case (but unchanged) there should be enough space to do it.

But your patch may cover some more cases (e.g. if the disk is filled by other program and some messages are only marked for deletion on server). I just wonder why net_pop3_write_state can't report any failure to higher levels.

Comment 38

[Johannes Buchner]

Attachment: Write updated popstate in tempfile first, then move over

Can't reproduce disk full bug using this patch.
Can reproduce disk full bug without this patch.
Move does not fail when the disk is full.

For testing, the disk must be filled up between connecting to the server and closing the connection, because Thunderbird does not try to fetch new messages anymore if the disk is full.

I just now saw Bug 246675 is the right approach. Will make an update

Comment 39

[:aceman]

So when you reference bug 246675, why do you not use the nsISafeFileOutputStream ? Is it not applicable to the file here?

Comment 40

[Johannes Buchner]

Attachment: Write updated popstate in tempfile first, then move over

As in Bug 246675, using NS_NewSafeLocalFileOutputStream now, which is an existing implementation that writes to a temp file first.

I verified that this patch works using strace and filling the disk. Writing to the temp file fails, and rename is not done, preserving the original file.

I think MsgNewSafeBufferedFileOutputStream should now be used in a number of other places. Please review.

Comment 41

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 756486 [details] [diff] [review]
Write updated popstate in tempfile first, then move over

Review of attachment 756486 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks so much for the patch; sorry it took so long for me to review it.

I'm OK with either landing this patch as is, or expanding it to refactor out the other places in mailnews where we create safe, buffered output streams.

::: mailnews/base/util/nsMsgUtils.cpp
@@ +1438,4 @@
>    return rv;
>  }
>  
> +nsresult MsgNewSafeBufferedFileOutputStream(nsIOutputStream **aResult,

There are a couple of other places in mailnews where we're creating safe output streams; it would be great to update those to use this helper function instead of repeating the code, as part of this patch.

Comment 42

[Ludovic Hirlimann [:Usul]]

Johannes do you want to do what Irving is proposing ?

Comment 43

[Johannes Buchner]

Hi Irving, thanks for the review.
For some reason I did not get/read the notification email from Mozilla.
I, too, noticed that there are other places which would deserve attention and use of a similar function. However, I would prefer this patch to go through first, and that we make another bug to (1) identify those code segments/files, (2) discuss the appropriate solution for each and, then, (3) write a patch, which I can help with.
So I would ask for this patch to go through as is, since you indicated it is ok.

Comment 44

[:aceman]

I have marked the bug so that the patch will be checked in.

Johannes, please file the follow-up bug for the remaining work on the other similar places. Please CC me there. I hope you can work on that bug too.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/92726142b524

Comment 46

[Johannes Buchner]

The follow-up bug is at Bug 912465 (feel free to change the component, etc). Lets continue there.
Thank you Irving, Aceman, and Ryan.

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Attachment: followup_fix_for_bug239455.patch

This bug did not fix at all.

nsISafeOutputStream.finish has to be invoked to overwrite popstate.dat.

Comment 48

[Johannes Buchner]

Thanks Hiro. I overlooked this. Patch looks good to me. We don't need to check for NS_FAILED(rv) again, because either way void is returned.

Comment 49

[Joe Sabash [:JoeS1]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #47)
> Created attachment 800048 [details] [diff] [review]
> followup_fix_for_bug239455.patch
> 
> This bug did not fix at all.
> 
> nsISafeOutputStream.finish has to be invoked to overwrite popstate.dat.

see bug 913502 That seems to occur only if you set the pref leave messages on the server.

Comment 50

[Joe Sabash [:JoeS1]]

I can't speak to the original intent of this bug.
But, with a local build including attachment 800048 [details] [diff] [review] the symptoms of bug 913502 are gone.

Comment 51

[:Irving Reid (No longer working on Firefox)]

This might be worth slipping in to the closed tree, to make sure it shows up in the next successful nightly.

Comment 52

[Wayne Mery (:wsmwk)]

(In reply to :Irving Reid from comment #51)
> This might be worth slipping in to the closed tree, to make sure it shows up
> in the next successful nightly.

I advocate for this. I believe "leave messages on the server" is a commonly used setting.

Comment 53

[Mark Banner (:standard8)]

Now the tree is at least building partially, I've landed this:

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