Last Comment Bug 361102 - If there were errors reading "prefs.js" then make a backup copy of it before overwriting it
: If there were errors reading "prefs.js" then make a backup copy of it before ...
Product: Core
Classification: Components
Component: Preferences: Backend (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Mats Palmgren (:mats)
: Benjamin Smedberg [:bsmedberg]
Depends on: 495735
  Show dependency treegraph
Reported: 2006-11-17 19:24 PST by Mats Palmgren (:mats)
Modified: 2009-05-31 22:41 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

wip (5.73 KB, patch)
2006-11-18 10:15 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 2 (6.44 KB, patch)
2006-11-18 14:55 PST, Mats Palmgren (:mats)
darin.moz: review+
darin.moz: superreview+
Details | Diff | Splinter Review

Description Mats Palmgren (:mats) 2006-11-17 19:24:15 PST
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 (:mats) 2006-11-18 10:15:50 PST
Created attachment 245900 [details] [diff] [review]

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
b. Only make a backup copy if a read error occur
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 (:mats) 2006-11-18 14:55:23 PST
Created attachment 245907 [details] [diff] [review]
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 2006-11-20 08:31:35 PST
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 (:mats) 2006-11-20 10:31:07 PST
(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 (:mats) 2006-11-28 07:13:28 PST
Checked in to trunk at 2006-11-28 06:11 PST


Note You need to log in before you can comment on or make changes to this bug.