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)

defect

Tracking

()

REOPENED

People

(Reporter: milan, Unassigned)

References

Details

Attachments

(2 files, 1 obsolete file)

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)
Priority: -- → P3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Bug 981818 will not fix this. Although I have a patch in queue that asserts.
Assignee: nobody → benjamin
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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.
Depends on: 1375077
Attachment #8879610 - Attachment is obsolete: true
Attachment #8879610 - Flags: review?(milan)
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)
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?
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 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.
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.
Attachment #8879610 - Flags: review?(milan) → review+
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
Leaving this open for part B (changing warnings to assertions) which I hope to land tomorrow.
Keywords: leave-open
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
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
RyanVM is going to try to land part B.
Assignee: benjamin → ryanvm
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)
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)
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)
Sure thing.
Flags: needinfo?(milan)
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
Assignee: ryanvm → nobody
Flags: needinfo?(milan)
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)
It's not a high priority for me at the moment :/
Flags: needinfo?(n.nethercote)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.