Bug 1768140 - Reset mTRRSuccess before a new lookup, r=necko-reviewers,dragana

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

netwerk/dns/TRR.cpp

@@ -669,8 +669,6 @@ void TRR::SaveAdditionalRecords(
     // This is quite hacky, and should be fixed.
     hostRecord->mResolving++;
     hostRecord->mEffectiveTRRMode = mRec->mEffectiveTRRMode;
-    RefPtr<AddrHostRecord> addrRec = do_QueryObject(hostRecord);
-    addrRec->mTrrStart = TimeStamp::Now();
     LOG(("Completing lookup for additional: %s",
          nsCString(recordEntry.GetKey()).get()));
     (void)mHostResolver->CompleteLookup(hostRecord, NS_OK, ai, mPB,
@@ -710,9 +708,6 @@ void TRR::StoreIPHintAsDNSRecord(const struct SVCB& aSVCBRecord) {
   // This is quite hacky, and should be fixed.
   hostRecord->mResolving++;
   hostRecord->mEffectiveTRRMode = mRec->mEffectiveTRRMode;
-  RefPtr<AddrHostRecord> addrRec = do_QueryObject(hostRecord);
-  addrRec->mTrrStart = TimeStamp::Now();
-
   (void)mHostResolver->CompleteLookup(hostRecord, NS_OK, ai, mPB, mOriginSuffix,
                                       TRRSkippedReason::TRR_OK, this);
 }

netwerk/dns/nsHostRecord.cpp

@@ -301,6 +301,8 @@ void AddrHostRecord::ResolveComplete() {
 
   if (mResolverType == DNSResolverType::TRR) {
     if (mTRRSuccess) {
+      MOZ_DIAGNOSTIC_ASSERT(mTRRSkippedReason ==
+                            mozilla::net::TRRSkippedReason::TRR_OK);
       uint32_t millis = static_cast<uint32_t>(mTrrDuration.ToMilliseconds());
       Telemetry::Accumulate(Telemetry::DNS_TRR_LOOKUP_TIME3,
                             TRRService::ProviderKey(), millis);

netwerk/dns/nsHostRecord.h

@@ -165,6 +165,16 @@ class nsHostRecord : public mozilla::LinkedListElement<RefPtr<nsHostRecord>>,
     return type == nsIDNSService::RESOLVE_TYPE_DEFAULT;
   }
 
+  virtual void Reset() {
+    mTRRSkippedReason = TRRSkippedReason::TRR_UNSET;
+    mFirstTRRSkippedReason = TRRSkippedReason::TRR_UNSET;
+    mTrrAttempts = 0;
+    mTRRSuccess = false;
+    mNativeSuccess = false;
+  }
+
+  virtual void OnCompleteLookup() {}
+
   // When the record began being valid. Used mainly for bookkeeping.
   mozilla::TimeStamp mValidStart;
 
@@ -187,8 +197,6 @@ class nsHostRecord : public mozilla::LinkedListElement<RefPtr<nsHostRecord>>,
 
   TRRSkippedReason mTRRSkippedReason = TRRSkippedReason::TRR_UNSET;
   TRRSkippedReason mFirstTRRSkippedReason = TRRSkippedReason::TRR_UNSET;
-  TRRSkippedReason mTRRAFailReason = TRRSkippedReason::TRR_UNSET;
-  TRRSkippedReason mTRRAAAAFailReason = TRRSkippedReason::TRR_UNSET;
 
   mozilla::DataMutex<RefPtr<mozilla::net::TRRQuery>> mTRRQuery;
 
@@ -206,6 +214,12 @@ class nsHostRecord : public mozilla::LinkedListElement<RefPtr<nsHostRecord>>,
 
   // Explicitly expired
   bool mDoomed = false;
+
+  // Whether this is resolved by TRR successfully or not.
+  bool mTRRSuccess = false;
+
+  // Whether this is resolved by native resolver successfully or not.
+  bool mNativeSuccess = false;
 };
 
 // b020e996-f6ab-45e5-9bf5-1da71dd0053a
@@ -284,16 +298,25 @@ class AddrHostRecord final : public nsHostRecord {
   // true if pending and on the queue (not yet given to getaddrinfo())
   bool onQueue() { return LoadNative() && isInList(); }
 
+  virtual void Reset() override {
+    nsHostRecord::Reset();
+    StoreNativeUsed(false);
+    mResolverType = DNSResolverType::Native;
+  }
+
+  virtual void OnCompleteLookup() override {
+    nsHostRecord::OnCompleteLookup();
+    // This should always be cleared when a request is completed.
+    StoreNative(false);
+  }
+
   // When the lookups of this record started and their durations
-  mozilla::TimeStamp mTrrStart;
   mozilla::TimeStamp mNativeStart;
   mozilla::TimeDuration mTrrDuration;
   mozilla::TimeDuration mNativeDuration;
 
   // TRR or ODoH was used on this record
   mozilla::Atomic<DNSResolverType> mResolverType{DNSResolverType::Native};
-  uint8_t mTRRSuccess = 0;     // number of successful TRR responses
-  uint8_t mNativeSuccess = 0;  // number of native lookup responses
 
   // clang-format off
   MOZ_ATOMIC_BITFIELDS(mAtomicBitfields, 8, (

netwerk/dns/nsHostResolver.cpp

@@ -1091,21 +1091,10 @@ nsresult nsHostResolver::NameLookup(nsHostRecord* rec,
 
   // Make sure we reset the reason each time we attempt to do a new lookup
   // so we don't wrongly report the reason for the previous one.
-  rec->mTRRSkippedReason = TRRSkippedReason::TRR_UNSET;
-  rec->mFirstTRRSkippedReason = TRRSkippedReason::TRR_UNSET;
-  rec->mTrrAttempts = 0;
+  rec->Reset();
 
   ComputeEffectiveTRRMode(rec);
 
-  if (rec->IsAddrRecord()) {
-    RefPtr<AddrHostRecord> addrRec = do_QueryObject(rec);
-    MOZ_ASSERT(addrRec);
-
-    addrRec->StoreNativeUsed(false);
-    addrRec->mResolverType = DNSResolverType::Native;
-    addrRec->mNativeSuccess = false;
-  }
-
   if (!rec->mTrrServer.IsEmpty()) {
     LOG(("NameLookup: %s use trr:%s", rec->host.get(), rec->mTrrServer.get()));
     if (rec->mEffectiveTRRMode != nsIRequest::TRR_ONLY_MODE) {
@@ -1443,7 +1432,7 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookupLocked(
         addrRec->RecordReason(TRRSkippedReason::TRR_FAILED);
       }
     } else {
-      addrRec->mTRRSuccess++;
+      addrRec->mTRRSuccess = true;
       addrRec->RecordReason(TRRSkippedReason::TRR_OK);
     }
 
@@ -1473,8 +1462,7 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookupLocked(
     }
   }
 
-  // This should always be cleared when a request is completed.
-  addrRec->StoreNative(false);
+  addrRec->OnCompleteLookup();
 
   // 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