Last Comment Bug 789945 - Do automatic periodic async, off-mainthread writes for the prefs file when prefs change
: Do automatic periodic async, off-mainthread writes for the prefs file when pr...
Status: ASSIGNED
[Snappy:P2]
: main-thread-io
Product: Core
Classification: Components
Component: Preferences: Backend (show other bugs)
: 16 Branch
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: :Gijs Kruitbosch (away 26-29 incl.)
:
Mentors:
Depends on: 1047389 1288519 918317 1056170 1287945
Blocks: 1133405 683808
  Show dependency treegraph
 
Reported: 2012-09-10 07:53 PDT by Vladan Djeric (:vladan)
Modified: 2016-07-21 13:25 PDT (History)
26 users (show)
gijskruitbosch+bugs: firefox‑backlog+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
do async omt pref writes - WIP, f?vladan (23.82 KB, patch)
2016-01-01 09:52 PST, :Gijs Kruitbosch (away 26-29 incl.)
aklotz: feedback+
Details | Diff | Splinter Review
Debug printfs I used (9.18 KB, patch)
2016-01-01 09:53 PST, :Gijs Kruitbosch (away 26-29 incl.)
no flags Details | Diff | Splinter Review
MozReview Request: Bug 789945 - do async omt pref writes, r?bsmedberg (58 bytes, text/x-review-board-request)
2016-01-15 12:02 PST, :Gijs Kruitbosch (away 26-29 incl.)
gijskruitbosch+bugs: review? (benjamin)
Details | Review
MozReview Request: Bug 789945 - part 2: add asyncshutdown blocker, r?bsmedberg (58 bytes, text/x-review-board-request)
2016-01-15 12:02 PST, :Gijs Kruitbosch (away 26-29 incl.)
gijskruitbosch+bugs: review? (benjamin)
Details | Review
Debug printfs I used (6.62 KB, patch)
2016-01-15 12:04 PST, :Gijs Kruitbosch (away 26-29 incl.)
no flags Details | Diff | Splinter Review

Description Vladan Djeric (:vladan) 2012-09-10 07:53:31 PDT
There is a significant number of Telemetry reports showing multi-second hangs from preferences being written to disk synchronously.

NtFlushBuffersFile (in ntdll.dll)
 -> FlushFileBuffersImplementation (in kernel32.dll)
 -> FileSync (in nspr4.dll)
 -> nsSafeFileOutputStream::Finish() (in xul.dll)
 -> nsBufferedOutputStream::Finish() (in xul.dll)
 -> mozilla::Preferences::WritePrefFile(nsIFile *) (in xul.dll)
 -> mozilla::Preferences::SavePrefFileInternal(nsIFile *) (in xul.dll)
 -> mozilla::Preferences::SavePrefFile(nsIFile *) (in xul.dll)
 -> NS_InvokeByIndex_P (in xul.dll)
 -> XPCWrappedNative::CallMethod(XPCCallContext &,XPCWrappedNative::CallMode) (in xul.dll)
Comment 1 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-21 08:59:01 PDT
I have the impression that backgrounding writes (with OS.File atomic write) would be pretty low-risk.
Comment 2 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-22 13:41:15 PDT
This stack trace is interesting, as it seems that the multi-second hangs originate from JavaScript. This would make a rewrite with OS.File rather natural, if we can serialize the preference file to a string.
Comment 3 Marco Castelluccio [:marco] 2013-02-08 14:44:17 PST
We could use nsIBackgroundFileSaver, that stores the file through a background thread.
Comment 4 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-02-08 14:47:40 PST
(In reply to Marco Castelluccio [:marco] from comment #3)
> We could use nsIBackgroundFileSaver, that stores the file through a
> background thread.

The main problem with this is that each write would require a new thread. I am almost sure that we have cases in which we modify many preferences in a row and save after each change, so this might possibly explode.
Comment 5 Ian Nartowicz 2013-09-01 07:07:41 PDT
(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> (In reply to Marco Castelluccio [:marco] from comment #3)
> > We could use nsIBackgroundFileSaver, that stores the file through a
> > background thread.
> 
> The main problem with this is that each write would require a new thread. I
> am almost sure that we have cases in which we modify many preferences in a
> row and save after each change, so this might possibly explode.

I've just been pointed to this bug because of issues I am having with exactly this situation.  It occurs in instant-apply prefwindows when savePrefFile() is called after every preference value change.  While that might not sound so terrible, it can occur for every single typed (or deleted) character in a textbox.  That, or programmatically applying multiple preference changes, can result in the multi-second hangs you are seeing from the telemetry.

This particular instance could be semi-solved just by modifying the preferences binding.  That still leaves a blocking write of the prefs file at some point, just not instantly and not for every single character.  The non-instant-apply approach to writing out all the preferences when the dialog closes is to not call savePrefFile() until all the changes have been applied, controlled by a batching property on the preference element.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-09-16 09:00:37 PDT
(In reply to Ian Nartowicz from comment #5)
> This particular instance could be semi-solved just by modifying the
> preferences binding.  That still leaves a blocking write of the prefs file
> at some point, just not instantly and not for every single character.

Yes, we should fix the preferences bindings to avoid calling savePrefFile too often (I thought there was a bug on file about that already; if not, please file one!). This bug is about the more general problem and is harder to fix.
Comment 7 :Gijs Kruitbosch (away 26-29 incl.) 2014-08-05 04:33:40 PDT
So, I would *think* that we can't just switch the API here, because on shutdown we actually need to be sure we write out the prefs, because of how the prefs system works right now.

Would switching to a regular async/OMT flush system work (ie saving automatically and asynchronously, off main thread, and saving no more often than once every 5 minutes or so (a la DelayedTask)), combining that with guarding savePrefFile to no-op iff passed "null" as the file and with no dirty state?

AFAICT this would eliminate the sync disk writes in most cases (where a pref has not been modified within 5 minutes of shutdown/backgrounding) and improve reliability in data loss scenarios.

Also, is this something we could work on as part of the backlog, Gavin? :-)
Comment 8 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2014-08-05 08:28:18 PDT
If we wish to do this in JS (e.g. by rebinding Services.prefs to a PreferenceService.jsm, rather than the XPCOM implementation), I suggest taking advantage of the OS.File.writeAtomic backup mechanism http://wp.me/p52O1-mh and using it for recovery if the file is corrupted, which should go a long way towards ensuring that we do not corrupt data. Similarly, if we do it in native code, a similar rename/backup mechanism would be a good idea.

Regardless, please use this with AsyncShutdown. I hope to land the XPCOM version in August (bug 918317).
Comment 9 :Gijs Kruitbosch (away 26-29 incl.) 2014-08-05 08:33:22 PDT
(In reply to David Rajchenbach Teller [:Yoric] (away until August 20th - use "needinfo") from comment #8)
> If we wish to do this in JS (e.g. by rebinding Services.prefs to a
> PreferenceService.jsm, rather than the XPCOM implementation)

This is generally an interesting idea - I don't know if there's a good reason not to implement the prefs system in JS instead of C++...

(we'd probably still want a small XPCOM wrapper for cpp, of course...)
Comment 10 :Irving Reid (No longer working on Firefox) 2014-08-05 09:00:07 PDT
If we're implementing in JS, DeferredSave.jsm could be what you're looking for - batches up "dirty" notifications for a configured delay period, then writes out the file asynchronously. It supports an explicit flush for shutdown time, and handles all the ugly edge cases of the data becoming dirty again part-way through a write and so on.

That said, prefs has so many C++ clients that I'd be pretty nervous about porting the main API to JS.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2014-08-05 09:53:39 PDT
(In reply to :Gijs Kruitbosch (Bugmail catchup, needinfo if urgent) from comment #7)
> Would switching to a regular async/OMT flush system work (ie saving
> automatically and asynchronously, off main thread, and saving no more often
> than once every 5 minutes or so (a la DelayedTask)), combining that with
> guarding savePrefFile to no-op iff passed "null" as the file and with no
> dirty state?

Sounds like a good plan. 5 minutes sounds like a lot - on the order of "seconds" would probably achieve a similar effect while lowering the chances of needing to write out synchronously on shutdown.

> AFAICT this would eliminate the sync disk writes in most cases (where a pref
> has not been modified within 5 minutes of shutdown/backgrounding) and
> improve reliability in data loss scenarios.

I'm not sure that that's "most cases" (I could see some code setting prefs on shutdown regularly), but it would be interesting to find out (via telemetry)!

> Also, is this something we could work on as part of the backlog, Gavin? :-)

Certainly, if we have a clear plan. I'm less certain about the relative priority of this compared to other main-thread-IO/perf efforts, perhaps Vladan can advise?
Comment 12 :Gijs Kruitbosch (away 26-29 incl.) 2014-08-06 01:25:23 PDT
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #11)
> (In reply to :Gijs Kruitbosch (Bugmail catchup, needinfo if urgent) from
> comment #7)
> > Also, is this something we could work on as part of the backlog, Gavin? :-)
> 
> Certainly, if we have a clear plan. I'm less certain about the relative
> priority of this compared to other main-thread-IO/perf efforts, perhaps
> Vladan can advise?

Vladan? :-)
Comment 13 Benjamin Smedberg [:bsmedberg] 2014-08-06 07:31:50 PDT
I don't think you need telemetry to measure pref writes during shutdown: I bet you could do it just by running the browser a bit. In any case, I think we should tightly scope this bug to periodically writing the pref file off the main thread before shutdown when it's dirty. At shutdown if it's dirty we can either synchronously write it one last time from the main thread, or use some sort of shutdown blocker to wait for the last async write.

We shouldn't reimplement this in JS now. If we're thinking about a deeper prefs overhaul, we need a redesign (and I have thoughts I can dump somewhere).

I actually think it's a high priority for data quality to do the periodic save (e.g. wait 5 seconds after prefs are dirty and then write them), entirely apart from the jank associated with the main-thread write. So I'd be happy to say we should prioritize this soon.
Comment 14 :Gijs Kruitbosch (away 26-29 incl.) 2014-08-06 12:38:05 PDT
Sounds like there's enough consensus to include this in the backlog, at the least. I would also agree with Benjamin that it would probably make sense to prioritize soon. We can do followups (to change consumers or such) later once we at least have an async, OMT way to save prefs.
Comment 15 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2014-08-20 04:29:07 PDT
Let's make sure this works nicely with AsyncShutdown, so let's wait until bug 918317 has landed.
Comment 16 :Irving Reid (No longer working on Firefox) 2014-08-20 09:07:25 PDT
We already skip writing to the default prefs file when the preferences aren't dirty: http://hg.mozilla.org/mozilla-central/annotate/ffdd1a398105/modules/libpref/Preferences.cpp#l916
Comment 17 Vladan Djeric (:vladan) 2014-08-20 17:14:30 PDT
I can work on this. We can use this bug for the short-term fix described in the comments, and then we can address proper prefs storage (e.g. log storage) in bug 897049 and bug 956713
Comment 18 Jared Wein [:jaws] (please needinfo? me) 2015-08-24 11:35:00 PDT
Hey Vladan, is there any update on this? Do you think you could find someone to work on it?
Comment 19 Jared Wein [:jaws] (please needinfo? me) 2015-08-24 11:35:24 PDT
(This came up in bug 1185567 fyi)
Comment 20 Vladan Djeric (:vladan) 2015-08-24 12:06:31 PDT
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> Hey Vladan, is there any update on this? Do you think you could find someone
> to work on it?

I wanted Aaron Klotz to work on this, but this quarter it's looking unlikely, given the lengthy list of other projects we have been asked to do. I'd be happy to turn this over to anyone interested and help them get started, discuss design, etc. If it's not done by Q4, I will ask Aaron to work on it.
Comment 21 :Gijs Kruitbosch (away 26-29 incl.) 2015-08-24 12:10:43 PDT
Is there other in-tree code that does OMT writes that could serve as some inspiration if I wanted to take a stab at this pre-Q4?
Comment 22 Vladan Djeric (:vladan) 2015-08-24 12:27:50 PDT
There's a bunch of C++ code that writes files OMT in the codebase. bug 880864 is one example, LocalStorage is another, there's also the reading of Telemetry.ShutdownTime.txt in Telemetry.cpp.

The basic approach is to send an event to the STS event queue and have the event do the I/O on the STS thread.

There's also an open question regarding acceptable delays for writing prefs. Some code (such as startupCrash detection) writes to prefs and expects the pref state to be resilient to a crash after the write call returns.
Comment 23 :Gijs Kruitbosch (away 26-29 incl.) 2016-01-01 09:52:18 PST
Created attachment 8703278 [details] [diff] [review]
do async omt pref writes - WIP, f?vladan

Happy new year!

It seems I didn't get anything done before Q4, and nobody else did anything in Q4 either, so today/yesterday I figured I'd have a poke at this.

This is a pretty rough WIP. I haven't done concurrency in C++ before. I'm mostly wondering if this is the right approach, if there are glaring things I've missed, or if there's a simpler way to do this.

Some notes:
- I left the SavePrefFile API alone (ie it is still sync), because there are some API consumers that I'm fairly sure assume that they get a sync write out of it.
- Telemetry and a number of other things write to the prefs right after startup which triggers a flurry of separate writes, so I ended up upping the delay to 15s instead of just 5
- I haven't implemented shutdown blocking yet. AFAICT, both pre- and post-patch there is no guarantee we'll write an updated file before shutdown (!). Happy to look at doing that, but it seems there's no MDN docs for the C++ version of the AsyncShutdown APIs.
- There are currently shutdown-time pref writes that don't get flushed to disk. I noticed SocialService.jsm that does this for xpcom-shutdown (ie sets a pref, potentially for no reason, but doesn't flush to disk), there might be others. So if I'm right that there's no guarantee right now, implementing such a guarantee might potentially regress shutdown performance (oh, the irony) if we don't update consumers
- I've not updated any consumers (that frequently call savePrefFile, that is) in this patch, but arguably we should do that when we land this and/or soon after.
Comment 24 :Gijs Kruitbosch (away 26-29 incl.) 2016-01-01 09:53:52 PST
Created attachment 8703279 [details] [diff] [review]
Debug printfs I used

Dunno if this is helpful, or could be helpful if I converted it to use "proper" NSPR logging or something, but I did use them to debug and check that my implementation was doing vaguely sane things, so I figured I might as well add them here.
Comment 25 Vladan Djeric (:vladan) 2016-01-02 10:24:46 PST
Comment on attachment 8703278 [details] [diff] [review]
do async omt pref writes - WIP, f?vladan

Review of attachment 8703278 [details] [diff] [review]:
-----------------------------------------------------------------

I'm on PTO this week, so redirecting to aklotz
Comment 26 Aaron Klotz [:aklotz] 2016-01-11 19:29:43 PST
Comment on attachment 8703278 [details] [diff] [review]
do async omt pref writes - WIP, f?vladan

Review of attachment 8703278 [details] [diff] [review]:
-----------------------------------------------------------------

OK, I do have a few suggestions. I'm not super familiar with libpref code so it's probably best to ask one of those peers to look at this as well.

::: modules/libpref/Preferences.cpp
@@ +107,5 @@
> +  NS_LINEBREAK;
> +
> +static const uint32_t kPrefDelayedSaveTimeoutMs = 15000;
> +
> +

Nit: extra blank line

@@ +220,5 @@
> +{
> +public:
> +  PreferenceRunnableSaver(nsIFile* aFile)
> +  {
> +    mFile = aFile;

Nit: please put these in an initializer list.

@@ +304,5 @@
> +    }
> +  }
> +
> +private:
> +  RefPtr<nsIFile> mFile;

nsCOMPtr since you're typing on an interface and not a concrete class.

::: modules/libpref/Preferences.h
@@ +375,5 @@
>  
>  private:
>    nsCOMPtr<nsIFile>        mCurrentFile;
>  
> +  RefPtr<nsITimer>         mCurrentOMTSaveTimer;

This should be nsCOMPtr since it is typed on an interface instead of a concrete class.

::: modules/libpref/prefapi.cpp
@@ +987,5 @@
> +void PREF_MarkDirty()
> +{
> +  if (!gDirty) {
> +    gDirty = true;
> +    gDirtyCallback();

Make sure this is non-null before calling.
Comment 27 Tony Mechelynck [:tonymec] 2016-01-12 19:23:33 PST
I hope that the write-on-shutdown will happen early and/or that there will remain a few synchronous writes because I have seen shutdown hangs in very busy circumstances (see e.g. my bug 570316, which seemed to have disappeared but has recently resurfaced).

I can think of at least the following:
- closing the prefs dialog, especially closing it by [OK] with browser.preferences.instantApply being false;
- closing the about:config tab. Maybe even refreshing it but I'm less sure.
Comment 28 :Gijs Kruitbosch (away 26-29 incl.) 2016-01-15 12:02:34 PST
Created attachment 8708517 [details]
MozReview Request: Bug 789945 - do async omt pref writes, r?bsmedberg

Review commit: https://reviewboard.mozilla.org/r/31079/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31079/
Comment 29 :Gijs Kruitbosch (away 26-29 incl.) 2016-01-15 12:02:36 PST
Created attachment 8708518 [details]
MozReview Request: Bug 789945 - part 2: add asyncshutdown blocker, r?bsmedberg

Review commit: https://reviewboard.mozilla.org/r/31081/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31081/
Comment 30 :Gijs Kruitbosch (away 26-29 incl.) 2016-01-15 12:04:01 PST
Created attachment 8708519 [details] [diff] [review]
Debug printfs I used
Comment 31 :Gijs Kruitbosch (away 26-29 incl.) 2016-01-15 12:15:47 PST
https://reviewboard.mozilla.org/r/31079/#review27911

I applied all the nits from aklotz, I think. (Thanks!)

The 2nd part implements a "proper" shutdown blocker instead of the current call to savePrefFile when profile-before-change fires. In general, this won't change anything compared to the status quo, but it will ensure that a running async save will complete before we shut down. Note that this means we'll still be doing an IO-on-the-main-thread save as we are now.

In this sense both these patches are conservative - savePrefFile() also continues to do sync main thread IO. I'd want to file followup bugs to teach the preference bindings not to be so eager to call that method themselves.

As a consequence, right now, this would often result in more disk IO than we currently do, because in some cases we'd normally change prefs and not save to disk, and we will now start async saving to disk before shut down. Some of telemetry/healthreport (particularly the "datareporting.sessions.current.totalTime" and friends prefs) writes prefs "late", meaning we almost always shutdown-write prefs anyway.

::: modules/libpref/Preferences.cpp:294
(Diff revision 1)
> +      // Notify the main thread we're done
> +      mozilla::MonitorAutoLock mon(Preferences::sPreferences->mOMTSaveMonitor);
> +      Preferences::sPreferences->mAreWaitingForOMTSave = false;
> +
> +      mon.Notify();

Note that I changed the ordering here compared to the earlier patch to match what the personal dictionary stuff was doing.

As noted earlier, it'd be good if someone actually sanity-checked me carefully, because after years of being told threads are hard, and not all that much experience with them, I'm not at all confident that this code is currently correct as written. :-)
Comment 32 :Gijs Kruitbosch (away 26-29 incl.) 2016-01-18 06:49:22 PST
https://reviewboard.mozilla.org/r/31081/#review27995

::: modules/libpref/Preferences.cpp:1042
(Diff revision 1)
> +  if (!hadCurrentFile) {
> +    mShutdownBlocker = new PrefShutdownBlocker(
> +      NS_LITERAL_STRING("Pref service shutdown blocker"));
> +    nsCOMPtr<nsIAsyncShutdownClient> shutdownPhase = GetShutdownPhase();
> +    shutdownPhase->AddBlocker(mShutdownBlocker,
> +                              NS_LITERAL_STRING(__FILE__),
> +                              __LINE__,
> +                              NS_LITERAL_STRING("Pref service shutdown blocker"));
> +  }

Note that I'm doing this here rather than in Init() for two reasons:
1) doing it in Init() breaks the world, because AsyncShutdown tries to read preferences
2) there isn't really a point in doing writes on shutdown (or blocking shutdown for such writes) unless we have an `mCurrentFile` to write to.
Comment 33 :Gijs Kruitbosch (away 26-29 incl.) 2016-02-19 10:30:48 PST
Milan, when I asked Benjamin about this patch ~3 weeks ago he suggested that perhaps you could review this if you feel comfortable doing so - do you? :-)
Comment 34 Milan Sreckovic [:milan] 2016-02-19 10:39:11 PST
Sure, I'll take a look.
Comment 35 Milan Sreckovic [:milan] 2016-02-19 11:30:25 PST
Are we now guaranteed that all the preferences setting is done on the main thread?  I know we had a big push to do that, wanted to confirm we've completed that work.
Comment 36 :Gijs Kruitbosch (away 26-29 incl.) 2016-02-19 12:07:46 PST
(In reply to Milan Sreckovic [:milan] from comment #35)
> Are we now guaranteed that all the preferences setting is done on the main
> thread?  I know we had a big push to do that, wanted to confirm we've
> completed that work.

There are some asserts for that in the code (ifdef'd out for b2g, admittedly), so I hope/assume so, but I don't know for sure.
Comment 37 Milan Sreckovic [:milan] 2016-02-22 11:04:58 PST
https://reviewboard.mozilla.org/r/31079/#review32313

Thought I'd send a few things while looking at the rest.

::: modules/libpref/Preferences.cpp:109
(Diff revision 1)
> +static const uint32_t kPrefDelayedSaveTimeoutMs = 15000;

I'd add a way to overwrite this, perhaps with an environment variable.  It would give us a quick way of triaging in the field.  Perhaps to the point where we're able to disable the async saving with that environment variable (e.g., zero negative value)

::: modules/libpref/Preferences.cpp:206
(Diff revision 1)
> +    MOZ_ASSERT(NS_IsMainThread());

We're in big trouble if this is ever called off main thread, so I'd go stronger than debug assert.  MOZ_CRASH or MOZ_RELEASE_ASSERT, or even guarding against it, rather than asserting.

::: modules/libpref/Preferences.cpp:210
(Diff revision 1)
> +      if (gDirty) {

gDirty will be set to true if we fail in the OMT write.  Before this patch, we would just fail, perhaps report, and be done with it.  With this patch, we are going to schedule another OMT write? That's new, are we sure we want it?

::: modules/libpref/Preferences.cpp:233
(Diff revision 1)
> +    // execute a "safe" save by saving through a tempfile

Do we want to check if gHashTable exists, or are we safe?

::: modules/libpref/Preferences.cpp:1030
(Diff revision 1)
>      if (!gDirty)

The "contract" is that when we return from SavePrefFileInternal(), the pref file on disk is up to date, correct?  If we set gDirty to false just before we dispatch, this won't be the case.  We seem to be already handling the "there is an OMT write in progress at some level" below.

This should probably stay, it's more describing why it doesn't feel right that we're setting gDirty to false in the timer callback.

::: modules/libpref/Preferences.cpp:1035
(Diff revision 1)
> +      if (!mAreWaitingForOMTSave) {

At first I thought that the monitor is protecting  mAreWaitingForOMTSave as the condition variable.  It seems to be the case in a couple of places (we get the lock, then we deal with that variable), but not in all of them?  Or am I reading the code wrong?

::: modules/libpref/Preferences.cpp:1070
(Diff revision 1)
> -  // execute a "safe" save by saving through a tempfile
> +  PreferenceRunnableSaver* saver = new PreferenceRunnableSaver(aFile);

This feels like a memory leak?

::: modules/libpref/Preferences.cpp:1093
(Diff revision 1)
> -      pref = nullptr;
> +      gDirty = false;

I'm not sure about this one.  Why reset dirty here and then change it back in the runnable's Run() method, if things go wrong?

::: modules/libpref/Preferences.cpp:1097
(Diff revision 1)
> +    } else {

The callback happens on the main thread, right? When would we ever be in a situation where gDirty is false?
Comment 38 Milan Sreckovic [:milan] 2016-02-24 11:25:36 PST
https://reviewboard.mozilla.org/r/31079/#review32947

::: modules/libpref/Preferences.h:384
(Diff revision 1)
> +  mozilla::Monitor         mOMTSaveMonitor;

Tracing through the code, it's starting to feel like we could benefit from a "state tracking" type of enum, rather than a boolean and the existence of the runnable.  Same functionality, just more explicit on what the current state is, between not doing anything, and having sent the request and being in the middle of processing the request, etc.

::: modules/libpref/prefapi.h:195
(Diff revision 1)
> +void PREF_MarkDirty();

I like this not only for new functionality, but for hiding the direct access of gDirty.  To the point where we may want to remove gDirty from extern, leave it in Preferences (probably just as a member variable instead of a global) and have the callback take care of both setting the state and deciding what to do afterwards:

Remove PREF_MarkDirty()after replacing it with

if (gDirtyCallback) gDirtyCallback();


and make the callback check:

void
Preferences::PrefsDirtyCallback()
{
  if (sPreferences && gHashTable && gDirty) {
    sPreferences->SavePrefsToDefaultFileOMT();
  }
}

Just random thoughts :)

Note You need to log in before you can comment on or make changes to this bug.