Backed out 3 changesets (bug 1630655) for causing failures in test_headless_screenshot.html

CLOSED TREE

Backed out changeset 939611f65438 (bug 1630655)
Backed out changeset 2e05e81cc858 (bug 1630655)
Backed out changeset bfb074351550 (bug 1630655)

modules/libpref/Preferences.cpp

@@ -3039,36 +3039,27 @@ class PreferencesWriter final {
   }
 
   static void Flush() {
-    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 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<nsIEventTarget> target =
+          do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID, &rv);
+      if (NS_SUCCEEDED(rv)) {
+        target->Dispatch(NS_NewRunnableFunction("Preferences_dummy", [] {}),
+                         nsIEventTarget::DISPATCH_SYNC);
+      }
+    }
   }
 
   // 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<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;
 };
 
 Atomic<PrefSaveData*> PreferencesWriter::sPendingWriteData(nullptr);
-Atomic<int> PreferencesWriter::sPendingWriteCount(0);
 
 class PWRunnable : public Runnable {
  public:
@@ -3098,13 +3089,6 @@ 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;
   }
 
@@ -4137,17 +4121,6 @@ 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);

modules/libpref/init/StaticPrefList.yaml

@@ -8462,11 +8462,11 @@
 #ifdef MOZ_CODE_COVERAGE
   value: 0
 #else
-  #ifdef NIGHTLY_BUILD
+#  ifdef NIGHTLY_BUILD
   value: 3
-  #else
+#  else
   value: 1
-  #endif
+#  endif
 #endif
   mirror: always
 
@@ -8475,7 +8475,7 @@
 - name: toolkit.shutdown.fastShutdownStage
   type: int32_t
 #if defined(NIGHTLY_BUILD) && !defined(DEBUG) && !defined(MOZ_CODE_COVERAGE) && !defined(MOZ_VALGRIND) && !defined(MOZ_PROFILE_GENERATE)
-  value: 1
+  value: 2
 #else
   value: 0
 #endif

xpcom/build/IOInterposer.cpp

@@ -290,10 +290,11 @@ class MasterList {
   }
 
   inline bool IsObservedOperation(mozilla::IOInterposeObserver::Operation aOp) {
-    // This does not occur inside of a lock, so this makes no guarantees that
-    // the observers are in sync with this. That is acceptable; it is not a
-    // problem if we occasionally report more or less IO than is actually
-    // occurring.
+    // The quick reader may observe that no locks are being employed here,
+    // hence the result of the operations is truly undefined. However, most
+    // computers will usually return either true or false, which is good enough.
+    // It is not a problem if we occasionally report more or less IO than is
+    // actually occurring.
     return mIsEnabled && !!(mObservedOperations & aOp);
   }
 
@@ -306,9 +307,7 @@ class MasterList {
   // during shutdown.
   mozilla::IOInterposer::Mutex mLock;
   // Flags tracking which operations are being observed
-  mozilla::Atomic<mozilla::IOInterposeObserver::Operation,
-                  mozilla::MemoryOrdering::Relaxed>
-      mObservedOperations;
+  mozilla::IOInterposeObserver::Operation mObservedOperations;
   // Used for quickly disabling everything by IOInterposer::Disable()
   mozilla::Atomic<bool> mIsEnabled;
   // Used to inform threads that the master observer list has changed