Bug 1521639 - Fix locking in TRRService. r=valentin
authorDragana Damjanovic <dd.mozilla@gmail.com>
Fri, 01 Feb 2019 20:46:00 +0000
changeset 458747 734770da29fd
parent 458746 74bd0fed9dbe
child 458748 c3fde50a85f1
push id35548
push useropoprus@mozilla.com
push dateWed, 13 Feb 2019 09:48:26 +0000
treeherdermozilla-central@93e37c529818 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvalentin
bugs1521639
milestone67.0a1
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
Bug 1521639 - Fix locking in TRRService. r=valentin 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