Backed out 2 changesets (bug 1420677) for causing bug 1534550 a=backout
authorBogdan Tara <btara@mozilla.com>
Tue, 12 Mar 2019 11:54:19 +0200
changeset 521498 4d15e90af575a68815975bac8c2b602f78d76ee8
parent 521497 78bc178af5840669071e87af36fa20b4bc6065fa
child 521499 445c24a51727b447ad2789275be2da490b97f7cd
child 521510 17b03d65297c1f5e56a6b5c7c12dcd8b44fe31a0
child 521558 581eed270cffaf9c19285ce97029aa5516123829
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)
reviewersbackout
bugs1420677, 1534550
milestone67.0a1
backs outcf114035c79fa00872568b863ef84c24ad8e7de2
edff1f39d426d7896dd9f48b06ada1e28fe820bb
first release with
nightly linux32
4d15e90af575 / 67.0a1 / 20190312095443 / files
nightly linux64
4d15e90af575 / 67.0a1 / 20190312095443 / files
nightly mac
4d15e90af575 / 67.0a1 / 20190312095443 / files
nightly win32
4d15e90af575 / 67.0a1 / 20190312095443 / files
nightly win64
4d15e90af575 / 67.0a1 / 20190312095443 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out 2 changesets (bug 1420677) for causing bug 1534550 a=backout Backed out changeset cf114035c79f (bug 1420677) Backed out changeset edff1f39d426 (bug 1420677)
netwerk/dns/DNS.cpp
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.cpp
+++ b/netwerk/dns/DNS.cpp
@@ -325,16 +325,20 @@ AddrInfo::AddrInfo(const AddrInfo *src) 
 
   for (auto element = src->mAddresses.getFirst(); element;
        element = element->getNext()) {
     AddAddress(new NetAddrElement(*element));
   }
 }
 
 AddrInfo::~AddrInfo() {
+  NetAddrElement *addrElement;
+  while ((addrElement = mAddresses.popLast())) {
+    delete addrElement;
+  }
 }
 
 void AddrInfo::AddAddress(NetAddrElement *address) {
   MOZ_ASSERT(address, "Cannot add the address to an uninitialized list");
 
   mAddresses.insertBack(address);
 }
 
--- a/netwerk/dns/DNS.h
+++ b/netwerk/dns/DNS.h
@@ -7,17 +7,16 @@
 #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
@@ -119,48 +118,46 @@ 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;
+  LinkedList<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);
-  RefPtr<AddrInfo> ai(new AddrInfo(aCanonHost, prai, disableIPv4,
+  nsAutoPtr<AddrInfo> ai(new AddrInfo(aCanonHost, prai, disableIPv4,
                                       filterNameCollision, canonName));
   PR_FreeAddrInfo(prai);
   if (ai->mAddresses.isEmpty()) {
     return NS_ERROR_UNKNOWN_HOST;
   }
 
-  ai.forget(aAddrInfo);
+  *aAddrInfo = ai.forget();
 
   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
-    RefPtr<AddrInfo> ai(new AddrInfo(mHost, mType));
+    nsAutoPtr<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, mPB,
+    (void)mHostResolver->CompleteLookup(mRec, NS_OK, ai.forget(), 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
-    RefPtr<AddrInfo> ai = new AddrInfo(mHost, mType);
+    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);
 
-  RefPtr<AddrInfo> newRRSet(aNewRRSet);
+  nsAutoPtr<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,16 +292,17 @@ 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
@@ -1003,16 +1004,19 @@ 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).
@@ -1682,17 +1686,19 @@ 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);
 
-  RefPtr<AddrInfo> newRRSet(aNewRRSet);
+  // newRRSet needs to be taken into the hostrecord (which will then own it)
+  // or deleted on early return.
+  nsAutoPtr<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;
@@ -1737,17 +1743,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.swap(newRRSet);  // autoPtr.swap()
+      addrRec->mFirstTRR = 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
@@ -1763,17 +1769,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.swap(addrRec->mFirstTRR);  // transfers
+          newRRSet = 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
@@ -1812,17 +1818,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);
-    RefPtr<AddrInfo> old_addr_info;
+    nsAutoPtr<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;
@@ -2027,17 +2033,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;
-  RefPtr<AddrInfo> ai;
+  AddrInfo *ai = nullptr;
 
   do {
     if (!rec) {
       RefPtr<AddrHostRecord> tmpRec;
       if (!GetHostToLookup(getter_AddRefs(tmpRec))) {
         break;  // thread shutdown signal
       }
       // GetHostToLookup() returns an owning reference
@@ -2048,20 +2054,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, getter_AddRefs(ai), getTtl);
+    nsresult status = GetAddrInfo(rec->host, rec->af, rec->flags, &ai, getTtl);
 #if defined(RES_RETRY_ON_FAILURE)
     if (NS_FAILED(status) && rs.Reset()) {
-      status = GetAddrInfo(rec->host, rec->af, rec->flags, getter_AddRefs(ai), getTtl);
+      status = GetAddrInfo(rec->host, rec->af, rec->flags, &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| */
-  RefPtr<mozilla::net::AddrInfo> addr_info;
+  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;
 
-  RefPtr<mozilla::net::AddrInfo> mFirstTRR;  // partial TRR storage
+  nsAutoPtr<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. */