Bug 1420677 - Make AddrHostRecord.addr_info a RefPtr r=dragana
authorValentin Gosu <valentin.gosu@gmail.com>
Mon, 11 Mar 2019 12:59:55 +0000
changeset 521345 cf114035c79fa00872568b863ef84c24ad8e7de2
parent 521344 edff1f39d426d7896dd9f48b06ada1e28fe820bb
child 521346 dbd1b0222c6f9f2f43a4bed4f0c8d4f262d23b45
push id10866
push usernerli@mozilla.com
push dateTue, 12 Mar 2019 18:59:09 +0000
treeherdermozilla-beta@445c24a51727 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdragana
bugs1420677
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 1420677 - Make AddrHostRecord.addr_info a RefPtr r=dragana Depends on D22960 Differential Revision: https://phabricator.services.mozilla.com/D22961
netwerk/dns/DNS.h
netwerk/dns/GetAddrInfo.cpp
netwerk/dns/TRR.cpp
netwerk/dns/TRRService.cpp
netwerk/dns/nsHostResolver.cpp
netwerk/dns/nsHostResolver.h
--- a/netwerk/dns/DNS.h
+++ b/netwerk/dns/DNS.h
@@ -7,16 +7,17 @@
 #ifndef DNS_h_
 #define DNS_h_
 
 #include "nscore.h"
 #include "nsString.h"
 #include "prio.h"
 #include "prnetdb.h"
 #include "plstr.h"
+#include "nsISupportsImpl.h"
 #include "mozilla/LinkedList.h"
 #include "mozilla/MemoryReporting.h"
 
 #if !defined(XP_WIN)
 #  include <arpa/inet.h>
 #endif
 
 #ifdef XP_WIN
@@ -118,46 +119,48 @@ class NetAddrElement : public LinkedList
   explicit NetAddrElement(const PRNetAddr *prNetAddr);
   NetAddrElement(const NetAddrElement &netAddr);
   ~NetAddrElement();
 
   NetAddr mAddress;
 };
 
 class AddrInfo {
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AddrInfo)
+
  public:
   // Creates an AddrInfo object.
   explicit AddrInfo(const nsACString &host, const PRAddrInfo *prAddrInfo,
                     bool disableIPv4, bool filterNameCollision,
                     const nsACString &cname);
 
   // Creates a basic AddrInfo object (initialize only the host, cname and TRR
   // type).
   explicit AddrInfo(const nsACString &host, const nsACString &cname,
                     unsigned int TRRType);
 
   // Creates a basic AddrInfo object (initialize only the host and TRR status).
   explicit AddrInfo(const nsACString &host, unsigned int TRRType);
-  ~AddrInfo();
 
   explicit AddrInfo(const AddrInfo *src);  // copy
 
   void AddAddress(NetAddrElement *address);
 
   size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const;
 
   nsCString mHostName;
   nsCString mCanonicalName;
   uint32_t ttl;
   static const uint32_t NO_TTL_DATA = (uint32_t)-1;
 
   AutoCleanLinkedList<NetAddrElement> mAddresses;
   unsigned int IsTRR() { return mFromTRR; }
 
  private:
+  ~AddrInfo();
   unsigned int mFromTRR;
 };
 
 // Copies the contents of a PRNetAddr to a NetAddr.
 // Does not do a ptr safety check!
 void PRNetAddrToNetAddr(const PRNetAddr *prAddr, NetAddr *addr);
 
 // Copies the contents of a NetAddr to a PRNetAddr.
--- a/netwerk/dns/GetAddrInfo.cpp
+++ b/netwerk/dns/GetAddrInfo.cpp
@@ -272,24 +272,24 @@ static MOZ_ALWAYS_INLINE nsresult
 
   nsAutoCString canonName;
   if (aFlags & nsHostResolver::RES_CANON_NAME) {
     canonName.Assign(PR_GetCanonNameFromAddrInfo(prai));
   }
 
   bool filterNameCollision =
       !(aFlags & nsHostResolver::RES_ALLOW_NAME_COLLISION);
-  nsAutoPtr<AddrInfo> ai(new AddrInfo(aCanonHost, prai, disableIPv4,
+  RefPtr<AddrInfo> ai(new AddrInfo(aCanonHost, prai, disableIPv4,
                                       filterNameCollision, canonName));
   PR_FreeAddrInfo(prai);
   if (ai->mAddresses.isEmpty()) {
     return NS_ERROR_UNKNOWN_HOST;
   }
 
-  *aAddrInfo = ai.forget();
+  ai.forget(aAddrInfo);
 
   return NS_OK;
 }
 
 //////////////////////////////////////
 // COMMON/PLATFORM INDEPENDENT CODE //
 //////////////////////////////////////
 nsresult GetAddrInfoInit() {
--- a/netwerk/dns/TRR.cpp
+++ b/netwerk/dns/TRR.cpp
@@ -871,17 +871,17 @@ nsresult TRR::DohDecode(nsCString &aHost
     return NS_ERROR_FAILURE;
   }
   return NS_OK;
 }
 
 nsresult TRR::ReturnData() {
   if (mType != TRRTYPE_TXT) {
     // create and populate an AddrInfo instance to pass on
-    nsAutoPtr<AddrInfo> ai(new AddrInfo(mHost, mType));
+    RefPtr<AddrInfo> ai(new AddrInfo(mHost, mType));
     DOHaddr *item;
     uint32_t ttl = AddrInfo::NO_TTL_DATA;
     while ((item = static_cast<DOHaddr *>(mDNS.mAddresses.popFirst()))) {
       PRNetAddr prAddr;
       NetAddrToPRNetAddr(&item->mNet, &prAddr);
       auto *addrElement = new NetAddrElement(&prAddr);
       ai->AddAddress(addrElement);
       if (item->mTtl < ttl) {
@@ -890,17 +890,17 @@ nsresult TRR::ReturnData() {
         // lowest number.
         ttl = item->mTtl;
       }
     }
     ai->ttl = ttl;
     if (!mHostResolver) {
       return NS_ERROR_FAILURE;
     }
-    (void)mHostResolver->CompleteLookup(mRec, NS_OK, ai.forget(), mPB,
+    (void)mHostResolver->CompleteLookup(mRec, NS_OK, ai, mPB,
                                         mOriginSuffix);
     mHostResolver = nullptr;
     mRec = nullptr;
   } else {
     (void)mHostResolver->CompleteLookupByType(mRec, NS_OK, &mTxt, mTxtTtl, mPB);
   }
   return NS_OK;
 }
@@ -910,17 +910,17 @@ nsresult TRR::FailData(nsresult error) {
     return NS_ERROR_FAILURE;
   }
 
   if (mType == TRRTYPE_TXT) {
     (void)mHostResolver->CompleteLookupByType(mRec, error, nullptr, 0, mPB);
   } else {
     // create and populate an TRR AddrInfo instance to pass on to signal that
     // this comes from TRR
-    AddrInfo *ai = new AddrInfo(mHost, mType);
+    RefPtr<AddrInfo> ai = new AddrInfo(mHost, mType);
 
     (void)mHostResolver->CompleteLookup(mRec, error, ai, mPB, mOriginSuffix);
   }
 
   mHostResolver = nullptr;
   mRec = nullptr;
   return NS_OK;
 }
--- a/netwerk/dns/TRRService.cpp
+++ b/netwerk/dns/TRRService.cpp
@@ -607,17 +607,17 @@ void TRRService::TRRIsOkay(enum TrrOkay 
 AHostResolver::LookupStatus TRRService::CompleteLookup(
     nsHostRecord *rec, nsresult status, AddrInfo *aNewRRSet, bool pb,
     const nsACString &aOriginSuffix) {
   // this is an NS check for the TRR blacklist or confirmationNS check
 
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(!rec);
 
-  nsAutoPtr<AddrInfo> newRRSet(aNewRRSet);
+  RefPtr<AddrInfo> newRRSet(aNewRRSet);
   MOZ_ASSERT(newRRSet && newRRSet->IsTRR() == TRRTYPE_NS);
 
 #ifdef DEBUG
   {
     MutexAutoLock lock(mLock);
     MOZ_ASSERT(!mConfirmer || (mConfirmationState == CONFIRM_TRYING));
   }
 #endif
--- a/netwerk/dns/nsHostResolver.cpp
+++ b/netwerk/dns/nsHostResolver.cpp
@@ -292,17 +292,16 @@ AddrHostRecord::AddrHostRecord(const nsH
       mTrrAUsed(INIT),
       mTrrAAAAUsed(INIT),
       mTrrLock("AddrHostRecord.mTrrLock"),
       mBlacklistedCount(0) {}
 
 AddrHostRecord::~AddrHostRecord() {
   mCallbacks.clear();
   Telemetry::Accumulate(Telemetry::DNS_BLACKLIST_COUNT, mBlacklistedCount);
-  delete addr_info;
 }
 
 bool AddrHostRecord::Blacklisted(NetAddr *aQuery) {
   // must call locked
   LOG(("Checking blacklist for host [%s], host record [%p].\n", host.get(),
        this));
 
   // skip the string conversion for the common case of no blacklist
@@ -1004,19 +1003,16 @@ nsresult nsHostResolver::ResolveHost(con
 
             LOG(("  Trying AF_UNSPEC entry for host [%s] af: %s.\n", host.get(),
                  (af == PR_AF_INET) ? "AF_INET" : "AF_INET6"));
 
             // We need to lock in case any other thread is reading
             // addr_info.
             MutexAutoLock lock(addrRec->addr_info_lock);
 
-            // XXX: note that this actually leaks addr_info.
-            // For some reason, freeing the memory causes a crash in
-            // nsDNSRecord::GetNextAddr - see bug 1422173
             addrRec->addr_info = nullptr;
             if (unspecRec->negative) {
               rec->negative = unspecRec->negative;
               rec->CopyExpirationTimesAndFlagsFrom(unspecRec);
             } else if (addrUnspecRec->addr_info) {
               // Search for any valid address in the AF_UNSPEC entry
               // in the cache (not blacklisted and from the right
               // family).
@@ -1686,19 +1682,17 @@ nsHostResolver::LookupStatus nsHostResol
   MutexAutoLock lock(mLock);
   MOZ_ASSERT(rec);
   MOZ_ASSERT(rec->pb == pb);
   MOZ_ASSERT(rec->IsAddrRecord());
 
   RefPtr<AddrHostRecord> addrRec = do_QueryObject(rec);
   MOZ_ASSERT(addrRec);
 
-  // newRRSet needs to be taken into the hostrecord (which will then own it)
-  // or deleted on early return.
-  nsAutoPtr<AddrInfo> newRRSet(aNewRRSet);
+  RefPtr<AddrInfo> newRRSet(aNewRRSet);
 
   bool trrResult = newRRSet && newRRSet->IsTRR();
 
   if (addrRec->mResolveAgain && (status != NS_ERROR_ABORT) && !trrResult) {
     LOG(("nsHostResolver record %p resolve again due to flushcache\n",
          addrRec.get()));
     addrRec->mResolveAgain = false;
     return LOOKUP_RESOLVEAGAIN;
@@ -1743,17 +1737,17 @@ nsHostResolver::LookupStatus nsHostResol
       addrRec->mFirstTRRresult = status;
       if (NS_FAILED(status)) {
         return LOOKUP_OK;  // wait for outstanding
       }
 
       // There's another TRR complete pending. Wait for it and keep
       // this RRset around until then.
       MOZ_ASSERT(!addrRec->mFirstTRR && newRRSet);
-      addrRec->mFirstTRR = newRRSet;  // autoPtr.swap()
+      addrRec->mFirstTRR.swap(newRRSet);  // autoPtr.swap()
       MOZ_ASSERT(addrRec->mFirstTRR && !newRRSet);
 
       if (addrRec->mDidCallbacks || addrRec->mResolverMode == MODE_SHADOW) {
         return LOOKUP_OK;
       }
 
       if (addrRec->mTrrA && (!gTRRService || !gTRRService->EarlyAAAA())) {
         // This is an early AAAA with a pending A response. Allowed
@@ -1769,17 +1763,17 @@ nsHostResolver::LookupStatus nsHostResol
 
     } else {
       // no more outstanding TRRs
       // If mFirstTRR is set, merge those addresses into current set!
       if (addrRec->mFirstTRR) {
         if (NS_SUCCEEDED(status)) {
           merge_rrset(newRRSet, addrRec->mFirstTRR);
         } else {
-          newRRSet = addrRec->mFirstTRR;  // transfers
+          newRRSet.swap(addrRec->mFirstTRR);  // transfers
         }
         addrRec->mFirstTRR = nullptr;
       }
 
       if (NS_FAILED(addrRec->mFirstTRRresult) && NS_FAILED(status) &&
           (addrRec->mFirstTRRresult != NS_ERROR_UNKNOWN_HOST) &&
           (status != NS_ERROR_UNKNOWN_HOST)) {
         // the errors are not failed resolves, that means
@@ -1818,17 +1812,17 @@ nsHostResolver::LookupStatus nsHostResol
   }
 
   // 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
   // a late second TRR response.
   // note that we don't update the addr_info if this is trr shadow results
   if (!mShutdown && !(trrResult && addrRec->mResolverMode == MODE_SHADOW)) {
     MutexAutoLock lock(addrRec->addr_info_lock);
-    nsAutoPtr<AddrInfo> old_addr_info;
+    RefPtr<AddrInfo> old_addr_info;
     if (different_rrset(addrRec->addr_info, newRRSet)) {
       LOG(("nsHostResolver record %p new gencnt\n", addrRec.get()));
       old_addr_info = addrRec->addr_info;
       addrRec->addr_info = newRRSet.forget();
       addrRec->addr_info_gencnt++;
     } else {
       if (addrRec->addr_info && newRRSet) {
         addrRec->addr_info->ttl = newRRSet->ttl;
@@ -2033,17 +2027,17 @@ size_t nsHostResolver::SizeOfIncludingTh
 
 void nsHostResolver::ThreadFunc() {
   LOG(("DNS lookup thread - starting execution.\n"));
 
 #if defined(RES_RETRY_ON_FAILURE)
   nsResState rs;
 #endif
   RefPtr<AddrHostRecord> rec;
-  AddrInfo *ai = nullptr;
+  RefPtr<AddrInfo> ai;
 
   do {
     if (!rec) {
       RefPtr<AddrHostRecord> tmpRec;
       if (!GetHostToLookup(getter_AddRefs(tmpRec))) {
         break;  // thread shutdown signal
       }
       // GetHostToLookup() returns an owning reference
@@ -2054,20 +2048,20 @@ void nsHostResolver::ThreadFunc() {
     LOG1(("DNS lookup thread - Calling getaddrinfo for host [%s].\n",
           rec->host.get()));
 
     TimeStamp startTime = TimeStamp::Now();
     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);
-    nsresult status = GetAddrInfo(rec->host, rec->af, rec->flags, &ai, getTtl);
+    nsresult status = GetAddrInfo(rec->host, rec->af, rec->flags, getter_AddRefs(ai), getTtl);
 #if defined(RES_RETRY_ON_FAILURE)
     if (NS_FAILED(status) && rs.Reset()) {
-      status = GetAddrInfo(rec->host, rec->af, rec->flags, &ai, getTtl);
+      status = GetAddrInfo(rec->host, rec->af, rec->flags, getter_AddRefs(ai), getTtl);
     }
 #endif
 
     {  // obtain lock to check shutdown and manage inter-module telemetry
       MutexAutoLock lock(mLock);
 
       if (!mShutdown) {
         TimeDuration elapsed = TimeStamp::Now() - startTime;
--- a/netwerk/dns/nsHostResolver.h
+++ b/netwerk/dns/nsHostResolver.h
@@ -176,17 +176,17 @@ class AddrHostRecord final : public nsHo
    * nsDNSService2 class.  |addr| doesn't change after it has been
    * assigned a value.  only the resolver worker thread modifies
    * nsHostRecord (and only in nsHostResolver::CompleteLookup);
    * the other threads just read it.  therefore the resolver worker
    * thread doesn't need to lock when reading |addr_info|.
    */
   Mutex addr_info_lock;
   int addr_info_gencnt; /* generation count of |addr_info| */
-  mozilla::net::AddrInfo *addr_info;
+  RefPtr<mozilla::net::AddrInfo> addr_info;
   mozilla::UniquePtr<mozilla::net::NetAddr> addr;
 
   // hold addr_info_lock when calling the blacklist functions
   bool Blacklisted(mozilla::net::NetAddr *query);
   void ResetBlacklist();
   void ReportUnusable(mozilla::net::NetAddr *addr);
 
   size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const override;
@@ -217,17 +217,17 @@ class AddrHostRecord final : public nsHo
   static DnsPriority GetPriority(uint16_t aFlags);
 
   // When the lookups of this record started and their durations
   mozilla::TimeStamp mTrrStart;
   mozilla::TimeStamp mNativeStart;
   mozilla::TimeDuration mTrrDuration;
   mozilla::TimeDuration mNativeDuration;
 
-  nsAutoPtr<mozilla::net::AddrInfo> mFirstTRR;  // partial TRR storage
+  RefPtr<mozilla::net::AddrInfo> mFirstTRR;  // partial TRR storage
   nsresult mFirstTRRresult;
 
   uint8_t mTRRSuccess;     // number of successful TRR responses
   uint8_t mNativeSuccess;  // number of native lookup responses
 
   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. */