Closed Bug 495735 Opened 13 years ago Closed 12 years ago

Corrupt prefs.js file not removed if backup file (invalidprefs.js) exists


(Core :: Preferences: Backend, defect, P2)




Tracking Status
status1.9.2 --- beta2-fixed
blocking1.9.1 --- .6+
status1.9.1 --- .6-fixed


(Reporter: zzxc, Assigned: mats)



(Keywords: common-issue+, fixed1.9.0.16, Whiteboard: [trunk landing 11/3])


(3 files)

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 have confirmed fixing the problem by manually deleting/renaming invalidprefs.js.
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Definitely a common issue.
QA Contact: preferences → preferences-backend
Duplicate of this bug: 485830
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.
Can anyone respond to this?
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.
Flags: blocking1.9.2?
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.
Flags: blocking1.9.2?
Flags: blocking1.9.2+
Flags: blocking1.9.0.16?
Priority: -- → P2
Component: Preferences: Backend → Preferences
Flags: wanted1.9.1?
Flags: blocking1.9.2+
Product: Core → Firefox
QA Contact: preferences-backend → preferences
Target Milestone: --- → Firefox 3.6
--> Firefox::Preferences
Flags: blocking-firefox3.6+
FWIW, this is the intended behavior, although I have to admit it's quite
useless without notifying the user...
Assignee: nobody → matspal
Attached patch Patch rev. 1Splinter Review
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.
Attachment #407530 - Flags: review?(dietrich)
blocking1.9.1: --- → ?
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Attachment #407530 - Flags: review?(dietrich) → review+
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);


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: looks like bsmedberg reviewed taras' recent changes, so ask him for additional review.
blocking1.9.1: ? → .5+
Flags: blocking1.9.0.16? → blocking1.9.0.16+
Component: Preferences → Preferences: Backend
Flags: blocking-firefox3.6+
Product: Firefox → Core
QA Contact: preferences → preferences-backend
Target Milestone: Firefox 3.6 → ---
Flags: blocking1.9.2+
Attached patch Patch rev. 2Splinter Review
With the above comments addressed... (only comment changes).
Filed bug 523725 about the notification.
Attachment #407638 - Flags: review+
Flags: in-litmus?
Attachment #407638 - Flags: review?(benjamin)
Attachment #407638 - Flags: review?(benjamin) → review+
Whiteboard: [has patch][can land]
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][can land]
Whiteboard: [trunk landing 11/3]
Attachment #407638 - Flags: approval1.9.1.6?
Attachment #407638 - Flags: approval1.9.1.6? → approval1.9.1.6+
Comment on attachment 407638 [details] [diff] [review]
Patch rev. 2

Approved for, a=dveditz for release-drivers
Do we need a different patch on 1.9.0 here? Mats, can you work one up if so?
Attached patch for 1.9.0Splinter Review
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
Attachment #411311 - Flags: review?(benjamin)
Comment on attachment 411311 [details] [diff] [review]
for 1.9.0

MOZ_PROFILESHARING doesn't matter, nobody builds it.
Attachment #411311 - Flags: review?(benjamin) → review+
Comment on attachment 411311 [details] [diff] [review]
for 1.9.0

Approved for, a=dveditz for release-drivers
Attachment #411311 - Flags: approval1.9.0.16+
Whiteboard: [trunk landing 11/3] → [trunk landing 11/3][needs 1.9.0 landing]
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
Keywords: fixed1.9.0.16
Whiteboard: [trunk landing 11/3][needs 1.9.0 landing] → [trunk landing 11/3]
Duplicate of this bug: 406471
Is there a particular way that prefs.js needs to be corrupt? Is empty or missing most data enough or ??
"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.
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.
You need to log in before you can comment on or make changes to this bug.