Closed Bug 1364146 Opened 7 years ago Closed 7 years ago

Incorrect handling of the dirty flag in the Preferences

Categories

(Core :: Preferences: Backend, enhancement)

Unspecified
All
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: milan, Assigned: milan)

References

Details

Attachments

(1 file)

In bug 160377, we added a flag to track the "dirty" status of the preferences and skip saving the default preference file when the flag is not set.  This is great.

The problem is that we reset the dirty flag even when we do a non-default preference file write.  When this happens, we have the internal preference state out of sync with the preference file, but the dirty flag is not set:

1. modify a preference - dirty flag is set to true
2. call savePrefFile() - default file is updated, dirty flag is set to false
3. modify a preference - dirty flag is set to true
4. call savePrefFile(someRandomFile) - default file is not updated, dirty flag is set to false

This has been an issue since the original version of dirty flag tracking, and we got so used to it that we have xpcshell test that depends on the (arguably) wrong behavior.
Assignee: nobody → milan
Blocks: 987745
Something to consider - today, savePrefFile() looks at the dirty flag if there is no argument (nullptr) and conditionally writes to the default preference file, but always writes if the argument is not-null - even if the argument specifies the default preference file.

In other words, we have a way of "forcing" a pref file write, even if nothing has changed.  I don't think this was necessarily intended, but it's there.

Wonder if this functionality should be removed.  There are places in preferences where we treat the argument being set to the default preference file the same as if it was nullptr.
Attachment #8866896 - Flags: feedback?(dveditz)
Attachment #8866896 - Flags: review?(aklotz) → review?(benjamin)
Comment on attachment 8866896 [details]
Bug 1364146: Only reset the prefs dirty flag when we save to the default prefs file, not on a save to another file.

https://reviewboard.mozilla.org/r/138508/#review143086

r=me with nits fixed

::: modules/libpref/Preferences.cpp:664
(Diff revision 1)
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
>    nsresult rv;
>  
> -  if (nullptr == aFile) {
> +  if (nullptr == aFile || mCurrentFile == aFile) {

Was this change necessary to fix some test? I don't think this check is especially helpful, because nsIFile aren't atoms, so you could have two nsIFiles pointing to the same path where this check would fail anyway. I'd leave it out unless we need it.

::: modules/libpref/test/unit/test_dirtyPrefs.js:27
(Diff revision 1)
>  
> +  // dirty flag only applies to the default pref file save, not all of them,
> +  // so we need to set the default pref file first.  Chances are, the file
> +  // does not exist, but we don't need it to - we just want to set the
> +  // name/location to match
> +  // 

trailing whitespace
Attachment #8866896 - Flags: review?(benjamin) → review+
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd4176474343
Only reset the prefs dirty flag when we save to the default prefs file, not on a save to another file. r=bsmedberg
https://hg.mozilla.org/mozilla-central/rev/cd4176474343
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: