Last Comment Bug 495735 - Corrupt prefs.js file not removed if backup file (invalidprefs.js) exists
: Corrupt prefs.js file not removed if backup file (invalidprefs.js) exists
Status: RESOLVED FIXED
[trunk landing 11/3]
: common-issue+, fixed1.9.0.16
Product: Core
Classification: Components
Component: Preferences: Backend (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
:
Mentors:
: 406471 485830 (view as bug list)
Depends on:
Blocks: 361102
  Show dependency treegraph
 
Reported: 2009-05-31 22:41 PDT by Matthew Middleton (:zzxc)
Modified: 2012-12-30 21:00 PST (History)
17 users (show)
mbeltzner: blocking1.9.2+
dveditz: blocking1.9.0.16+
samuel.sidler+old: wanted1.9.0.x+
mats: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta2-fixed
.6+
.6-fixed


Attachments
Patch rev. 1 (3.71 KB, patch)
2009-10-21 07:37 PDT, Mats Palmgren (:mats)
dietrich: review+
Details | Diff | Splinter Review
Patch rev. 2 (5.42 KB, patch)
2009-10-21 15:38 PDT, Mats Palmgren (:mats)
mats: review+
benjamin: review+
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review
for 1.9.0 (8.72 KB, patch)
2009-11-09 16:19 PST, Mats Palmgren (:mats)
benjamin: review+
dveditz: approval1.9.0.16+
Details | Diff | Splinter Review

Description Matthew Middleton (:zzxc) 2009-05-31 22:41:19 PDT
If an "invalidprefs.js" file exists in the profile folder, a corrupt prefs.js file will not be removed and backed up as intended.  (see bug 361102)  The result is that preferences are not saved if prefs.js becomes corrupt more than one time.  This could be fixed either by inserting a number into the backup filename or by deleting the existing file.

A number of people reporting that preferences do not save on support.mozilla.com have confirmed fixing the problem by manually deleting/renaming invalidprefs.js.
Comment 1 [:Cww] 2009-06-03 11:04:18 PDT
Definitely a common issue.
Comment 2 [:Cww] 2009-08-27 11:11:18 PDT
possible cause of https://bugzilla.mozilla.org/show_bug.cgi?id=485830
Comment 3 [:Cww] 2009-09-01 11:20:40 PDT
*** Bug 485830 has been marked as a duplicate of this bug. ***
Comment 4 ken kovash 2009-10-13 15:04:43 PDT
Is anyone working to resolve this issue?

Early evidence from user feedback we're receiving from users hitting the firstrun and whatsnew pages suggests that this may be a significant problem (i.e., far and away the #1 issue cited by users).  We'll have more complete analysis in a couple weeks.
Comment 5 ken kovash 2009-10-20 11:15:00 PDT
Can anyone respond to this?
Comment 6 Dietrich Ayala (:dietrich) 2009-10-20 15:57:39 PDT
Cc'd Mats, who wrote the backup code in the dependent bug.

Requesting blocking 3.6 due to the severity and volume of the effect on users per comment #4.
Comment 7 Mike Beltzner [:beltzner, not reading bugmail] 2009-10-20 18:04:40 PDT
Yeah, and we should probably backport to 1.9.1, not so sure about 1.9.0 since the update will come with a move across to the newer branch.
Comment 8 Mike Beltzner [:beltzner, not reading bugmail] 2009-10-20 18:08:16 PDT
--> Firefox::Preferences
Comment 9 Mats Palmgren (:mats) 2009-10-21 07:33:37 PDT
FWIW, this is the intended behavior, although I have to admit it's quite
useless without notifying the user...
Comment 10 Mats Palmgren (:mats) 2009-10-21 07:37:08 PDT
Created attachment 407530 [details] [diff] [review]
Patch rev. 1

This patch saves the corrupt file as before, but doesn't block writing
"prefs.js" if the save operation fails.  Also, if "Invalidprefs.js"
already exists it is removed first.

Still, the "Invalidprefs.js" file isn't very useful unless we notify
the user of the problem, something like:
  "Some of your preferences has been lost due to a corrupt file.
   See <URL> for further instructions."
I guess that can be handled in a separate bug though.
Comment 11 Dietrich Ayala (:dietrich) 2009-10-21 12:05:47 PDT
Comment on attachment 407530 [details] [diff] [review]
Patch rev. 1


>@@ -323,17 +334,20 @@ nsresult nsPrefService::ReadAndOwnUserPr
>   mCurrentFile = aFile;
> 
>   nsresult rv = NS_OK;
>   PRBool exists = PR_FALSE;
>   mCurrentFile->Exists(&exists);
>   if (exists) {
>     rv = openPrefFile(mCurrentFile);
>     if (NS_FAILED(rv)) {
>-      mDontWriteUserPrefs = NS_FAILED(MakeBackupPrefFile(mCurrentFile));
>+      // Save a backup copy of the current (invalid) prefs file, since all prefs
>+      // from the error line to the end of the file will be lost (bug 361102).
>+      // XXX I wish we had some way of notifying the user about it.
>+      MakeBackupPrefFile(mCurrentFile);

s/XXX/TODO/

please file the followup about notification, and put that bug # in the comment.

also, please request a litmus or windmill test for this, since it'd require an app restart to test properly, from what i can tell.

finally, add yourself to the contributors list in the changed files.

r=me otherwise, thanks for the quick response! however, i'm not a peer or module owner here. hm, actually, according to this, there are no owners: http://www.mozilla.org/about/owners.html#preferences. looks like bsmedberg reviewed taras' recent changes, so ask him for additional review.
Comment 12 Mats Palmgren (:mats) 2009-10-21 15:38:32 PDT
Created attachment 407638 [details] [diff] [review]
Patch rev. 2

With the above comments addressed... (only comment changes).
Filed bug 523725 about the notification.
Comment 13 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-10-30 09:45:52 PDT
Is this ready to land?
Comment 14 Mats Palmgren (:mats) 2009-11-03 11:32:12 PST
http://hg.mozilla.org/mozilla-central/rev/43819c1c05de
Comment 15 Mats Palmgren (:mats) 2009-11-07 05:23:46 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2783586663ae
Comment 16 Daniel Veditz [:dveditz] 2009-11-09 14:47:43 PST
Comment on attachment 407638 [details] [diff] [review]
Patch rev. 2

Approved for 1.9.1.6, a=dveditz for release-drivers
Comment 17 Samuel Sidler (old account; do not CC) 2009-11-09 14:48:01 PST
Do we need a different patch on 1.9.0 here? Mats, can you work one up if so?
Comment 18 Mats Palmgren (:mats) 2009-11-09 16:19:45 PST
Created attachment 411311 [details] [diff] [review]
for 1.9.0

Here's a patch for 1.9.0 should we need it (see comment 7).
I have tested it on Windows with a broken prefs.js.
I haven't tested the MOZ_PROFILESHARING case.
There's a 1.9.0 TryServer build at
https://build.mozilla.org/tryserver-builds/mpalmgren@mozilla.com-495735-1.9.0/
Comment 19 Mats Palmgren (:mats) 2009-11-09 21:38:58 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f17856ac4249
Comment 20 Benjamin Smedberg [:bsmedberg] 2009-11-10 09:35:17 PST
Comment on attachment 411311 [details] [diff] [review]
for 1.9.0

MOZ_PROFILESHARING doesn't matter, nobody builds it.
Comment 21 Daniel Veditz [:dveditz] 2009-11-10 16:40:49 PST
Comment on attachment 411311 [details] [diff] [review]
for 1.9.0

Approved for 1.9.0.16, a=dveditz for release-drivers
Comment 22 Mats Palmgren (:mats) 2009-11-10 19:53:58 PST
Comment on attachment 411311 [details] [diff] [review]
for 1.9.0

Landed on CVS trunk:
mozilla/modules/libpref/src/nsPrefService.cpp 	1.105
mozilla/modules/libpref/src/nsPrefService.h 	1.26
Comment 23 Robert Strong [:rstrong] (use needinfo to contact me) 2009-11-18 10:51:17 PST
*** Bug 406471 has been marked as a duplicate of this bug. ***
Comment 24 Al Billings [:abillings] 2009-11-23 12:08:13 PST
Is there a particular way that prefs.js needs to be corrupt? Is empty or missing most data enough or ??
Comment 25 Mats Palmgren (:mats) 2009-11-24 06:05:37 PST
"Corrupt" here means "an error occurred while parsing it".  I think an empty
prefs.js file is valid.  When testing I edited the file removing a value
somewhere, ie changing user_pref(x,y) to user_pref(x,) which causes this
pref and any that follows to be lost.
Comment 26 Thomas Sisson 2012-12-30 21:00:50 PST
Sorry, this just seemed the most appropriate place to leave this comment even though it is marked as RESOLVED FIXED. I don't mean to open a can of worms.

I agree with Comment 10 from Mats Palmgren about some notification about creating the Invalidprefs.js file. Additionally, creating such a file does not say which are invalid. I personally thought that all preferences in the file were bad. Otherwise, one should call it InvalidPrefsFile.js, or better yet, simply curruptPrefs,js. If it simply a backup of the bad file, it could simply be called prefs.jsInteger. Each successive bad file would be prefs.jsInteger+1. This is simple to do and follows backup conventions to prevent overwrite on both MS Windows and *nix or X systems.

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