From 9b25e934ea965407b2d8b106cd5123b5d044b347 Mon Sep 17 00:00:00 2001
From: Valentin Gosu <valentin.gosu@gmail.com>
Date: Tue, 7 Jul 2020 10:08:28 +0000
Subject: [PATCH] Bug 1649127 - Make sure we only set AddrHostRecord::mTRRUsed
 = true when TRRServiceChannel::AsyncOpen succeeds r=dragana,necko-reviewers

mTRRUsed is a variable that we check to gate several telemetry probes, and to
decide if TRR really failed and we should add a domain to the TRR blocklist.
The problem with setting this too early is that when this is true but we
don't actually send the TRR request, then we will report that we fell back
to Do53 and potentially skip next TRR requests in the future.

The solution here is to only set mTRRUsed if TRRServiceChannel::AsyncOpen
succeeds.

Differential Revision: https://phabricator.services.mozilla.com/D81517
---
 netwerk/dns/TRR.cpp            |  7 +++++++
 netwerk/dns/nsHostResolver.cpp | 12 ++++++++++--
 netwerk/dns/nsHostResolver.h   | 15 +++++++--------
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/netwerk/dns/TRR.cpp b/netwerk/dns/TRR.cpp
index 53369dab3d72..aece94c30342 100644
--- a/netwerk/dns/TRR.cpp
+++ b/netwerk/dns/TRR.cpp
@@ -410,6 +410,13 @@ nsresult TRR::SendHTTPRequest() {
     return rv;
   }
 
+  // If the asyncOpen succeeded we can say that we actually attempted to
+  // use the TRR connection.
+  RefPtr<AddrHostRecord> addrRec = do_QueryObject(mRec);
+  if (addrRec) {
+    addrRec->mTRRUsed = true;
+  }
+
   NS_NewTimerWithCallback(getter_AddRefs(mTimeout), this,
                           gTRRService->GetRequestTimeout(),
                           nsITimer::TYPE_ONE_SHOT);
diff --git a/netwerk/dns/nsHostResolver.cpp b/netwerk/dns/nsHostResolver.cpp
index ce3ab8b5550d..23e0a59deb5b 100644
--- a/netwerk/dns/nsHostResolver.cpp
+++ b/netwerk/dns/nsHostResolver.cpp
@@ -281,10 +281,10 @@ AddrHostRecord::AddrHostRecord(const nsHostKey& key)
       addr_info(nullptr),
       addr(nullptr),
       mFirstTRRresult(NS_OK),
+      mTRRUsed(false),
       mTRRSuccess(0),
       mNativeSuccess(0),
       mNative(false),
-      mTRRUsed(false),
       mNativeUsed(false),
       onQueue(false),
       usingAnyThread(false),
@@ -1309,7 +1309,6 @@ nsresult nsHostResolver::TrrLookup(nsHostRecord* aRec, TRR* pushedTRR) {
   if (addrRec) {
     addrRec->mTRRSuccess = 0;  // bump for each successful TRR response
     addrRec->mTrrStart = TimeStamp::Now();
-    addrRec->mTRRUsed = true;  // this record gets TRR treatment
     addrRec->mTrrAUsed = AddrHostRecord::INIT;
     addrRec->mTrrAAAAUsed = AddrHostRecord::INIT;
 
@@ -1568,6 +1567,15 @@ nsresult nsHostResolver::NameLookup(nsHostRecord* rec) {
     if (!rec->IsAddrRecord()) {
       return rv;
     }
+
+#ifdef DEBUG
+    // If we use this branch then the mTRRUsed flag should not be set
+    // Even if we did call TrrLookup above, the fact that it failed sync-ly
+    // means that we didn't actually succeed in opening the channel.
+    RefPtr<AddrHostRecord> addrRec = do_QueryObject(rec);
+    MOZ_ASSERT(addrRec && !addrRec->mTRRUsed);
+#endif
+
     rv = NativeLookup(rec);
   }
 
diff --git a/netwerk/dns/nsHostResolver.h b/netwerk/dns/nsHostResolver.h
index 8114eae1695e..788cd501b6b5 100644
--- a/netwerk/dns/nsHostResolver.h
+++ b/netwerk/dns/nsHostResolver.h
@@ -206,10 +206,9 @@ class AddrHostRecord final : public nsHostRecord {
 
   size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const override;
 
-  bool IsTRR() { return mTRRUsed; }
-
  private:
   friend class nsHostResolver;
+  friend class mozilla::net::TRR;
 
   explicit AddrHostRecord(const nsHostKey& key);
   ~AddrHostRecord();
@@ -240,13 +239,13 @@ class AddrHostRecord final : public nsHostRecord {
   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
+  mozilla::Atomic<bool> mTRRUsed;  // TRR was used on this record
+  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. */
-  uint16_t mTRRUsed : 1;  // TRR was used on this record
+  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. */
   uint16_t mNativeUsed : 1;
   uint16_t onQueue : 1;         // true if pending and on the queue (not yet
                                 // given to getaddrinfo())