361102 - If there were errors reading "prefs.js" then make a backup copy of it before overwriting it

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Mats Palmgren (inactive)]

Bug 340244 removed a couple of flags that were intended to implement this
but they were never set to true because of the way NS_UNLIKELY was defined,
so it was dead code. The problem with the old code was that it flagged a
non-existent "prefs.js" file as a read error too and thus the command:
HOME=/tmp/emptydir; firefox -CreateProfile default
would not create "prefs.js", causing Tinderbox testing stage to fail.

Comment 1

[Mats Palmgren (inactive)]

Attachment: wip

This restores the two flags and fixes the remaining issues:
1. it's ok to write if the file does not exist
2. propagate parse errors from openPrefFile()

As an example: "prefs.js" has 10 prefs, line 5 cannot be parsed at startup:

Current behavior:
1. pref 1-4 will be used, pref 5-10 will not be used
2. The file will be written => prefs 5-10 are lost, but any new
   or changed prefs are saved. (The file is now "repaired".)

New behavior (the original intention of these flags AFAICT):
1. as above
2. The file will not be written => new or changed prefs are lost.
   (a corrupt prefs.js it will appear to the user as if preferences
    are not persistent)

Both have there drawbacks but I think I prefer the new behavior since it's
slightly less destructive. I don't have a strong opinion on it though.

I have tested this on Windows/Linux/MacOSX.

================

A few other solutions:
a. always make a backup copy of "prefs.js" on startup (as we do for
   bookmarks?)
-or-
b. Only make a backup copy if a read error occur
-or-
c. If an error occurs, backtrack to the beginning of the line and save
   everything from that point to the end of the file in an internal
   buffer. Everytime the file is written, add the junk buffer to the
   end of the file.

I think b. wouldn't be to hard to implement and it combines the best of the
old and new behavior described above so I'll take a shot at it...

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

This patch makes a backup copy if reading the file results in an error,
then proceeds as normal - whatever prefs could be read + new and
changed prefs will be written to the file, making it valid again.
If making the backup copy fails (should hopefully be a rare condition)
then we prevent the original prefs file from being overwritten until
a backup copy can be made (new and changed prefs will be lost until
the backup is made).
The backup is made in the same directory as the original, with "Invalid"
prepended to the file name. E.g. "prefs.js" => "Invalidprefs.js"

Tested on Windows/Linux/MacOSX (not the MOZ_PROFILESHARING bits though).

Comment 3

[Darin Fisher]

Comment on attachment 245907 [details] [diff] [review]
Patch rev. 2

if PREF_ParseBuf fails, then perhaps you should exit from the loop immediately.

"invalid" as a filename prefix sounds funny.  maybe "backup" or a ".bak" extension instead?

Comment 4

[Mats Palmgren (inactive)]

(In reply to comment #3)
> (From update of attachment 245907 [details] [diff] [review] [edit])
> if PREF_ParseBuf fails, then perhaps you should exit from the loop
> immediately.

I wanted to keep the loop as before to minimize the risk for regressions.

> "invalid" as a filename prefix sounds funny.  maybe "backup" or a ".bak"
> extension instead?

The "Invalid" came from "Invalid.mfasl" which I think is a broken XUL
fastload file. Your suggestions sounds to much like it's just a backup of
a valid file. I would prefer a name that indicates it's broken and also
keep the extension since it says something about the type of the file.
Does "Brokenprefs.js" sound better?
(I wish we could use "Invalid_..." or "Broken_..." but I'm not sure all
systems we run on can handle "_").

Comment 5

[Mats Palmgren (inactive)]

Checked in to trunk at 2006-11-28 06:11 PST

-> FIXED