Backed out 2 changesets (bug 1670982) for tsan xpc shell failures CLOSED TREE

Backed out changeset 3ce1e8c65cbc (bug 1670982)
Backed out changeset e7211a44d968 (bug 1670982)

mozglue/build/TsanOptions.cpp

@@ -196,6 +196,11 @@ extern "C" const char* __tsan_default_suppressions() {
          // These should all still be fixed because the compiler is incentivized
          // to combine/cache these accesses without proper atomic annotations.
 
+         // No Bug
+         "race:WalkDiskCacheRunnable::Run\n"
+         // No Bug - Modifying `mResolveAgain` while reading `mGetTtl`
+         "race:RemoveOrRefresh\n"
+         "race:nsHostResolver::ThreadFunc\n"
          // Bug 1614697
          "race:nsHttpChannel::OnCacheEntryCheck\n"
          "race:~AutoCacheWaitFlags\n"

netwerk/cache2/CacheStorageService.cpp

@@ -29,7 +29,6 @@
 #include "nsNetUtil.h"
 #include "nsServiceManagerUtils.h"
 #include "nsXULAppAPI.h"
-#include "mozilla/AtomicBitfields.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/DebugOnly.h"
 #include "mozilla/Services.h"
@@ -203,10 +202,10 @@ class WalkCacheRunnable : public Runnable,
         mService(CacheStorageService::Self()),
         mCallback(aVisitor),
         mSize(0),
+        mNotifyStorage(true),
+        mVisitEntries(aVisitEntries),
         mCancel(false) {
     MOZ_ASSERT(NS_IsMainThread());
-    SetNotifyStorage(true);
-    SetVisitEntries(aVisitEntries);
   }
 
   virtual ~WalkCacheRunnable() {
@@ -220,12 +219,8 @@ class WalkCacheRunnable : public Runnable,
 
   uint64_t mSize;
 
-  // clang-format off
-  MOZ_ATOMIC_BITFIELDS(mAtomicBitfields, 8, (
-    (bool, NotifyStorage, 1),
-    (bool, VisitEntries, 1)
-  ))
-  // clang-format on
+  bool mNotifyStorage : 1;
+  bool mVisitEntries : 1;
 
   Atomic<bool> mCancel;
 };
@@ -280,7 +275,7 @@ class WalkMemoryCacheRunnable : public WalkCacheRunnable {
     } else if (NS_IsMainThread()) {
       LOG(("WalkMemoryCacheRunnable::Run - notifying [this=%p]", this));
 
-      if (GetNotifyStorage()) {
+      if (mNotifyStorage) {
         LOG(("  storage"));
 
         uint64_t capacity = CacheObserver::MemoryCacheCapacity();
@@ -289,9 +284,9 @@ class WalkMemoryCacheRunnable : public WalkCacheRunnable {
         // Second, notify overall storage info
         mCallback->OnCacheStorageInfo(mEntryArray.Length(), mSize, capacity,
                                       nullptr);
-        if (!GetVisitEntries()) return NS_OK;  // done
+        if (!mVisitEntries) return NS_OK;  // done
 
-        SetNotifyStorage(false);
+        mNotifyStorage = false;
 
       } else {
         LOG(("  entry [left=%zu, canceled=%d]", mEntryArray.Length(),
@@ -436,7 +431,7 @@ class WalkDiskCacheRunnable : public WalkCacheRunnable {
           uint32_t size;
           rv = CacheIndex::GetCacheStats(mLoadInfo, &size, &mCount);
           if (NS_FAILED(rv)) {
-            if (GetVisitEntries()) {
+            if (mVisitEntries) {
               // both onStorageInfo and onCompleted are expected
               NS_DispatchToMainThread(this);
             }
@@ -448,7 +443,7 @@ class WalkDiskCacheRunnable : public WalkCacheRunnable {
           // Invoke onCacheStorageInfo with valid information.
           NS_DispatchToMainThread(this);
 
-          if (!GetVisitEntries()) {
+          if (!mVisitEntries) {
             return NS_OK;  // done
           }
 
@@ -482,13 +477,13 @@ class WalkDiskCacheRunnable : public WalkCacheRunnable {
           NS_DispatchToMainThread(this);
       }
     } else if (NS_IsMainThread()) {
-      if (GetNotifyStorage()) {
+      if (mNotifyStorage) {
         nsCOMPtr<nsIFile> dir;
         CacheFileIOManager::GetCacheDirectory(getter_AddRefs(dir));
         uint64_t capacity = CacheObserver::DiskCacheCapacity();
         capacity <<= 10;  // kilobytes to bytes
         mCallback->OnCacheStorageInfo(mCount, mSize, capacity, dir);
-        SetNotifyStorage(false);
+        mNotifyStorage = false;
       } else {
         mCallback->OnCacheEntryVisitCompleted();
       }

netwerk/dns/nsHostResolver.cpp

@@ -295,7 +295,12 @@ AddrHostRecord::AddrHostRecord(const nsHostKey& key)
       addr(nullptr),
       mTRRUsed(false),
       mTRRSuccess(0),
-      mNativeSuccess(0) {}
+      mNativeSuccess(0),
+      mNative(false),
+      mNativeUsed(false),
+      usingAnyThread(false),
+      mGetTtl(false),
+      mResolveAgain(false) {}
 
 AddrHostRecord::~AddrHostRecord() {
   mCallbacks.clear();
@@ -375,21 +380,21 @@ bool AddrHostRecord::HasUsableResultInternal() const {
 }
 
 // Returns true if the entry can be removed, or false if it should be left.
-// Sets ResolveAgain true for entries being resolved right now.
+// Sets mResolveAgain true for entries being resolved right now.
 bool AddrHostRecord::RemoveOrRefresh(bool aTrrToo) {
   // no need to flush TRRed names, they're not resolved "locally"
   MutexAutoLock lock(addr_info_lock);
   if (addr_info && !aTrrToo && addr_info->IsTRR()) {
     return false;
   }
-  if (GetNative()) {
+  if (mNative) {
     if (!onQueue()) {
       // The request has been passed to the OS resolver. The resultant DNS
       // record should be considered stale and not trusted; set a flag to
       // ensure it is called again.
-      SetResolveAgain(true);
+      mResolveAgain = true;
     }
-    // if onQueue is true, the host entry is already added to the cache
+    // if Onqueue is true, the host entry is already added to the cache
     // but is still pending to get resolved: just leave it in hash.
     return false;
   }
@@ -398,7 +403,7 @@ bool AddrHostRecord::RemoveOrRefresh(bool aTrrToo) {
 }
 
 void AddrHostRecord::ResolveComplete() {
-  if (GetNativeUsed()) {
+  if (mNativeUsed) {
     if (mNativeSuccess) {
       uint32_t millis = static_cast<uint32_t>(mNativeDuration.ToMilliseconds());
       Telemetry::Accumulate(Telemetry::DNS_NATIVE_LOOKUP_TIME, millis);
@@ -768,7 +773,7 @@ void nsHostResolver::ClearPendingQueue(
 //
 // This function removes all existing resolved host entries from the hash.
 // Names that are in the pending queues can be left there. Entries in the
-// cache that have 'Resolve' set true but not 'OnQueue' are being resolved
+// cache that have 'Resolve' set true but not 'onQueue' are being resolved
 // right now, so we need to mark them to get re-resolved on completion!
 
 void nsHostResolver::FlushCache(bool aTrrToo) {
@@ -1447,8 +1452,8 @@ nsresult nsHostResolver::NativeLookup(nsHostRecord* aRec) {
   }
   mPendingCount++;
 
-  addrRec->SetNative(true);
-  addrRec->SetNativeUsed(true);
+  addrRec->mNative = true;
+  addrRec->mNativeUsed = true;
   addrRec->mResolving++;
 
   nsresult rv = ConditionallyCreateThread(rec);
@@ -1566,7 +1571,7 @@ nsresult nsHostResolver::NameLookup(nsHostRecord* rec) {
     RefPtr<AddrHostRecord> addrRec = do_QueryObject(rec);
     MOZ_ASSERT(addrRec);
 
-    addrRec->SetNativeUsed(false);
+    addrRec->mNativeUsed = false;
     addrRec->mTRRUsed = false;
     addrRec->mNativeSuccess = false;
   }
@@ -1662,7 +1667,7 @@ bool nsHostResolver::GetHostToLookup(AddrHostRecord** result) {
     // remove next record from Q; hand over owning reference. Check high, then
     // med, then low
 
-#define SET_GET_TTL(var, val) (var)->SetGetTtl(sGetTtlEnabled && (val))
+#define SET_GET_TTL(var, val) (var)->mGetTtl = sGetTtlEnabled && (val)
 
     if (!mHighQ.isEmpty()) {
       DeQueue(mHighQ, result);
@@ -1674,7 +1679,7 @@ bool nsHostResolver::GetHostToLookup(AddrHostRecord** result) {
       if (!mMediumQ.isEmpty()) {
         DeQueue(mMediumQ, result);
         mActiveAnyThreadCount++;
-        (*result)->SetUsingAnyThread(true);
+        (*result)->usingAnyThread = true;
         SET_GET_TTL(*result, true);
         return true;
       }
@@ -1682,7 +1687,7 @@ bool nsHostResolver::GetHostToLookup(AddrHostRecord** result) {
       if (!mLowQ.isEmpty()) {
         DeQueue(mLowQ, result);
         mActiveAnyThreadCount++;
-        (*result)->SetUsingAnyThread(true);
+        (*result)->usingAnyThread = true;
         SET_GET_TTL(*result, true);
         return true;
       }
@@ -1847,10 +1852,10 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookup(
     newRRSet = nullptr;
   }
 
-  if (addrRec->GetResolveAgain() && (status != NS_ERROR_ABORT) && !trrResult) {
+  if (addrRec->mResolveAgain && (status != NS_ERROR_ABORT) && !trrResult) {
     LOG(("nsHostResolver record %p resolve again due to flushcache\n",
          addrRec.get()));
-    addrRec->SetResolveAgain(false);
+    addrRec->mResolveAgain = false;
     return LOOKUP_RESOLVEAGAIN;
   }
 
@@ -1902,9 +1907,9 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookup(
       status = NS_ERROR_UNKNOWN_HOST;
     }
   } else {  // native resolve completed
-    if (addrRec->GetUsingAnyThread()) {
+    if (addrRec->usingAnyThread) {
       mActiveAnyThreadCount--;
-      addrRec->SetUsingAnyThread(false);
+      addrRec->usingAnyThread = false;
     }
 
     addrRec->mNativeSuccess = static_cast<bool>(newRRSet);
@@ -1914,7 +1919,7 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookup(
   }
 
   // This should always be cleared when a request is completed.
-  addrRec->SetNative(false);
+  addrRec->mNative = false;
 
   // update record fields.  We might have a addrRec->addr_info already if a
   // previous lookup result expired and we're reresolving it or we get
@@ -1992,8 +1997,8 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookup(
       hasNativeResult = true;
     }
   }
-  if (hasNativeResult && !mShutdown && !addrRec->GetGetTtl() &&
-      !rec->mResolving && sGetTtlEnabled) {
+  if (hasNativeResult && !mShutdown && !addrRec->mGetTtl && !rec->mResolving &&
+      sGetTtlEnabled) {
     LOG(("Issuing second async lookup for TTL for host [%s].",
          addrRec->host.get()));
     addrRec->flags = (addrRec->flags & ~RES_PRIORITY_MEDIUM) | RES_PRIORITY_LOW;
@@ -2153,7 +2158,7 @@ void nsHostResolver::ThreadFunc() {
           rec->host.get()));
 
     TimeStamp startTime = TimeStamp::Now();
-    bool getTtl = rec->GetGetTtl();
+    bool getTtl = rec->mGetTtl;
     TimeDuration inQueue = startTime - rec->mNativeStart;
     uint32_t ms = static_cast<uint32_t>(inQueue.ToMilliseconds());
     Telemetry::Accumulate(Telemetry::DNS_NATIVE_QUEUING, ms);

netwerk/dns/nsHostResolver.h

@@ -19,7 +19,6 @@
 #include "GetAddrInfo.h"
 #include "mozilla/net/DNS.h"
 #include "mozilla/net/DashboardTypes.h"
-#include "mozilla/AtomicBitfields.h"
 #include "mozilla/Atomics.h"
 #include "mozilla/LinkedList.h"
 #include "mozilla/TimeStamp.h"
@@ -283,7 +282,7 @@ class AddrHostRecord final : public nsHostRecord {
   static DnsPriority GetPriority(uint16_t aFlags);
 
   // true if pending and on the queue (not yet given to getaddrinfo())
-  bool onQueue() { return GetNative() && isInList(); }
+  bool onQueue() { return mNative && isInList(); }
 
   // When the lookups of this record started and their durations
   mozilla::TimeStamp mTrrStart;
@@ -295,18 +294,17 @@ class AddrHostRecord final : public nsHostRecord {
   uint8_t mTRRSuccess;             // number of successful TRR responses
   uint8_t mNativeSuccess;          // number of native lookup responses
 
-  // clang-format off
-  MOZ_ATOMIC_BITFIELDS(mAtomicBitfields, 8, (
-    // true if this record is being resolved "natively", which means that
-    // it is either on the pending queue or owned by one of the worker threads.
-    (uint16_t, Native, 1),
-    (uint16_t, NativeUsed, 1),
-    // true if off queue and contributing to mActiveAnyThreadCount
-    (uint16_t, UsingAnyThread, 1),
-    (uint16_t, GetTtl, 1),
-    (uint16_t, ResolveAgain, 1)
-  ))
-  // clang-format on
+  uint16_t mNative : 1;  // true if this record is being resolved "natively",
+                         // which means that it is either on the pending queue
+                         // or owned by one of the worker threads. */
+  uint16_t mNativeUsed : 1;
+  uint16_t usingAnyThread : 1;  // true if off queue and contributing to
+                                // mActiveAnyThreadCount
+  uint16_t mGetTtl : 1;
+
+  // when the results from this resolve is returned, it is not to be
+  // trusted, but instead a new resolve must be made!
+  uint16_t mResolveAgain : 1;
 
   // The number of times ReportUnusable() has been called in the record's
   // lifetime.