Open
Bug 1372988
Opened 7 years ago
Updated 2 years ago
Preferences modified after profile-before-change are not saved
Categories
(Core :: Preferences: Backend, defect, P3)
Core
Preferences: Backend
Tracking
()
REOPENED
People
(Reporter: milan, Unassigned)
References
Details
Attachments
(2 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
milan
:
review+
|
Details |
1.70 KB,
patch
|
Details | Diff | Splinter Review |
Preference save happens at profile-before-change observer in Preferences::Observe. Before and after bug 789945, some preferences are modified after that stage (toolkit.telemetry.cachedClientID, for example, see bug 1364498 and bug 1364496)
Updated•7 years ago
|
Priority: -- → P3
Reporter | ||
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Comment 2•7 years ago
|
||
Bug 981818 will not fix this. Although I have a patch in queue that asserts.
Assignee: nobody → benjamin
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
Note: this patch is currently before bug 981818 in my patch series but that's not necessary. I'll probably rearrange and land that part first since it's less likely to cause random orange.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8879610 -
Attachment is obsolete: true
Attachment #8879610 -
Flags: review?(milan)
Comment 7•7 years ago
|
||
Comment on attachment 8879610 [details] Bug 1372988 part A - Warn when user prefs are set after the profile is dead, because the prefs won't get saved anyway. This wasn't supposed to be discarded. I don't know how to un-discard it, so Milan you have to mark review in bugzilla instead of reviewboard.
Attachment #8879610 -
Attachment is obsolete: false
Attachment #8879610 -
Flags: review?(milan)
Reporter | ||
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8879610 [details] Bug 1372988 part A - Warn when user prefs are set after the profile is dead, because the prefs won't get saved anyway. https://reviewboard.mozilla.org/r/150948/#review156434 ::: modules/libpref/Preferences.h:489 (Diff revision 2) > MatchKind aMatchKind); > > private: > nsCOMPtr<nsIFile> mCurrentFile; > bool mDirty; > + bool mProfileShutdown = false; I wonder if this will give us a OS X compile error. Either way, perhaps it's better if we initialize in the constructor, like mDirty. ::: modules/libpref/Preferences.cpp:805 (Diff revision 2) > > if (!nsCRT::strcmp(aTopic, "profile-before-change")) { > - rv = SavePrefFile(nullptr); > - } else if (!nsCRT::strcmp(aTopic, "profile-before-change-telemetry")) { > + // Normally prefs aren't written after this point, and so we kick off > + // an asynchronous pref save so that I/O can be done in parallel with > + // other shutdown. > if (AllowOffMainThreadSave()) { Hmm. Why do we only need to save if OMT save is enabled? In the other bug, we only save on dirty when this OMT save is enabled, so if anything, we should do a save when OMT save is disabled. ::: modules/libpref/Preferences.cpp:812 (Diff revision 2) > } > + } else if (!nsCRT::strcmp(aTopic, "profile-before-change-telemetry")) { > + // It's possible that a profile-before-change observer after ours > + // set a pref. A blocking save here re-saves if necessary and also waits > + // for any pending saves to complete. > + SavePrefFileBlocking(); We are deciding that saving those preferences is worth a sync save on shutdown. Do we want to do that when we know there are preferences being modified after profile-before-change? ::: modules/libpref/Preferences.cpp:1170 (Diff revision 2) > return NS_OK; > } > > + // check for profile shutdown after mDirty because the runnables from > + // DirtyCallback can still be pending > + if (mProfileShutdown) { Do we want to do this for non-default file saves as well? If somebody calls SavePrefFile(theirFile), should we let them do it after the mProfileShutdown?
Reporter | ||
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8880126 [details] Bug 1372988 part B - setting prefs after profile shutdown triggers assertions instead of just warnings, https://reviewboard.mozilla.org/r/151518/#review156436
Attachment #8880126 -
Flags: review?(milan) → review+
Comment 10•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8879610 [details] Bug 1372988 part A - Warn when user prefs are set after the profile is dead, because the prefs won't get saved anyway. https://reviewboard.mozilla.org/r/150948/#review156434 > I wonder if this will give us a OS X compile error. Either way, perhaps it's better if we initialize in the constructor, like mDirty. It shouldn't, this is C++11 and I've been using it in other places in the tree. > Hmm. Why do we only need to save if OMT save is enabled? In the other bug, we only save on dirty when this OMT save is enabled, so if anything, we should do a save when OMT save is disabled. We still force a save, but we do it in profile-before-change-telemetry instead of here. > We are deciding that saving those preferences is worth a sync save on shutdown. Do we want to do that when we know there are preferences being modified after profile-before-change? It's pretty unusual for prefs to be written after that. In my try runs it was only happening < 5% of the time, so as a perf optimization I think this will usually win. > Do we want to do this for non-default file saves as well? If somebody calls SavePrefFile(theirFile), should we let them do it after the mProfileShutdown? It's technically ok to do that since it's not guarded by the profile lock. I don't think it matters either way.
Reporter | ||
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8879610 [details] Bug 1372988 part A - Warn when user prefs are set after the profile is dead, because the prefs won't get saved anyway. https://reviewboard.mozilla.org/r/150948/#review156764 ::: modules/libpref/Preferences.h:489 (Diff revision 2) > MatchKind aMatchKind); > > private: > nsCOMPtr<nsIFile> mCurrentFile; > bool mDirty; > + bool mProfileShutdown = false; I meant the order of initialization, but it's easy enough to see what happens. I still don't like the inconsistency (mDirty and mProfileShutdown are next to each other, but one is initialized in the .h, and one on the default constructor in the .cpp), though I don't really care which approach we're using - just that it'd be nice if they were both the same.
Reporter | ||
Updated•7 years ago
|
Attachment #8879610 -
Flags: review?(milan) → review+
Comment 12•7 years ago
|
||
Pushed by bsmedberg@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/93e8c43340e4 part A - Warn when user prefs are set after the profile is dead, because the prefs won't get saved anyway. r=milan
Comment 13•7 years ago
|
||
Leaving this open for part B (changing warnings to assertions) which I hope to land tomorrow.
Keywords: leave-open
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/93e8c43340e4
Comment 15•7 years ago
|
||
Backed out in https://hg.mozilla.org/mozilla-central/rev/cec56ac74abe872bb15731451573c3fed845c3a9 because something in that push made "dom/filesystem/tests/test_worker_basic.html | Something when wrong" permaorange on Windows opt, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=109432905&repo=mozilla-inbound
Comment 16•7 years ago
|
||
Pushed by bsmedberg@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/17153099b556 part A - Warn when user prefs are set after the profile is dead, because the prefs won't get saved anyway. r=milan
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/17153099b556
Comment 19•7 years ago
|
||
Comment on attachment 8880126 [details] Bug 1372988 part B - setting prefs after profile shutdown triggers assertions instead of just warnings, I rebased this and ran it through Try and the setting pref assertion did hit at least once (looks intermittent - 4 green retriggers). https://treeherder.mozilla.org/logviewer.html#?job_id=130160005&repo=try https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cc69756568a28342ad2027689922f53ba0818bb
Flags: needinfo?(milan)
Reporter | ||
Comment 20•7 years ago
|
||
Good to have this data. This wouldn't have gotten worse, meaning that we had these "missed saves" before as well. We could land and tag the intermittents, just to make sure more of them don't show up, but it's probably better to wait until we can clean up the root cause. I'd wait post 57.
Flags: needinfo?(milan)
Comment 21•6 years ago
|
||
If I were to land this after the Gecko 60 bump, would you be able to resource fixing any intermittents that arise from it? I'd like to get this out of my patch queue :)
Flags: needinfo?(milan)
Updated•6 years ago
|
Keywords: leave-open
Comment 23•6 years ago
|
||
I rebased the patch and ran an updated Try push, but the "Setting user pref after profile shutdown" assert is hitting more frequently than I comfortably land this with. https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbca99dbb3d6402f065cc8790cf07c1eae590e10 Attached is the current patch if anybody else wants to pick this up, but I think it's not doing any good sitting in my queue at this point.
Attachment #8880126 -
Attachment is obsolete: true
Updated•6 years ago
|
Assignee: ryanvm → nobody
Updated•6 years ago
|
Flags: needinfo?(milan)
Comment 24•6 years ago
|
||
Nick, is this something you might be interested in looking at? I suppose the first question is whether the results from comment 23 are still representative of the current state of things.
Flags: needinfo?(milaninbugzilla) → needinfo?(n.nethercote)
Comment 25•5 years ago
|
||
It's not a high priority for me at the moment :/
Flags: needinfo?(n.nethercote)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•