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

Status: RESOLVED FIXED

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

Description

[Matthew Middleton (:zzxc)]

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]]

Definitely a common issue.

Comment 2

[[:Cww]]

possible cause of https://bugzilla.mozilla.org/show_bug.cgi?id=485830

Comment 4

[ken kovash]

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]

Can anyone respond to this?

Comment 6

[Dietrich Ayala (:dietrich)]

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]]

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]]

--> Firefox::Preferences

Comment 9

[Mats Palmgren (inactive)]

FWIW, this is the intended behavior, although I have to admit it's quite
useless without notifying the user...

Comment 10

[Mats Palmgren (inactive)]

Attachment: 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)]

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 (inactive)]

Attachment: 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)]

Is this ready to land?

Comment 14

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/43819c1c05de

Comment 15

[Mats Palmgren (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2783586663ae

Comment 16

[Daniel Veditz [:dveditz]]

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)]

Do we need a different patch on 1.9.0 here? Mats, can you work one up if so?

Comment 18

[Mats Palmgren (inactive)]

Attachment: 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/[e-mail]-495735-1.9.0/

Comment 19

[Mats Palmgren (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f17856ac4249

Comment 20

[Benjamin Smedberg]

Comment on attachment 411311 [details] [diff] [review]
for 1.9.0

MOZ_PROFILESHARING doesn't matter, nobody builds it.

Comment 21

[Daniel Veditz [:dveditz]]

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 (inactive)]

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 24

[Al Billings [:abillings - ex-MoCo]]

Is there a particular way that prefs.js needs to be corrupt? Is empty or missing most data enough or ??

Comment 25

[Mats Palmgren (inactive)]

"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]

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.