diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp index 40c76fc5ba77..7875044379b6 100644 --- a/modules/libpref/Preferences.cpp +++ b/modules/libpref/Preferences.cpp @@ -3039,27 +3039,36 @@ class PreferencesWriter final { } static void Flush() { - // This can be further optimized; instead of waiting for all of the writer - // thread to be available, we just have to wait for all the pending writes - // to be done. - if (!sPendingWriteData.compareExchange(nullptr, nullptr)) { - nsresult rv = NS_OK; - nsCOMPtr target = - do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID, &rv); - if (NS_SUCCEEDED(rv)) { - target->Dispatch(NS_NewRunnableFunction("Preferences_dummy", [] {}), - nsIEventTarget::DISPATCH_SYNC); - } - } + MOZ_DIAGNOSTIC_ASSERT(sPendingWriteCount >= 0); + // SpinEventLoopUntil is unfortunate, but ultimately it's the best thing + // we can do here given the constraint that we need to ensure that + // the preferences on disk match what we have in memory. We could + // easily perform the write here ourselves by doing exactly what + // happens in PWRunnable::Run. This would be the right thing to do + // if we're stuck here because other unrelated runnables are taking + // a long time, and the wrong thing to do if PreferencesWriter::Write + // is what takes a long time, as we would be trading a SpinEventLoopUntil + // for a synchronous disk write, wherein we could not even spin the + // event loop. Given that PWRunnable generally runs on a thread pool, + // if we're stuck here, it's likely because of PreferencesWriter::Write + // and not some other runnable. Thus, spin away. + mozilla::SpinEventLoopUntil([]() { return sPendingWriteCount <= 0; }); } // This is the data that all of the runnables (see below) will attempt // to write. It will always have the most up to date version, or be // null, if the up to date information has already been written out. static Atomic sPendingWriteData; + + // This is the number of writes via PWRunnables which have been dispatched + // but not yet completed. This is intended to be used by Flush to ensure + // that there are no outstanding writes left incomplete, and thus our prefs + // on disk are in sync with what we have in memory. + static Atomic sPendingWriteCount; }; Atomic PreferencesWriter::sPendingWriteData(nullptr); +Atomic PreferencesWriter::sPendingWriteCount(0); class PWRunnable : public Runnable { public: @@ -3089,6 +3098,13 @@ class PWRunnable : public Runnable { } })); } + + // We've completed the write to the best of our abilities, whether + // we had prefs to write or another runnable got to them first. If + // PreferencesWriter::Write failed, this is still correct as the + // write is no longer outstanding, and the above HandleDirty call + // will just start the cycle again. + PreferencesWriter::sPendingWriteCount--; return rv; } @@ -4121,6 +4137,17 @@ nsresult Preferences::WritePrefFile(nsIFile* aFile, SaveMethod aSaveMethod) { do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID, &rv); if (NS_SUCCEEDED(rv)) { bool async = aSaveMethod == SaveMethod::Asynchronous; + + // Increment sPendingWriteCount, even though it's redundant to track this + // in the case of a sync runnable; it just makes it easier to simply + // decrement this inside PWRunnable. We could alternatively increment + // sPendingWriteCount in PWRunnable's constructor, but if for any reason + // in future code we create a PWRunnable without dispatching it, we would + // get stuck in an infinite SpinEventLoopUntil inside + // PreferencesWriter::Flush. Better that in future code we miss an + // increment of sPendingWriteCount and cause a simple crash due to it + // ending up negative. + PreferencesWriter::sPendingWriteCount++; if (async) { rv = target->Dispatch(new PWRunnable(aFile), nsIEventTarget::DISPATCH_NORMAL);