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)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: milan, Assigned: milan)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Details |
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 | ||
Comment 1•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8866896 -
Flags: feedback?(dveditz)
Updated•7 years ago
|
Attachment #8866896 -
Flags: review?(aklotz) → review?(benjamin)
Comment 3•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cd4176474343
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•