bug 1463356 do not count "not started" TRR resolves as failures r?mcmanus draft
authorDaniel Stenberg <daniel@haxx.se>
Wed, 20 Jun 2018 11:00:19 +0200
changeset 808822 cbdf9b7127ed40b4aeada3c1f0c94e70d14fc99d
parent 808416 257c191e7903523a1132e04460a0b2460d950809
push id113506
push userbmo:daniel@haxx.se
push dateWed, 20 Jun 2018 21:15:25 +0000
reviewersmcmanus
bugs1463356
milestone62.0a1
bug 1463356 do not count "not started" TRR resolves as failures r?mcmanus ... when comparing against the native resolver. DNS_TRR_COMPARE is meant to compare how the actually performed name resolves fare against each other. MozReview-Commit-ID: 98NoUGPpHr6
netwerk/dns/TRR.cpp
netwerk/dns/TRR.h
netwerk/dns/nsHostResolver.cpp
netwerk/dns/nsHostResolver.h
--- a/netwerk/dns/TRR.cpp
+++ b/netwerk/dns/TRR.cpp
@@ -117,17 +117,17 @@ TRR::DohEncode(nsCString &aBody)
   return NS_OK;
 }
 
 NS_IMETHODIMP
 TRR::Run()
 {
   MOZ_ASSERT(NS_IsMainThread());
   if ((gTRRService == nullptr) || NS_FAILED(SendHTTPRequest())) {
-    FailData();
+    FailData(NS_ERROR_FAILURE);
     // The dtor will now be run
   }
   return NS_OK;
 }
 
 nsresult
 TRR::SendHTTPRequest()
 {
@@ -845,26 +845,26 @@ TRR::ReturnData()
   }
   (void)mHostResolver->CompleteLookup(mRec, NS_OK, ai.forget(), mPB);
   mHostResolver = nullptr;
   mRec = nullptr;
   return NS_OK;
 }
 
 nsresult
-TRR::FailData()
+TRR::FailData(nsresult error)
 {
   if (!mHostResolver) {
     return NS_ERROR_FAILURE;
   }
   // create and populate an TRR AddrInfo instance to pass on to signal that
   // this comes from TRR
   AddrInfo *ai = new AddrInfo(mHost, mType);
 
-  (void)mHostResolver->CompleteLookup(mRec, NS_ERROR_FAILURE, ai, mPB);
+  (void)mHostResolver->CompleteLookup(mRec, error, ai, mPB);
   mHostResolver = nullptr;
   mRec = nullptr;
   return NS_OK;
 }
 
 nsresult
 TRR::On200Response()
 {
@@ -928,17 +928,17 @@ TRR::OnStopRequest(nsIRequest *aRequest,
     nsresult rv = NS_OK;
     nsAutoCString contentType;
     httpChannel->GetContentType(contentType);
     if (contentType.Length() &&
         !contentType.LowerCaseEqualsLiteral("application/dns-udpwireformat")) {
       // try and parse missing content-types, but otherwise require udpwireformat
       LOG(("TRR:OnStopRequest %p %s %d should fail due to content type %s\n",
            this, mHost.get(), mType, contentType.get()));
-      FailData();
+      FailData(NS_ERROR_UNEXPECTED);
       return NS_OK;
     }
 
     uint32_t httpStatus;
     rv = httpChannel->GetResponseStatus(&httpStatus);
     if (NS_SUCCEEDED(rv) && httpStatus == 200) {
       rv = On200Response();
       if (NS_SUCCEEDED(rv)) {
@@ -947,17 +947,17 @@ TRR::OnStopRequest(nsIRequest *aRequest,
     } else {
       LOG(("TRR:OnStopRequest:%d %p rv %x httpStatus %d\n", __LINE__,
            this, (int)rv, httpStatus));
     }
   }
 
   LOG(("TRR:OnStopRequest %p status %x mFailed %d\n",
        this, (int)aStatusCode, mFailed));
-  FailData();
+  FailData(NS_ERROR_UNKNOWN_HOST);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 TRR::OnDataAvailable(nsIRequest *aRequest,
                      nsISupports *aContext,
                      nsIInputStream *aInputStream,
                      uint64_t aOffset,
--- a/netwerk/dns/TRR.h
+++ b/netwerk/dns/TRR.h
@@ -142,17 +142,24 @@ public:
 private:
   ~TRR() = default;
   nsresult SendHTTPRequest();
   nsresult DohEncode(nsCString &target);
   nsresult PassQName(unsigned int &index);
   nsresult GetQname(nsAutoCString &aQname, unsigned int &aIndex);
   nsresult DohDecode(nsCString &aHost);
   nsresult ReturnData();
-  nsresult FailData();
+
+  // FailData() must be called to signal that the asynch TRR resolve is
+  // completed. For failed name resolves ("no such host"), the 'error' it
+  // passses on in its argument must be NS_ERROR_UNKNOWN_HOST. Other errors
+  // (if host was blacklisted, there as a bad content-type received, etc)
+  // other error codes must be used. This distinction is important for the
+  // subsequent logic to separate the error reasons.
+  nsresult FailData(nsresult error);
   nsresult DohDecodeQuery(const nsCString &query,
                           nsCString &host, enum TrrType &type);
   nsresult ReceivePush(nsIHttpChannel *pushed, nsHostRecord *pushedRec);
   nsresult On200Response();
 
   nsCOMPtr<nsIChannel> mChannel;
   enum TrrType mType;
   TimeStamp mStartTime;
--- a/netwerk/dns/nsHostResolver.cpp
+++ b/netwerk/dns/nsHostResolver.cpp
@@ -179,16 +179,17 @@ nsHostKey::SizeOfExcludingThis(mozilla::
 nsHostRecord::nsHostRecord(const nsHostKey& key)
     : nsHostKey(key)
     , addr_info_lock("nsHostRecord.addr_info_lock")
     , addr_info_gencnt(0)
     , addr_info(nullptr)
     , addr(nullptr)
     , negative(false)
     , mResolverMode(MODE_NATIVEONLY)
+    , mFirstTRRresult(NS_OK)
     , mResolving(0)
     , mTRRSuccess(0)
     , mNativeSuccess(0)
     , mNative(false)
     , mTRRUsed(false)
     , mNativeUsed(false)
     , onQueue(false)
     , usingAnyThread(false)
@@ -1511,16 +1512,17 @@ nsHostResolver::CompleteLookup(nsHostRec
         } else {
             MOZ_ASSERT(0);
         }
 
         if (NS_SUCCEEDED(status)) {
             rec->mTRRSuccess++;
         }
         if (TRROutstanding()) {
+            rec->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(!rec->mFirstTRR && newRRSet);
             rec->mFirstTRR = newRRSet; // autoPtr.swap()
@@ -1550,28 +1552,39 @@ nsHostResolver::CompleteLookup(nsHostRec
                     merge_rrset(newRRSet, rec->mFirstTRR);
                 }
                 else {
                     newRRSet = rec->mFirstTRR; // transfers
                 }
                 rec->mFirstTRR = nullptr;
             }
 
+            if (NS_FAILED(rec->mFirstTRRresult) &&
+                NS_FAILED(status) &&
+                (rec->mFirstTRRresult != NS_ERROR_UNKNOWN_HOST) &&
+                (status != NS_ERROR_UNKNOWN_HOST)) {
+                // the errors are not failed resolves, that means
+                // something else failed, consider this as *TRR not used*
+                // for actually trying to resolve the host
+                rec->mTRRUsed = false;
+            }
+
             if (!rec->mTRRSuccess) {
                 // no TRR success
                 newRRSet = nullptr;
                 status = NS_ERROR_UNKNOWN_HOST;
             }
 
             if (!rec->mTRRSuccess && rec->mResolverMode == MODE_TRRFIRST) {
                 MOZ_ASSERT(!rec->mResolving);
                 NativeLookup(rec);
                 MOZ_ASSERT(rec->mResolving);
                 return LOOKUP_OK;
             }
+
             // continue
         }
 
         if (NS_SUCCEEDED(status) && (rec->mTRRSuccess == 1)) {
             // store the duration on first (used) TRR response
             rec->mTrrDuration = TimeStamp::Now() - rec->mTrrStart;
         }
 
@@ -1795,17 +1808,17 @@ nsHostResolver::ThreadFunc(void *arg)
             }
             // GetHostToLookup() returns an owning reference
             MOZ_ASSERT(tmpRec);
             rec.swap(tmpRec);
         }
 
         LOG(("DNS lookup thread - Calling getaddrinfo for host [%s].\n",
              rec->host.get()));
-        
+
         TimeStamp startTime = TimeStamp::Now();
         bool getTtl = rec->mGetTtl;
         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,
--- a/netwerk/dns/nsHostResolver.h
+++ b/netwerk/dns/nsHostResolver.h
@@ -169,16 +169,17 @@ public:
     mozilla::net::ResolverMode mResolverMode;
 
 private:
     friend class nsHostResolver;
 
     explicit nsHostRecord(const nsHostKey& key);
     mozilla::LinkedList<RefPtr<nsResolveHostCallback>> mCallbacks;
     nsAutoPtr<mozilla::net::AddrInfo> mFirstTRR; // partial TRR storage
+    nsresult mFirstTRRresult;
 
     uint16_t  mResolving;  // counter of outstanding resolving calls
     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. */