Bug 1513519 - Always hold lock when accessing mResolver r=necko-reviewers,dragana

Differential Revision: https://phabricator.services.mozilla.com/D106913

netwerk/dns/nsDNSService2.cpp

@@ -523,6 +523,7 @@ size_t nsDNSAsyncRequest::SizeOfIncludingThis(MallocSizeOf mallocSizeOf) const {
 NS_IMETHODIMP
 nsDNSAsyncRequest::Cancel(nsresult reason) {
   NS_ENSURE_ARG(NS_FAILED(reason));
+  MOZ_DIAGNOSTIC_ASSERT(mResolver, "mResolver should not be null");
   mResolver->DetachCallback(mHost, mTrrServer, mType, mOriginAttributes, mFlags,
                             mAF, this, reason);
   return NS_OK;
@@ -833,10 +834,10 @@ nsDNSService::Shutdown() {
   RefPtr<nsHostResolver> res;
   {
     MutexAutoLock lock(mLock);
-    res = mResolver;
+    res = std::move(mResolver);
-    mResolver = nullptr;
   }
   if (res) {
+    // Shutdown outside lock.
     res->Shutdown();
   }
 
@@ -894,6 +895,11 @@ bool nsDNSService::DNSForbiddenByActiveProxy(const nsACString& aHostname,
   return false;
 }
 
+already_AddRefed<nsHostResolver> nsDNSService::GetResolverLocked() {
+  MutexAutoLock lock(mLock);
+  return do_AddRef(mResolver);
+}
+
 nsresult nsDNSService::PreprocessHostname(bool aLocalDomain,
                                           const nsACString& aInput,
                                           nsIIDNService* aIDN,
@@ -1258,19 +1264,21 @@ NS_IMETHODIMP
 nsDNSService::Observe(nsISupports* subject, const char* topic,
                       const char16_t* data) {
   bool flushCache = false;
+  RefPtr<nsHostResolver> resolver = GetResolverLocked();
+
   if (!strcmp(topic, NS_NETWORK_LINK_TOPIC)) {
     nsAutoCString converted = NS_ConvertUTF16toUTF8(data);
-    if (mResolver && !strcmp(converted.get(), NS_NETWORK_LINK_DATA_CHANGED)) {
+    if (!strcmp(converted.get(), NS_NETWORK_LINK_DATA_CHANGED)) {
       flushCache = true;
     }
   } else if (!strcmp(topic, "last-pb-context-exited")) {
     flushCache = true;
   } else if (!strcmp(topic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) {
     ReadPrefs(NS_ConvertUTF16toUTF8(data).get());
-    NS_ENSURE_TRUE(mResolver, NS_ERROR_NOT_INITIALIZED);
+    NS_ENSURE_TRUE(resolver, NS_ERROR_NOT_INITIALIZED);
-    if (mResolverPrefsUpdated && mResolver) {
+    if (mResolverPrefsUpdated && resolver) {
-      mResolver->SetCacheLimits(mResCacheEntries, mResCacheExpiration,
+      resolver->SetCacheLimits(mResCacheEntries, mResCacheExpiration,
-                                mResCacheGrace);
+                               mResCacheGrace);
     }
   } else if (!strcmp(topic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
     Shutdown();
@@ -1278,8 +1286,8 @@ nsDNSService::Observe(nsISupports* subject, const char* topic,
     mODoHActivated = u"true"_ns.Equals(data);
   }
 
-  if (flushCache && mResolver) {
+  if (flushCache && resolver) {
-    mResolver->FlushCache(false);
+    resolver->FlushCache(false);
     return NS_OK;
   }
 
@@ -1349,15 +1357,17 @@ uint16_t nsDNSService::GetAFForLookup(const nsACString& host, uint32_t flags) {
 NS_IMETHODIMP
 nsDNSService::GetDNSCacheEntries(
     nsTArray<mozilla::net::DNSCacheEntries>* args) {
-  NS_ENSURE_TRUE(mResolver, NS_ERROR_NOT_INITIALIZED);
+  RefPtr<nsHostResolver> resolver = GetResolverLocked();
-  mResolver->GetDNSCacheEntries(args);
+  NS_ENSURE_TRUE(resolver, NS_ERROR_NOT_INITIALIZED);
+  resolver->GetDNSCacheEntries(args);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDNSService::ClearCache(bool aTrrToo) {
-  NS_ENSURE_TRUE(mResolver, NS_ERROR_NOT_INITIALIZED);
+  RefPtr<nsHostResolver> resolver = GetResolverLocked();
-  mResolver->FlushCache(aTrrToo);
+  NS_ENSURE_TRUE(resolver, NS_ERROR_NOT_INITIALIZED);
+  resolver->FlushCache(aTrrToo);
   return NS_OK;
 }
 

netwerk/dns/nsDNSService2.h

@@ -77,6 +77,9 @@ class nsDNSService final : public nsPIDNSService,
 
   bool DNSForbiddenByActiveProxy(const nsACString& aHostname, uint32_t flags);
 
+  // Locks the mutex and returns an addreffed resolver. May return null.
+  already_AddRefed<nsHostResolver> GetResolverLocked();
+
   RefPtr<nsHostResolver> mResolver;
   nsCOMPtr<nsIIDNService> mIDN;