Bug 1521639 - Fix locking in TRRService. r=valentin a=lizzard
authorDragana Damjanovic <dd.mozilla@gmail.com>
Fri, 01 Feb 2019 20:46:00 +0000
changeset 515949 0fcc6713d0248efd3494e5a6a66ec77021c81552
parent 515948 d1f0fc61d27685ddb926e5a436512da3dfc68cd9
child 515950 0f8d776acfbf488e9c357b923f0b401cbc368713
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvalentin, lizzard
bugs1521639
milestone66.0
Bug 1521639 - Fix locking in TRRService. r=valentin a=lizzard Differential Revision: https://phabricator.services.mozilla.com/D17822
netwerk/dns/TRRService.cpp
netwerk/dns/TRRService.h
--- a/netwerk/dns/TRRService.cpp
+++ b/netwerk/dns/TRRService.cpp
@@ -297,31 +297,35 @@ TRRService::~TRRService() {
 NS_IMETHODIMP
 TRRService::Observe(nsISupports *aSubject, const char *aTopic,
                     const char16_t *aData) {
   MOZ_ASSERT(NS_IsMainThread(), "wrong thread");
   LOG(("TRR::Observe() topic=%s\n", aTopic));
   if (!strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) {
     ReadPrefs(NS_ConvertUTF16toUTF8(aData).get());
 
+    MutexAutoLock lock(mLock);
     if (((mConfirmationState == CONFIRM_INIT) && !mBootstrapAddr.IsEmpty() &&
          (mMode == MODE_TRRONLY)) ||
         (mConfirmationState == CONFIRM_FAILED)) {
       mConfirmationState = CONFIRM_TRYING;
-      MaybeConfirm();
+      MaybeConfirm_locked();
     }
 
   } else if (!strcmp(aTopic, kOpenCaptivePortalLoginEvent)) {
     // We are in a captive portal
     LOG(("TRRservice in captive portal\n"));
     mCaptiveIsPassed = false;
   } else if (!strcmp(aTopic, NS_CAPTIVE_PORTAL_CONNECTIVITY)) {
     nsAutoCString data = NS_ConvertUTF16toUTF8(aData);
     LOG(("TRRservice captive portal was %s\n", data.get()));
     if (!mTRRBLStorage) {
+     // We need a lock if we modify mTRRBLStorage variable because it is
+     // access off the main thread as well.
+     MutexAutoLock lock(mLock);
       mTRRBLStorage = DataStorage::Get(DataStorageClass::TRRBlacklist);
       if (mTRRBLStorage) {
         bool storageWillPersist = true;
         if (NS_FAILED(mTRRBLStorage->Init(storageWillPersist))) {
           mTRRBLStorage = nullptr;
         }
         if (mClearTRRBLStorage) {
           if (mTRRBLStorage) {
@@ -348,37 +352,39 @@ TRRService::Observe(nsISupports *aSubjec
     if (mTRRBLStorage) {
       mTRRBLStorage->Clear();
     }
   }
   return NS_OK;
 }
 
 void TRRService::MaybeConfirm() {
+  MutexAutoLock lock(mLock);
+  MaybeConfirm_locked();
+}
+
+void TRRService::MaybeConfirm_locked() {
+  mLock.AssertCurrentThreadOwns();
   if (TRR_DISABLED(mMode) || mConfirmer ||
       mConfirmationState != CONFIRM_TRYING) {
     LOG(
         ("TRRService:MaybeConfirm mode=%d, mConfirmer=%p "
          "mConfirmationState=%d\n",
          (int)mMode, (void *)mConfirmer, (int)mConfirmationState));
     return;
   }
-  nsAutoCString host;
-  {
-    MutexAutoLock lock(mLock);
-    host = mConfirmationNS;
-  }
-  if (host.Equals("skip")) {
+
+  if (mConfirmationNS.Equals("skip")) {
     LOG(("TRRService starting confirmation test %s SKIPPED\n",
          mPrivateURI.get()));
     mConfirmationState = CONFIRM_OK;
   } else {
     LOG(("TRRService starting confirmation test %s %s\n", mPrivateURI.get(),
-         host.get()));
-    mConfirmer = new TRR(this, host, TRRTYPE_NS, EmptyCString(), false);
+         mConfirmationNS.get()));
+    mConfirmer = new TRR(this, mConfirmationNS, TRRTYPE_NS, EmptyCString(), false);
     NS_DispatchToMainThread(mConfirmer);
   }
 }
 
 bool TRRService::MaybeBootstrap(const nsACString &aPossible,
                                 nsACString &aResult) {
   MutexAutoLock lock(mLock);
   if (TRR_DISABLED(mMode) || mBootstrapAddr.IsEmpty()) {
@@ -410,16 +416,19 @@ bool TRRService::MaybeBootstrap(const ns
 
 // When running in TRR-only mode, the blacklist is not used and it will also
 // try resolving the localhost / .local names.
 bool TRRService::IsTRRBlacklisted(const nsACString &aHost,
                                   const nsACString &aOriginSuffix,
                                   bool aPrivateBrowsing,
                                   bool aParentsToo)  // false if domain
 {
+  // Only use the Storage API on the main thread
+  MOZ_ASSERT(NS_IsMainThread(), "wrong thread");
+
   if (mMode == MODE_TRRONLY) {
     return false;  // might as well try
   }
 
   // hardcode these so as to not worry about expiration
   if (StringEndsWith(aHost, NS_LITERAL_CSTRING(".local")) ||
       aHost.Equals(NS_LITERAL_CSTRING("localhost"))) {
     return true;
@@ -427,19 +436,16 @@ bool TRRService::IsTRRBlacklisted(const 
 
   if (!Enabled()) {
     return true;
   }
   if (!mTRRBLStorage) {
     return false;
   }
 
-  // Only use the Storage API in the main thread
-  MOZ_ASSERT(NS_IsMainThread(), "wrong thread");
-
   if (mClearTRRBLStorage) {
     mTRRBLStorage->Clear();
     mClearTRRBLStorage = false;
     return false;  // just cleared!
   }
 
   int32_t dot = aHost.FindChar('.');
   if ((dot == kNotFound) && aParentsToo) {
@@ -454,43 +460,34 @@ bool TRRService::IsTRRBlacklisted(const 
 
     // recursively check the domain part of this name
     if (IsTRRBlacklisted(check, aOriginSuffix, aPrivateBrowsing, false)) {
       // the domain name of this name is already TRR blacklisted
       return true;
     }
   }
 
-  MutexAutoLock lock(mLock);
   // use a unified casing for the hashkey
   nsAutoCString hashkey(aHost + aOriginSuffix);
   nsCString val(mTRRBLStorage->Get(hashkey, aPrivateBrowsing
                                                 ? DataStorage_Private
                                                 : DataStorage_Persistent));
 
   if (!val.IsEmpty()) {
     nsresult code;
     int32_t until = val.ToInteger(&code) + mTRRBlacklistExpireTime;
     int32_t expire = NowInSeconds();
     if (NS_SUCCEEDED(code) && (until > expire)) {
       LOG(("Host [%s] is TRR blacklisted\n", nsCString(aHost).get()));
       return true;
     }
+
     // the blacklisted entry has expired
-    RefPtr<DataStorage> storage = mTRRBLStorage;
-    nsCOMPtr<nsIRunnable> runnable = NS_NewRunnableFunction(
-        "proxyStorageRemove", [storage, hashkey, aPrivateBrowsing]() {
-          storage->Remove(hashkey, aPrivateBrowsing ? DataStorage_Private
+    mTRRBLStorage->Remove(hashkey, aPrivateBrowsing ? DataStorage_Private
                                                     : DataStorage_Persistent);
-        });
-    if (!NS_IsMainThread()) {
-      NS_DispatchToMainThread(runnable);
-    } else {
-      runnable->Run();
-    }
   }
   return false;
 }
 
 class ProxyBlacklist : public Runnable {
  public:
   ProxyBlacklist(TRRService *service, const nsACString &aHost,
                  const nsACString &aOriginSuffix, bool pb, bool aParentsToo)
@@ -513,38 +510,40 @@ class ProxyBlacklist : public Runnable {
   nsCString mOriginSuffix;
   bool mPB;
   bool mParentsToo;
 };
 
 void TRRService::TRRBlacklist(const nsACString &aHost,
                               const nsACString &aOriginSuffix,
                               bool privateBrowsing, bool aParentsToo) {
-  if (!mTRRBLStorage) {
-    return;
+  {
+    MutexAutoLock lock(mLock);
+    if (!mTRRBLStorage) {
+      return;
+    }
   }
 
   if (!NS_IsMainThread()) {
     NS_DispatchToMainThread(new ProxyBlacklist(this, aHost, aOriginSuffix,
                                                privateBrowsing, aParentsToo));
     return;
   }
 
+  MOZ_ASSERT(NS_IsMainThread());
+
   LOG(("TRR blacklist %s\n", nsCString(aHost).get()));
   nsAutoCString hashkey(aHost + aOriginSuffix);
   nsAutoCString val;
   val.AppendInt(NowInSeconds());  // creation time
 
   // this overwrites any existing entry
-  {
-    MutexAutoLock lock(mLock);
-    mTRRBLStorage->Put(
-        hashkey, val,
-        privateBrowsing ? DataStorage_Private : DataStorage_Persistent);
-  }
+  mTRRBLStorage->Put(
+      hashkey, val,
+      privateBrowsing ? DataStorage_Private : DataStorage_Persistent);
 
   if (aParentsToo) {
     // when given a full host name, verify its domain as well
     int32_t dot = aHost.FindChar('.');
     if (dot != kNotFound) {
       // this has a domain to be checked
       dot++;
       nsDependentCSubstring domain =
@@ -577,16 +576,17 @@ TRRService::Notify(nsITimer *aTimer) {
   } else {
     MOZ_CRASH("Unknown timer");
   }
 
   return NS_OK;
 }
 
 void TRRService::TRRIsOkay(enum TrrOkay aReason) {
+  MOZ_ASSERT(NS_IsMainThread());
   Telemetry::AccumulateCategorical(
       aReason == OKAY_NORMAL ? Telemetry::LABELS_DNS_TRR_SUCCESS::Fine
                              : (aReason == OKAY_TIMEOUT
                                     ? Telemetry::LABELS_DNS_TRR_SUCCESS::Timeout
                                     : Telemetry::LABELS_DNS_TRR_SUCCESS::Bad));
   if (aReason == OKAY_NORMAL) {
     mTRRFailures = 0;
   } else if ((mMode == MODE_TRRFIRST) && (mConfirmationState == CONFIRM_OK)) {
@@ -609,23 +609,31 @@ AHostResolver::LookupStatus TRRService::
   // this is an NS check for the TRR blacklist or confirmationNS check
 
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(!rec);
 
   nsAutoPtr<AddrInfo> newRRSet(aNewRRSet);
   MOZ_ASSERT(newRRSet && newRRSet->IsTRR() == TRRTYPE_NS);
 
-  MOZ_ASSERT(!mConfirmer || (mConfirmationState == CONFIRM_TRYING));
+#ifdef DEBUG
+  {
+    MutexAutoLock lock(mLock);
+    MOZ_ASSERT(!mConfirmer || (mConfirmationState == CONFIRM_TRYING));
+  }
+#endif
   if (mConfirmationState == CONFIRM_TRYING) {
-    MOZ_ASSERT(mConfirmer);
-    mConfirmationState = NS_SUCCEEDED(status) ? CONFIRM_OK : CONFIRM_FAILED;
-    LOG(("TRRService finishing confirmation test %s %d %X\n", mPrivateURI.get(),
-         (int)mConfirmationState, (unsigned int)status));
-    mConfirmer = nullptr;
+    {
+      MutexAutoLock lock(mLock);
+      MOZ_ASSERT(mConfirmer);
+      mConfirmationState = NS_SUCCEEDED(status) ? CONFIRM_OK : CONFIRM_FAILED;
+      LOG(("TRRService finishing confirmation test %s %d %X\n", mPrivateURI.get(),
+           (int)mConfirmationState, (unsigned int)status));
+      mConfirmer = nullptr;
+    }
     if (mConfirmationState == CONFIRM_FAILED) {
       // retry failed NS confirmation
       NS_NewTimerWithCallback(getter_AddRefs(mRetryConfirmTimer), this,
                               mRetryConfirmInterval, nsITimer::TYPE_ONE_SHOT);
       if (mRetryConfirmInterval < 64000) {
         // double the interval up to this point
         mRetryConfirmInterval *= 2;
       }
--- a/netwerk/dns/TRRService.h
+++ b/netwerk/dns/TRRService.h
@@ -57,23 +57,25 @@ class TRRService : public nsIObserver,
   enum TrrOkay { OKAY_NORMAL = 0, OKAY_TIMEOUT = 1, OKAY_BAD = 2 };
   void TRRIsOkay(enum TrrOkay aReason);
 
  private:
   virtual ~TRRService();
   nsresult ReadPrefs(const char *name);
   void GetPrefBranch(nsIPrefBranch **result);
   void MaybeConfirm();
+  void MaybeConfirm_locked();
 
   bool mInitialized;
   Atomic<uint32_t, Relaxed> mMode;
   Atomic<uint32_t, Relaxed> mTRRBlacklistExpireTime;
   Atomic<uint32_t, Relaxed> mTRRTimeout;
 
-  Mutex mLock;             // protects mPrivate* string
+  Mutex mLock;
+
   nsCString mPrivateURI;   // main thread only
   nsCString mPrivateCred;  // main thread only
   nsCString mConfirmationNS;
   nsCString mBootstrapAddr;
 
   Atomic<bool, Relaxed> mWaitForCaptive;  // wait for the captive portal to say
                                           // OK before using TRR
   Atomic<bool, Relaxed>
@@ -83,16 +85,19 @@ class TRRService : public nsIObserver,
   Atomic<bool, Relaxed> mUseGET;  // do DOH using GET requests (instead of POST)
   Atomic<bool, Relaxed> mEarlyAAAA;  // allow use of AAAA results before A is in
   Atomic<bool, Relaxed> mDisableIPv6;  // don't even try
   Atomic<bool, Relaxed> mDisableECS;   // disable EDNS Client Subnet in requests
   Atomic<uint32_t, Relaxed>
       mDisableAfterFails;  // this many fails in a row means failed TRR service
 
   // TRR Blacklist storage
+  // mTRRBLStorage is only modified on the main thread, but we query whether it is initialized or not
+  // off the main thread as well. Therefore we need to lock while creating it and while accessing it
+  // off the main thread.
   RefPtr<DataStorage> mTRRBLStorage;
   Atomic<bool, Relaxed> mClearTRRBLStorage;
 
   enum ConfirmationState {
     CONFIRM_INIT = 0,
     CONFIRM_TRYING = 1,
     CONFIRM_OK = 2,
     CONFIRM_FAILED = 3