192425 - prefs.js corrupted in case of disk quota exceeded

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Segura M.]

User-Agent:       Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.2.1) Gecko/20021130
Build Identifier: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.2.1) Gecko/20021130

I use mozilla in a school, and very often, when students exceed disk quota,
when they stop mozilla, mozilla want to write the file prefs.js.
As there is no more space left on disk, the file prefs.js is corrupted.

Reproducible: Always

Steps to Reproduce:
1. Already explained before
2.
3.

Actual Results:  
prefs.js is corrupted, and so the user mozilla profile does not work.

Expected Results:  
Could Mozilla test before writing the "prefs.js" file is there is
sufficient free disk space ? Perhaps with a function like "stat" ?
If there is no disk space, it is better to not write any thing in prefs.js.
I have 10 students a week, over 450, which come to me to search help about
this problem. Very anoying...
Perhaps the problem exist also when you try to write a mail and the is not disk
space ?

Comment 1

[Boris Zbarsky [:bzbarsky]]

To prefs backend....

Comment 2

[R.K.Aa.]

Is this a Windows NT bug or related to bug 72892?

Comment 3

[saari (gone)]

->ccarlen, just to live somewhere where someone may look at it 

Comment 4

[Conrad Carlen (not reading bugmail)]

When saving anything, the data should be written to a temp file and then, only
when that's known to have succeeded, exchanged that with the actual file. That
prevents this kind of mishap.

Isn't that what the prefs "safe save" feature is supposed to do?

Comment 5

[Brendan Eich [:brendan]]

How many places in Mozilla fail to use atomic rename of a tmp file that was
successfully written?

Conrad, to whom was your question in comment #4 addressed?  Any answers?

/be

Comment 6

[Juan M. Gonzalez]

Well, Conrad, others and I commented on that point in bug 170539, comment 67,
68, 69, 75... ("prefs.js gets wiped - all mail/news accounts settings lost",
another different prefs.js bug fixed by Conrad recently).

If I'm not wrong, as a summary of what I said there,
nsPrefService::WritePrefFile, using the class nsSafeSaveFile, makes a backup (or
tries to), then overwrites the preferences file and then, if something went
wrong, tries to restore the lost file from backup:

http://lxr.mozilla.org/seamonkey/source/modules/libpref/src/nsPrefService.cpp#349

Differently, for instance the successful fix for the bookmarks bug 86501
("bookmarks truncated when disk full", fixed by Ben Goodger) does not delete the
original file until there is a new correct temporary file ready to substitute it
(by renaming). It's simple, but surely safer (for low memory situations, etc.).

http://lxr.mozilla.org/mozilla/source/xpfe/components/bookmarks/src/nsBookmarksService.cpp#5078
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/components/bookmarks/src/nsBookmarksService.cpp#5078

From CVS Blame:

"Fix for bug 86501 - bookmarks.html file is truncated to 0-length when shutting
down with a full profile disk. Write bookmarks to a temporary file, and rename
that to 'bookmarks.html' only if the write operation succeeded."

Comment 7

[Matthias Versen [:Matti]]

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

Comment 8

[timeless]

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

Comment 9

[Dave Miller [:justdave]]

only trying to add myself to the CC but it won't let me do it unless I removed
the "requested" from blocking 1.4, because blocking 1.4 is disabled....  I'll
file a bug against Bugzilla on that shortly

Comment 10

[Jo Hermans]

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

Comment 11

[Maciej Zenczykowski]

IMHO the bug lies in line 166:

http://lxr.mozilla.org/seamonkey/source/modules/libpref/src/nsSafeSaveFile.cpp#166

165     // and finally, copy the file (preserves file permissions)
166     rv = mBackupFile->CopyToNative(parentDir, fileName);

change to

166     rv = mBackupFile->MoveToNative(parentDir, fileName);

is a partial fix (IMHO sufficient), adding

[166.5  rv = mBackupFile->CopyToNative(originalbackupdir, filenameofbackup);]

[require more IMHO useless logic, since this isn't stricte necessary]

where it should be MoveToNative and afterwards CopyToNative in the opposite
direction - move is guaranteed to succeed (well... almost guaranteed) while
restoring a backup by creating a new copy will likely fail (especially if the
extra space which was freed has just been used up by something else in the
meantime (bg download), or the user was past allowed disk usage in the first
space (ie starting position of usage>limit instead of more normal usage==limit -
happens if the entire disk is filled and only root-reserved space is left).

I didn't spend much time (3 min) - but this seems like the problem to me.
Nb the entire class seems to suck, making safe saves is such a simple problem,
and there is so much code for it... weird.

SafeSave("/path/to/file.ext") {
  UnSafeSave("/path/to/file.ext~") -> failure delete the ~ file and return with
error
* Move("/path/to/file.ext" to "/path/to/file.ext~~") -> failure means permission
error most likely - return with error (file not moved)
  Move("/path/to/file.ext~" to "/path/to/file.ext") -> failure means permission
error most likely - return with error after reversing the previous move
* Remove("/path/to/file.ext~~")
}

Of course on most normal filesystems the two lines marked with '*' aren't
necessary at all (and are indeed more likely to harm than help...)  I'd vote for
omiting them (if a file system screws up on a move(a,b) with neither a nor b
being in the destination after an error condition then the filesystem sux anyways).

Comment 12

[Prognathous]

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

Comment 13

[Darin Fisher]

the patch for bug 132517 (which just got checked in) should help with this bug
as well.

Comment 14

[chris hofmann]

let's mark fixed so this one gets on the testing radar, then reopen or create a
new bug for remaining problems