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

RESOLVED FIXED

Status

()

Core
Preferences: Backend
P2
normal
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: zzxc, Assigned: mats)

Tracking

({common-issue+, fixed1.9.0.16})

Trunk
common-issue+, fixed1.9.0.16
Points:
---
Bug Flags:
blocking1.9.2 +
blocking1.9.0.16 +
wanted1.9.0.x +
in-litmus ?

Firefox Tracking Flags

(status1.9.2 beta2-fixed, blocking1.9.1 .6+, status1.9.1 .6-fixed)

Details

(Whiteboard: [trunk landing 11/3])

Attachments

(3 attachments)

(Reporter)

Description

8 years ago
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.
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Keywords: common-issue?

Comment 1

8 years ago
Definitely a common issue.
Keywords: common-issue? → common-issue+
QA Contact: preferences → preferences-backend

Comment 2

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

Updated

8 years ago
Duplicate of this bug: 485830

Comment 4

8 years ago
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

8 years ago
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+
(Assignee)

Comment 9

8 years ago
FWIW, this is the intended behavior, although I have to admit it's quite
useless without notifying the user...
Assignee: nobody → matspal
(Assignee)

Comment 10

8 years ago
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.
Attachment #407530 - Flags: review?(dietrich)
blocking1.9.1: --- → ?
status1.9.1: --- → wanted
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);

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.
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+
(Assignee)

Comment 12

8 years ago
Created attachment 407638 [details] [diff] [review]
Patch rev. 2

With the above comments addressed... (only comment changes).
Filed bug 523725 about the notification.
Attachment #407638 - Flags: review+
(Assignee)

Updated

8 years ago
Flags: in-litmus?
(Assignee)

Updated

8 years ago
Attachment #407638 - Flags: review?(benjamin)
Attachment #407638 - Flags: review?(benjamin) → review+
Is this ready to land?
Whiteboard: [has patch][can land]
(Assignee)

Comment 14

8 years ago
http://hg.mozilla.org/mozilla-central/rev/43819c1c05de
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][can land]
Whiteboard: [trunk landing 11/3]
(Assignee)

Comment 15

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2783586663ae
status1.9.2: --- → beta2-fixed
(Assignee)

Updated

8 years ago
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 1.9.1.6, a=dveditz for release-drivers
Do we need a different patch on 1.9.0 here? Mats, can you work one up if so?
(Assignee)

Comment 18

8 years ago
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/
Attachment #411311 - Flags: review?(benjamin)
(Assignee)

Comment 19

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f17856ac4249
status1.9.1: wanted → .6-fixed
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 1.9.0.16, 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]
(Assignee)

Comment 22

8 years ago
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
(Assignee)

Updated

8 years ago
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 ??
(Assignee)

Comment 25

8 years ago
"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

5 years ago
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.