Closed Bug 86501 Opened 23 years ago Closed 22 years ago

bookmarks truncated when disk full

Categories

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

Product:
SeaMonkey
Component:
Bookmarks & History
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: greggyb, Assigned: bugs)

References

Details

(Keywords: dataloss, qawanted, relnote, Whiteboard: [adt2 RTM][ETA trunk: 06/21])

Attachments

(2 files, 1 obsolete file)

patch
2.51 KB, patch
bryner
: review+
jag+mozilla
: superreview+
Details | Diff | Splinter Review
revised patch
3.31 KB, patch
darin.moz
: review+
brendan
: superreview+
Details | Diff | Splinter Review 
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.
-> XPApps
Assignee: asa → pchen
Component: Browser-General → XP Apps
QA Contact: doronr → sairuh
Status: UNCONFIRMED → NEW
Ever confirmed: true
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 =) )


not sure what to do with this bug, but I'm sure my manager can figure something
out ;-) ->trudelle
Assignee: pchen → 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.
Assignee: trudelle → ben
Severity: normal → critical
Priority: -- → P2
Target Milestone: --- → mozilla0.9.6
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 ;)
Status: NEW → ASSIGNED
*** Bug 106220 has been marked as a duplicate of this bug. ***
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)
OS: Linux → All
Hardware: Other → All
*** Bug 107409 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Target Milestone: mozilla0.9.9 → mozilla1.1
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).
*** Bug 125456 has been marked as a duplicate of this bug. ***
Uhm.. I think this is being handled in bug 98476. Am I wrong?
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.
Blocks: profile-corrupt
Keywords: dataloss, nsbeta1
nsbeta1+ per Nav triage team.  Bookmarks are the main ones to worry about.
Keywords: nsbeta1 → nsbeta1+
Target Milestone: mozilla1.1alpha → mozilla1.0
*** Bug 113907 has been marked as a duplicate of this bug. ***
Please update this bug with an [adt1] - [adt3] impact rating (or take it off the
list if it doesn't even rate adt3.)  Thanks!
*** Bug 136357 has been marked as a duplicate of this bug. ***
ADT1
Whiteboard: [adt1]
Changing to [ADT2] per ADT triage.
Whiteboard: [adt1] → [adt2]
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]
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. 
*** Bug 94010 has been marked as a duplicate of this bug. ***
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.
Depends on: 98476
Keywords: relnote
Ben, do you need some help fixing this?
BEN??
Whiteboard: [adt2] → [adt2 RTM]
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.
Summary: preferences and bookmarks truncated when disk full → bookmarks truncated when disk full
Attached patch patch — — Splinter Review 
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 on attachment 84384 [details] [diff] [review]
patch

r=bryner
Attachment #84384 - Flags: review+
Comment on attachment 84384 [details] [diff] [review]
patch

sr=jag
Attachment #84384 - Flags: superreview+
Fixed on the trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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?

*** Bug 147228 has been marked as a duplicate of this bug. ***
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 ;-) 
Let me just verify this using an almost-full floppy disk. 
Which, since my floppy disk is modular and is not in my laptop at present, will
have to be tomorrow. 
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
How could this get reviewed and checked in without being tested?
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)
Status: REOPENED → ASSIGNED
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 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
Attachment #85420 - Flags: needs-work+
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!
Darin, why not return the saved nsresult, instead of NS_SUCCEEDED(thatResult)?

/be
(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). 
Er, if |succeeded| is used, the return value of that function should be PRBool,
not nsresult. -_-;;;
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.

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.
Attached patch revised patch — — Splinter Review 
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?
Attachment #85420 - Attachment is obsolete: true
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 on attachment 86041 [details] [diff] [review]
revised patch

r=darin
Attachment #86041 - Flags: review+
Comment on attachment 86041 [details] [diff] [review]
revised patch

sr=brendan@mozilla.org, no big deal what you do in that crufty old code, but I
woulda called it mWriteStatus or some such, and lastWriteStatus().

/be
Attachment #86041 - Flags: superreview+
Component: XP Apps → Bookmarks
QA Contact: sairuh → claudius
*** Bug 151955 has been marked as a duplicate of this bug. ***
Ben says he'll land this on the trunk tomorrow afternoon (06/21).
Whiteboard: [adt2 RTM] → [adt2 RTM][ETA trunk: 06/21]
*** Bug 153343 has been marked as a duplicate of this bug. ***
*** Bug 153485 has been marked as a duplicate of this bug. ***
*** Bug 153890 has been marked as a duplicate of this bug. ***
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Keywords: adt1.0.1
Claudius, plese verify this fix on the trunk.  thx.
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.
Keywords: qawanted
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.
Status: RESOLVED → VERIFIED
Adding adt1.0.1-.  Let's try to get it into the next release.
Keywords: adt1.0.1 → adt1.0.1-
Removing the item for this bug from the release notes for 1.1beta and beyond.
*** Bug 158730 has been marked as a duplicate of this bug. ***
*** Bug 158838 has been marked as a duplicate of this bug. ***
*** Bug 163997 has been marked as a duplicate of this bug. ***
*** Bug 168774 has been marked as a duplicate of this bug. ***
*** Bug 196860 has been marked as a duplicate of this bug. ***
*** Bug 199523 has been marked as a duplicate of this bug. ***
*** Bug 207374 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.