Closed Bug 981818 Opened 11 years ago Closed 7 years ago

Preferences not saved upon force quit

Categories

(Core :: Preferences: Backend, defect, P2)

Product:
Core
Component:
Preferences: Backend
Version:
27 Branch
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: sibi.antony, Assigned: benjamin)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 981818 - Save preferences every time they are dirty on an timer for batching (only enabled when async pref writes are enabled)
59 bytes, text/x-review-board-request
milan
: review+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140212131424

Steps to reproduce:

Working on a simple extension, found this strange behavior with preferences saving. I often use a Ctrl+C to quit ff between tests. Whenever I do this, the preferences which I  set using nsIPrefBranch methods (setCharPref, setBoolPref) are not saved. I can easily see the problem in my working profile, but can be reproduced on a fresh profile with these steps. 

1. Set default preferences during initialization. Code snippet for default branch: 
    let branch = Services.prefs.getDefaultBranch("extensions.myextension.");
    branch.setCharPref("test.string", "Hello world");
2. Modify 'extensions.myextension.test.string' to say "Old String" in about:config
3. Do a reset (Right click-> Reset) immediately to reset the value.
4. Force quit ff (Ctrl+C for console or a SIGKILL should do).

Start ff again, and If the problem has occured, one can see "Old string" in the preference rather than the reset value. If the string has the expected value (reset value), I could easily hit the problem by repeating the above steps one more time.

At this stage, any more changes to this preference value will not get saved as long as a force quit is performed. This can be tested by these steps : 
1. Set the default preference as usual
2. Change the preference value from js on the preference branch.
    const prefs = Components.classes["@mozilla.org/preferences-service;1"]
                                             .getService(Components.interfaces.nsIPrefBranch);
    prefs.setCharPref("extensions.myextension.test.string", "New string");
3. Force quit. 

Remove the code for preference setting on preference branch, and restarting firefox shows "Old String"! The new preferences will be saved only if I shutdown ff normally. 
If this is really a problem (and not some mistake from my end), then I believe it's a major issue, as many users do not properly shutdown the browser (force quits happen with OS shutdown etc.)




Actual results:

Preferences are not saved when ff is force quit.


Expected results:

User preferences must be saved immediately and must be available after a browser restart.
Severity: normal → major
Component: Untriaged → Preferences
Component: Preferences → Preferences: Backend
Product: Firefox → Core
Bug 789945 was kinda tracking this but only implemented the "async" part not the reliable write on dirty, so I'm going to take this bug to do the rest.
Assignee: nobody → benjamin
Status: UNCONFIRMED → NEW
Depends on: 789945
Ever confirmed: true
Priority: -- → P2
Note: due to a bunch of unit test failures, this patch series depends on bug 1367813 landing first.

This patch series is currently stacked on top of bug 1372988 but that is not essential and I'm planning on reordering before landing since this is less risky.
Depends on: 1367813
Comment on attachment 8879612 [details]
Bug 981818 - Save preferences every time they are dirty on an timer for batching (only enabled when async pref writes are enabled)

https://reviewboard.mozilla.org/r/150956/#review155808

::: modules/libpref/Preferences.cpp:107
(Diff revision 1)
>    }
>    if (gHashTable && sPreferences && !sPreferences->mDirty) {
>      sPreferences->mDirty = true;
>      MOZ_ASSERT(!sPreferences->mProfileShutdown, "Setting user pref after profile shutdown.");
> +
> +    static const int MAX_PREF_DELAY_MS = 2000;

It may be worth having a preference for the delay, perhaps it would let us run a telemetry experiment (not sure what we'd measure, but that's a separate topic.)

::: modules/libpref/Preferences.cpp:109
(Diff revision 1)
>      sPreferences->mDirty = true;
>      MOZ_ASSERT(!sPreferences->mProfileShutdown, "Setting user pref after profile shutdown.");
> +
> +    static const int MAX_PREF_DELAY_MS = 2000;
> +    NS_IdleDispatchToCurrentThread(
> +      mozilla::NewRunnableMethod("save dirty pref file",

I don't think we want to do this when we're not allowing off main thread preference saves.  So, either we get rid of the preference controlling that, and always save pref files off main thread, or we look at that preference here as well.
Given that we know some preferences show up very late, we probably want to "disconnect" the dirty notification at some point in the shutdown process (i.e., right after we do a save in profile-before-change), so that we're not tempted to try more saves for those late changes.
Comment on attachment 8879612 [details]
Bug 981818 - Save preferences every time they are dirty on an timer for batching (only enabled when async pref writes are enabled)

https://reviewboard.mozilla.org/r/150956/#review155840

::: modules/libpref/Preferences.cpp:108
(Diff revision 1)
>    if (gHashTable && sPreferences && !sPreferences->mDirty) {
>      sPreferences->mDirty = true;
>      MOZ_ASSERT(!sPreferences->mProfileShutdown, "Setting user pref after profile shutdown.");
> +
> +    static const int MAX_PREF_DELAY_MS = 2000;
> +    NS_IdleDispatchToCurrentThread(

Does this need to be one of those "labelled" dispatches (SystemGroup?) that we're using to improve scheduling?  Or is the 2s delay enough to cover for it?
I don't know much about the new dispatching setup. Who would I ask?
:billm - I've passed the query, they'll talk about it, as it isn't clear if the delayed dispatches like this should be automatically assumed to be in a certain group.
Elsewhere in Preferences.ccp, in the PWRunnable::Run method, there is an example of "labelling" the dispatchh as a system group, but not sure if the delayed dispatch is covered in the API.
Comment on attachment 8879613 [details]
Bug 981818 part B - don't bother explicitly saving prefs when we know that they'll be written out automatically,

https://reviewboard.mozilla.org/r/150958/#review155740

Tentative r=me, though I'd like Marco to have a look at the sanitize.js change and a mobile peer to look at the mobile/android changes in light of the different OS logic about killing apps there. Minor other comments below.

::: browser/base/content/sanitize.js:865
(Diff revision 1)
>    let s = new Sanitizer();
>    s.prefDomain = "privacy.clearOnShutdown.";
>    await s.sanitize(null, options);
>    // We didn't crash during shutdown sanitization, so annotate it to avoid
>    // sanitizing again on startup.
>    Preferences.set(Sanitizer.PREF_SANITIZE_DID_SHUTDOWN, true);

I'd like Marco to, uh, sanity-check this change, given the talk of crash safety.

::: browser/components/nsBrowserGlue.js
(Diff revision 1)
> -    // This method can be called via [NSApplication terminate:] on Mac, which
> -    // ends up causing prefs not to be flushed to disk, so we need to do that
> -    // explicitly here. See bug 497652.
> -    Services.prefs.savePrefFile(null);

Why is removing this OK? Is the idle dispatch guaranteed to still fire even though we're shutting down?

::: mobile/android/chrome/content/browser.js:1717
(Diff revision 1)
>          // Ensure that this choice is immediately persisted, because
>          // Gecko won't be told again if it forgets.
>          Services.prefs.setCharPref("intl.locale.os", languageTag);
> -        Services.prefs.savePrefFile(null);

Because of how backgrounding and involuntary shutdown works on android, I'm not sure if removing this makes sense. Getting an android peer to review would be a good idea. Note that if we're removing this, the comment should be removed, too.

::: toolkit/content/widgets/preferences.xml
(Diff revision 1)
>              var preference = preferences[i];
>              preference.batching = true;
>              preference.valueFromPreferences = preference.value;
>              preference.batching = false;
>            }
> -          if (aFlushToDisk) {

Please also remove the batching field, the reads/writes to it just above this code, and the `aFlushToDisk` argument. Note that this will have add-on impact, and I'm not 100% sure that this will be OK for all consumers. Seems like the only in-tree one is for ondialogaccept for prefdialogs, which I think is fine for Firefox - all the in-content pref dialogs should persist for another 2 seconds unless the main browser goes down in those 2 seconds (and by the same argument, that might currently happen before the flush completes).
Attachment #8879613 - Flags: review?(gijskruitbosch+bugs) → review+
Bill said he thinks we can label delayed runnables but he's going to verify the API is there.
Flags: needinfo?(wmccloskey)
This is an idle dispatch, not a delayed dispatch. Idle dispatch doesn't need to be labeled.

However, the name that's passed to NewRunnableMethod should be changed to "Preferences::SavePrefFileAsynchronous" (instead of "save dirty pref file").
Flags: needinfo?(wmccloskey)
Attachment #8879613 - Attachment is obsolete: true
I've decided to land part A of this only, and the behavior is conditioned on having async saving enabled. I'll move part B to a separate bug which only lands once we've shipped async pref saving and removed the sync-saving path.
Comment on attachment 8879612 [details]
Bug 981818 - Save preferences every time they are dirty on an timer for batching (only enabled when async pref writes are enabled)

https://reviewboard.mozilla.org/r/150956/#review156428
Attachment #8879612 - Flags: review?(milan) → review+
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b34a8ccdb17
Save preferences every time they are dirty (on an idle timer for batching and jank reduction). r=milan
https://hg.mozilla.org/mozilla-central/rev/8b34a8ccdb17
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Backed out in https://hg.mozilla.org/mozilla-central/rev/63a63e56db037c889c762484f4c8b8cb26c9c915 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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla56 → ---
Depends on: 1375848
Depends on: 1377260
Comment on attachment 8879612 [details]
Bug 981818 - Save preferences every time they are dirty on an timer for batching (only enabled when async pref writes are enabled)

re-requesting review because I made substantive changes. I've gotten rid of the idle callback and instead am using a straight 500ms timer. This should coalesce much more effectively and also deal with a persistent problem where it appears that idle runnables often fire in the middle of the startup sequence (I'll file that separately).

The two issues caused by this bug should now be resolved:
* the orange was caused by a DOM test enumerating the profile directory and they've changed the test
* the I/O counters during startup should end up with similar behavior as before due to the timer+write coalescing
Attachment #8879612 - Flags: review+ → review?(milan)
Comment on attachment 8879612 [details]
Bug 981818 - Save preferences every time they are dirty on an timer for batching (only enabled when async pref writes are enabled)

https://reviewboard.mozilla.org/r/150956/#review162910

::: modules/libpref/Preferences.cpp:111
(Diff revision 3)
>      NS_WARNING_ASSERTION(!sPreferences->mProfileShutdown,
>                           "Setting user pref after profile shutdown.");
> +
> +    if (sPreferences->AllowOffMainThreadSave() && !sPreferences->mSavePending) {
> +      sPreferences->mSavePending = true;
> +      static const int PREF_DELAY_MS = 500;

Would you consider making this value a preference
?  I'm often nervous about hardcoding heuristcs.  Perhaps in a follow up bug...

::: modules/libpref/Preferences.cpp:111
(Diff revision 3)
>      NS_WARNING_ASSERTION(!sPreferences->mProfileShutdown,
>                           "Setting user pref after profile shutdown.");
> +
> +    if (sPreferences->AllowOffMainThreadSave() && !sPreferences->mSavePending) {
> +      sPreferences->mSavePending = true;
> +      static const int PREF_DELAY_MS = 500;

Would you consider making this value a preference?  Perhaps in a follow up bug?
Attachment #8879612 - Flags: review?(milan) → review+
I thought about it but the obvious way to do that doesn't work because this is running inside the dirty callback where some hashtables may be in an inconsistent state. So we'd have to cache it.

So followup it is, at least to make an explicit decision.
Right, it would have to be something like ::AllowOffMainThreadSave() that gets initialized earlier.
We still have the telemetry for the total number of saves to disk, right?  I think we'll want to see what a change like this does to that number, and then perhaps, if the timeout is controlled by a preference, see how that timeout value affects it...
AFAIK we have never had telemetry on the number of saves to disk, and I don't see any of that currently.
Right, I was thinking of the profiler label.  So, yeah, never mind.
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b14c76c32914
Save preferences every time they are dirty on an timer for batching (only enabled when async pref writes are enabled) r=milan
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30af079927c7
followup - initialize member variable for correctness and valgrind happiness, r=trivial
Depends on: 1381808
https://hg.mozilla.org/mozilla-central/rev/b14c76c32914
https://hg.mozilla.org/mozilla-central/rev/30af079927c7
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: