Backed out changeset 82ef6d7419cc (bug 1681950) for causing leaks and assertion failures. CLOSED TREE
authorCsoregi Natalia <ncsoregi@mozilla.com>
Fri, 11 Dec 2020 20:52:43 +0200
changeset 560372 020f33180480c9eab71b641ee1233c88d4fefe89
parent 560371 82ef6d7419cc43ed92a7aa64f51956ee7127a145
child 560373 c484aab51bf26bf4e9e753b86ccbebbec8bafdb3
push id132612
push userncsoregi@mozilla.com
push dateFri, 11 Dec 2020 18:55:33 +0000
treeherderautoland@020f33180480 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1681950
milestone85.0a1
backs out82ef6d7419cc43ed92a7aa64f51956ee7127a145
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out changeset 82ef6d7419cc (bug 1681950) for causing leaks and assertion failures. CLOSED TREE
modules/libpref/Preferences.cpp
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -21,17 +21,16 @@
 #include "mozilla/Attributes.h"
 #include "mozilla/Components.h"
 #include "mozilla/dom/PContent.h"
 #include "mozilla/HashFunctions.h"
 #include "mozilla/HashTable.h"
 #include "mozilla/Logging.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/MemoryReporting.h"
-#include "mozilla/Mutex.h"
 #include "mozilla/Omnijar.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/ResultExtensions.h"
 #include "mozilla/SchedulerGroup.h"
 #include "mozilla/ScopeExit.h"
 #include "mozilla/Services.h"
 #include "mozilla/ServoStyleSet.h"
 #include "mozilla/SpinEventLoopUntil.h"
@@ -2978,89 +2977,50 @@ class PreferencesWriter final {
   // null, if the up to date information has already been written out.
   static Atomic<PrefSaveData*> 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<int> sPendingWriteCount;
-
-  // See PWRunnable::Run for details on why we need this lock.
-  static Mutex sWritingToFile;
 };
 
 Atomic<PrefSaveData*> PreferencesWriter::sPendingWriteData(nullptr);
 Atomic<int> PreferencesWriter::sPendingWriteCount(0);
-Mutex PreferencesWriter::sWritingToFile("PreferencesWriter::sWritingToFile");
 
 class PWRunnable : public Runnable {
  public:
   explicit PWRunnable(nsIFile* aFile) : Runnable("PWRunnable"), mFile(aFile) {}
 
   NS_IMETHOD Run() override {
-    // Preference writes are handled a bit strangely, in that a "newer"
-    // write is generally regarded as always better. For this reason,
-    // sPendingWriteData can be overwritten multiple times before anyone
-    // gets around to actually using it, minimizing writes. However,
-    // once we've acquired sPendingWriteData we've reached a
-    // "point of no return" and have to complete the write.
-    //
-    // Unfortunately, this design allows the following behaviour:
-    //
-    // 1. write1 is queued up
-    // 2. thread1 acquires write1
-    // 3. write2 is queued up
-    // 4. thread2 acquires write2
-    // 5. thread1 and thread2 concurrently clobber each other
-    //
-    // To avoid this, we use this lock to ensure that only one thread
-    // at a time is trying to acquire the write, and when it does,
-    // all other threads are prevented from acquiring writes until it
-    // completes the write. New writes are still allowed to be queued
-    // up in this time.
-    //
-    // Although it's atomic, the acquire needs to be guarded by the mutex
-    // to avoid reordering of writes -- we don't want an older write to
-    // run after a newer one. To avoid this causing too much waiting, we check
-    // if sPendingWriteData is already null before acquiring the mutex. If it
-    // is, then there's definitely no work to be done (or someone is in the
-    // middle of doing it for us).
-    //
-    // Note that every time a new write is queued up, a new write task is
-    // is also queued up, so there will always be a task that can see the newest
-    // write.
-    //
-    // Ideally this lock wouldn't be necessary, and the PreferencesWriter
-    // would be used more carefully, but it's hard to untangle all that.
+    // If we get a nullptr on the exchange, it means that somebody
+    // else has already processed the request, and we can just return.
+    UniquePtr<PrefSaveData> prefs(
+        PreferencesWriter::sPendingWriteData.exchange(nullptr));
     nsresult rv = NS_OK;
-    if (PreferencesWriter::sPendingWriteData) {
-      MutexAutoLock lock(PreferencesWriter::sWritingToFile);
-      // If we get a nullptr on the exchange, it means that somebody
-      // else has already processed the request, and we can just return.
-      UniquePtr<PrefSaveData> prefs(
-          PreferencesWriter::sPendingWriteData.exchange(nullptr));
-      if (prefs) {
-        rv = PreferencesWriter::Write(mFile, *prefs);
-        // Make a copy of these so we can have them in runnable lambda.
-        // nsIFile is only there so that we would never release the
-        // ref counted pointer off main thread.
-        nsresult rvCopy = rv;
-        nsCOMPtr<nsIFile> fileCopy(mFile);
-        SchedulerGroup::Dispatch(
-            TaskCategory::Other,
-            NS_NewRunnableFunction("Preferences::WriterRunnable",
-                                   [fileCopy, rvCopy] {
-                                     MOZ_RELEASE_ASSERT(NS_IsMainThread());
-                                     if (NS_FAILED(rvCopy)) {
-                                       Preferences::HandleDirty();
-                                     }
-                                   }));
-      }
+    if (prefs) {
+      rv = PreferencesWriter::Write(mFile, *prefs);
+
+      // Make a copy of these so we can have them in runnable lambda.
+      // nsIFile is only there so that we would never release the
+      // ref counted pointer off main thread.
+      nsresult rvCopy = rv;
+      nsCOMPtr<nsIFile> fileCopy(mFile);
+      SchedulerGroup::Dispatch(
+          TaskCategory::Other,
+          NS_NewRunnableFunction("Preferences::WriterRunnable",
+                                 [fileCopy, rvCopy] {
+                                   MOZ_RELEASE_ASSERT(NS_IsMainThread());
+                                   if (NS_FAILED(rvCopy)) {
+                                     Preferences::HandleDirty();
+                                   }
+                                 }));
     }
+
     // 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;
   }