86501 - bookmarks truncated when disk full

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect, P2)

Description

[greggyb]

I ran out of disk space on my laptop today, and made the mistake of exiting
mozilla when the disk was 100% full.  The following files in my .mozilla
directory were truncated (nothing left in them at all):
bookmarks.html
cookieperm.txt
localstore.rdf
nswrapper.copy_defs
prefs.js

Some of these may have had zero length before, but there were definitely
bookmarks and preferences before.  There were no errors or warnings, so it took
me a little while to figure out what happened and why.

I'm running on a Debian PPC (woody/testing) machine, with a build of Mozilla
0.9.1 that I built myself.

Comment 1

[Asa Dotzler [:asa]]

-> XPApps

Comment 2

[Nicolás Lichtmaier]

I've just lost all my preferences, including mail settings, saved passwords,
bookmarks, etc. Please, asign this bug the proper priority: this bug causes user
data loss. Sorry for the spam, but I wanted to remark the importance of this.
And fixing this is just doing somwthing like the following when saving important
files:

f=fopen("file.tmp","w")
rc=fwrite(p,1,len,f);
if(rc==len && !fclose(f))
        rename("file.tmp","file");

(at least on UNIX =) )

Comment 3

[Paul Chen]

not sure what to do with this bug, but I'm sure my manager can figure something
out ;-) ->trudelle

Comment 4

[Peter Trudelle]

Well, I think we should fix it!  I guess in this case, we should detect the
error, try to recover space from disk cache etc., and in the last resort exit
without saving the new bookmarks/prefs (but also not deleting the previous files).
Does this only happen on Linux? Sara, could you try to repro on Win32?
 ->ben, critical severity, P2/096, cc sgehani.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

I can easily use the approach described here to prevent dataloss by writing to a
temp file and then copy, but anything more elaborate sounds like a feature ;) 

Comment 6

[Samir Gehani]

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

Comment 7

[Ryan Pertusio]

well, I sadly discovered that this bug also occurs on win32  :(

Luckily, I only lost a week's worth of bookmarks (due to backups of the
bookmarks file).  :D

A temp file sounds like a good idea to me.  I also really like Peter's idea of
freeing disk cache, or at least keep the pre-session files that are affected.

Changing OS/Platfom to "All"  (please change if you don't agree)

Comment 8

[sairuh (rarely reading bugmail)]

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

Comment 9

[Bernd Eckenfels]

this bug bites me, too. It also happens for downloaded fiels (you see empty
files). The temp file aproach is mandatory. You can't ensure that files arent
corrupted if you rewrite them in place.

Not only disk full errors, but also program shutdown/power outage could bite a
mozilla user (of course more unlikeley).

Comment 10

[Samir Gehani]

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

Comment 11

[Nicolás Lichtmaier]

Uhm.. I think this is being handled in bug 98476. Am I wrong?

Comment 12

[greggyb]

The patches for bug 98476 seem to address only the prefs file (I've just looked
through them, not applied them).  The same ideas could be applied to bookmarks,
cookies, etc, but until they are, I think this bug should stay open.

Comment 13

[Peter Trudelle]

nsbeta1+ per Nav triage team.  Bookmarks are the main ones to worry about.

Comment 14

[Pierre Chanial]

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

Comment 15

[selmer (gone)]

Please update this bug with an [adt1] - [adt3] impact rating (or take it off the
list if it doesn't even rate adt3.)  Thanks!

Comment 16

[Vadim Berezniker]

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

Comment 17

[Peter Trudelle]

ADT1

Comment 18

[Jaime Rodriguez, Jr.]

Changing to [ADT2] per ADT triage.

Comment 19

[Gregory P. Smith]

I just got bitten by this bug as well; running linux with 0.9.9.  bookmarks,
cookies, preferences (and with them any notion of what themes i had already
downloaded) are all toast.  thanks a lot. :-(

anyways, easy way to avoid this in any application merely saving internal state:

1) NEVER open an existing file for writing when you just want to replace it by
writing all new data.
2) Open a temporary file in the same subdirectory as the existing file for
writing.  Write all necessary data there.  If there's an error (disk full, etc)
report it as appropriate. a simple [could not save FOO, continue / abort] type
dialog would suffice.
3) after successfully writing to the temporary file, rename (move) the temporary
file into place over the original.  on windows you may need to delete the
original before the rename depending on its silly file semantics.

[hmm, looks like comments #2 and #5 already cover this; too bad nobody found
time to do it]

Comment 20

[Andrew Hagen]

When this bug and some related bugs are fixed, the Mozilla Project will have
grown up. 

If you have programming skills, please contribute some time. 

Comment 21

[Andrew Hagen]

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

Comment 22

[Andrew Hagen]

Proposed relnote: If the drive where the Mozilla profiles are installed runs out
of free space while Mozilla is running, when Mozilla exits all preferences will
be lost.

Comment 23

[Peter Trudelle]

Ben, do you need some help fixing this?

Comment 24

[Peter Trudelle]

BEN??

Comment 25

[Ben Goodger (use ben at mozilla dot org for email)]

I've created bugs against other components for work required there. 

bug 145558 - localstore.rdf truncated (Component: RDF)
bug 145557 - cookieperm.txt truncated (Component: Cookies)
bug 145556 - prefs.js truncated  (Component: Preferences: BackEnd)

I'm resummarizing this one to make it about Bookmarks only, which is the
component that I own. Patch shortly. 

Comment 26

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

Create a unique temporary file based on the bookmarks file name, and write
bookmarks to that file. Ensure that the stream operation &
WriteBookmarksContainer functions succeeded before renaming the temporary file
to the old bookmarks file name. If either operation fails, we don't rename the
temporary, just remove it.

Comment 27

[Brian Ryner (not reading)]

Comment on attachment 84384 [details] [diff] [review]
patch

r=bryner

Comment 28

[jag (Peter Annema)]

Comment on attachment 84384 [details] [diff] [review]
patch

sr=jag

Comment 29

[Ben Goodger (use ben at mozilla dot org for email)]

Fixed on the trunk. 

Comment 30

[Stephen P. Morse]

Curious as to how you tested this out.  Did you really create to a disk-full 
situation?  Or did you simulate it by altering the return value from 
WriteBookmarksContainer?

Comment 31

[R.K.Aa.]

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

Comment 32

[Ben Goodger (use ben at mozilla dot org for email)]

I altered the return value. I don't have any disks that are anywhere near full
enough to test that on. If someone can show that this patch doesn't actually fix
the bug though (as the stream should return error in such cases), I'll be glad
to give it another try ;-) 

Comment 33

[Ben Goodger (use ben at mozilla dot org for email)]

Let me just verify this using an almost-full floppy disk. 

Comment 34

[Ben Goodger (use ben at mozilla dot org for email)]

Which, since my floppy disk is modular and is not in my laptop at present, will
have to be tomorrow. 

Comment 35

[Ryan Pertusio]

This bug is not fixed :\

I tested it with a real scenario with the latest build (2002052508) for Win32. I
am running Windows XP, 20Gb hard drive (NTFS compressed, if that makes a difference)

-- TEST 1 --
Starting with 64k free space, then exit:
Bookmarks truncated to 0 bytes

-- TEST 2 --
Starting Mozilla with 80MB, then reducing disk space to 28k while Mozilla
running, then exit:
Bookmarks truncated to 0 bytes
-- TEST 3 --
Starting MOzilla with 80MB, then reducing to approx 200k while mozilla running,
then exit:

All bookmark locations are saved, yet individual bookmark names are truncated if
there is not enough disk space. (Names are successfully saved up to available
disk space, but the rest of the names are truncated)

Example:

BEFORE:
    Name: Getting Involved with mozilla.org
Location: http://www.mozilla.org/start/

AFTER:
    Name:
Location: http://www.mozilla.org/start/

To Note: The IE Imported Favorites did not adhere to this 'rule'. All of those
bookmarks kept their names. It is only the 'true' Mozilla bookmarks that were
truncated.

---------
--> Reopening bug, as per my findings. (Please change if you disagree)

I will keep my hard drive in current condition (full) for about a week, if
anyone needs me to do further testing, or clarify my situation.

Comment 36

[Peter Trudelle]

How could this get reviewed and checked in without being tested?

Comment 37

[Ben Goodger (use ben at mozilla dot org for email)]

This was tested under the assumption that a failed disk write would cause
nsErrorProne (a base class to nsOutputFileStream) to store an error code. This
turned out to be an incorrect assumption, however the patch did make the
bookmarks code somewhat more robust for un-openable files (read only media etc). 

It turns out there is NO error reporting for write operations in nsOutputStream,
which seems like a flaw. I have a patch that fixes the bug (verified by
following the following steps:
- take a floppy disk that is almost full
- copy a bookmarks.html file that fits in the available space
- set the browser.bookmarks.file pref to point to that file
- run the browser, add some bookmarks)

Comment 38

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: enhanced patch that additionally fixes the bug reported ;-)

I want someone to check over the modifications to XPCOM in this latter patch.
The inheritance hierarchy of the (file) stream classes seems complex leading to
the belief that some functionality (e.g. |nsErrorProne|) is available to all,
when in fact it is not. The base output stream |nsOutputStream| has a write
method which returns absolutely no status as to whether or not the write
succeeded or failed - IMO a bad bug. This patch changes that, but also adds a
public method to that class to allow clients to determine if something bad
happend. If this seems reasonable, say so, otherwise, any other ideas? ;-)

Comment 39

[Brendan Eich [:brendan]]

Comment on attachment 85420 [details] [diff] [review]
enhanced patch that additionally fixes the bug reported ;-) 

Shouldn't we return -1 on error from nsOutputStream::write, not 0 -- then the
nsresult error code (not a boolean telling success or failure) should be
recoverable using a getStatus method (why are C++ methods in nsFileStream.*
interCaps instead of InterCaps -- darin, you know the history?	Lots of tedious
//----... comments in the .cpp file too, grumble).

/be

Comment 40

[Darin Fisher]

SIGH!!  we really need to phase out xpcom/io/nsIFileStream.h (in favor of
netwerk/base/public/nsIFileStreams.idl, of course!)... what a mess :-(

that said, i totally agree with brendan, but sadly it looks like
nsBookmarksService.cpp wants to use operator<< which doesn't return a status code.

so, it looks like ben's patch is probably right on.  though, i'd probably still
make write() return -1 on failure.

die nsFileStream die!

Comment 41

[Brendan Eich [:brendan]]

Darin, why not return the saved nsresult, instead of NS_SUCCEEDED(thatResult)?

/be

Comment 42

[Ben Goodger (use ben at mozilla dot org for email)]

(with the |succeeded| method, I was just copying what nsErrorProne did
(nsErrorProne::failed), but offering an inverted function name/purpose so as to
avoid ambiguity in subclasses ;-) I can easily make this fn return just the
error code, but I'll have to name it something other than |error|

Yes, all this stuff sucks. One day, Bookmarks will be a good little citizen and
be rewritten to use all the proper stuff (nsIFileStreams stuff, nsIFile, etc). 

Comment 43

[Ben Goodger (use ben at mozilla dot org for email)]

Er, if |succeeded| is used, the return value of that function should be PRBool,
not nsresult. -_-;;;

Comment 44

[Darin Fisher]

i'm not very picky about what you do with nsFileStream, but perhaps for
completeness it'd be good to offer both a succeeded() method as well as a
lastError() method that returns nsresult.  that way consumers of this interface
could choose to call either.

Comment 45

[epu]

Why do you save ANY file at exit?

If you modify mozilla prefs then save the prefs when the user clicks OK.
If you add, remove, edit, ... a bookmark then save the bookmarks when
the operation completes.
...

There shouldn't be any file written at exit.

If you say me that the bookmarks and prefs are only saved at exit because
saving such a large file it's slow (this should use it's own thread) then
move to a db file.

I don't know how this is being done now, but having these critical problems
I'm sure it's not being done very good.

Comment 46

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: revised patch

This patch replaces the |succeeded| method with |lastError| as darin suggests.
I decided to ditch succeeded rather than augment it, as it's somewhat
redundant, and could serve as confusion with |failed| methods implemented
elsewhere in this file, and minimize my intrusions. 

brendan/darin, care to review?

Comment 47

[Stephane Chauveau]

I aggree with comment 45 especially for the bookmarks.
Saving bookmark at exit also has a big disadvantage: New 
bookmarks are lost when mozilla crashes.

Comment 48

[Darin Fisher]

Comment on attachment 86041 [details] [diff] [review]
revised patch

r=darin

Comment 49

[Brendan Eich [:brendan]]

Comment on attachment 86041 [details] [diff] [review]
revised patch

sr=[e-mail], no big deal what you do in that crufty old code, but I
woulda called it mWriteStatus or some such, and lastWriteStatus().

/be

Comment 50

[Jesse Ruderman]

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

Comment 51

[Samir Gehani]

Ben says he'll land this on the trunk tomorrow afternoon (06/21).

Comment 52

[timeless]

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

Comment 53

[timeless]

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

Comment 54

[R.K.Aa.]

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

Comment 55

[Ben Goodger (use ben at mozilla dot org for email)]

Checked in. 

Comment 56

[Paul Wyskoczka]

Claudius, plese verify this fix on the trunk.  thx.

Comment 57

[Claudius Gayle]

So technically, I filled my disk, ran disk cleanup a bunch of times and filled
it some more and then ran mozilla, grew my bookmarks and shut down. Upon a
restart all of the new BM's wee intact. Apparently i was able to save 200k more
of BM's when I had less than that available to start with. How is that possible
you might ask?

Well, thanks to the combined magic of mozilla and Windows98 I gained 20MB simply
by launching and quitting a trunk build! This happened twice, 20MB+ each time.

Since then I've had problems cleanly quitting the process and starting up
mozilla. My system is currently hosed and I'm trying to make it stable again.

To conclude, I can't mark this bug as verified yet -still more testing needed.
If any of the original reporters want to try and contribute real world proof
that this bug is no longer a problem please do.

Comment 58

[Claudius Gayle]

Okay, okay. With a 20020627 trunk build I can verify that when the BM file to
write was larger than the space on disk available we threw away the changes and
left the previous version of the file intact.

VERIFIED Fixed.

I recommend prioritizing hte other related bugs as well. I think we're throwing
away a lot of data in low memory situations.

Comment 59

[Paul Wyskoczka]

Adding adt1.0.1-.  Let's try to get it into the next release.

Comment 60

[Dawn Endico]

Removing the item for this bug from the release notes for 1.1beta and beyond.

Comment 61

[Bill Mason]

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

Comment 62

[Pierre Chanial]

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

Comment 63

[timeless]

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

Comment 64

[Jason Bassford]

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

Comment 65

[Chris Lyon]

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

Comment 66

[Chris Lyon]

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

Comment 67

[Bill Mason]

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