Bug 1552438 - Remove TRR mode 4 (MODE_SHADOW) r=agrover

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

--HG--
extra : moz-landing-system : lando

netwerk/dns/TRRService.cpp

@@ -125,7 +125,7 @@ void TRRService::GetPrefBranch(nsIPrefBranch** result) {
 nsresult TRRService::ReadPrefs(const char* name) {
   MOZ_ASSERT(NS_IsMainThread(), "wrong thread");
   if (!name || !strcmp(name, TRR_PREF("mode"))) {
-    // 0 - off, 1 - reserved, 2 - TRR first, 3 - TRR only, 4 - shadow,
+    // 0 - off, 1 - reserved, 2 - TRR first, 3 - TRR only, 4 - reserved,
     // 5 - explicit off
     uint32_t tmp;
     if (NS_SUCCEEDED(Preferences::GetUint(TRR_PREF("mode"), &tmp))) {
@@ -135,6 +135,9 @@ nsresult TRRService::ReadPrefs(const char* name) {
       if (tmp == MODE_RESERVED1) {
         tmp = MODE_TRROFF;
       }
+      if (tmp == MODE_RESERVED4) {
+        tmp = MODE_TRROFF;
+      }
       mMode = tmp;
     }
   }

netwerk/dns/nsHostResolver.cpp

@@ -442,44 +442,7 @@ void AddrHostRecord::ResolveComplete() {
     }
   }
 
-  if (mTRRUsed && mNativeUsed && mNativeSuccess &&
-      mTRRSuccess) {  // race or shadow!
-    static const TimeDuration k50ms = TimeDuration::FromMilliseconds(50);
-    static const TimeDuration k100ms = TimeDuration::FromMilliseconds(100);
-    if (mTrrDuration <= mNativeDuration) {
-      if ((mNativeDuration - mTrrDuration) > k100ms) {
-        AccumulateCategorical(Telemetry::LABELS_DNS_TRR_RACE2::TRRFasterBy100);
-      } else if ((mNativeDuration - mTrrDuration) > k50ms) {
-        AccumulateCategorical(Telemetry::LABELS_DNS_TRR_RACE2::TRRFasterBy50);
-      } else {
-        AccumulateCategorical(Telemetry::LABELS_DNS_TRR_RACE2::TRRFaster);
-      }
-      LOG(("nsHostRecord::Complete %s Dns Race: TRR\n", host.get()));
-    } else {
-      if ((mTrrDuration - mNativeDuration) > k100ms) {
-        AccumulateCategorical(
-            Telemetry::LABELS_DNS_TRR_RACE2::NativeFasterBy100);
-      } else if ((mTrrDuration - mNativeDuration) > k50ms) {
-        AccumulateCategorical(
-            Telemetry::LABELS_DNS_TRR_RACE2::NativeFasterBy50);
-      } else {
-        AccumulateCategorical(Telemetry::LABELS_DNS_TRR_RACE2::NativeFaster);
-      }
-      LOG(("nsHostRecord::Complete %s Dns Race: NATIVE\n", host.get()));
-    }
-  }
-
-  if (mTRRUsed && mNativeUsed && ((mResolverMode == MODE_SHADOW))) {
-    // both were used, accumulate comparative success
-    AccumulateCategorical(
-        mNativeSuccess && mTRRSuccess
-            ? Telemetry::LABELS_DNS_TRR_COMPARE::BothWorked
-            : ((mNativeSuccess
-                    ? Telemetry::LABELS_DNS_TRR_COMPARE::NativeWorked
-                    : (mTRRSuccess
-                           ? Telemetry::LABELS_DNS_TRR_COMPARE::TRRWorked
-                           : Telemetry::LABELS_DNS_TRR_COMPARE::BothFailed))));
-  } else if (mResolverMode == MODE_TRRFIRST) {
+  if (mResolverMode == MODE_TRRFIRST) {
     if (flags & nsIDNSService::RESOLVE_DISABLE_TRR) {
       // TRR is disabled on request, which is a next-level back-off method.
       Telemetry::Accumulate(Telemetry::DNS_TRR_DISABLED, mNativeSuccess);
@@ -510,12 +473,12 @@ void AddrHostRecord::ResolveComplete() {
     case MODE_TRRONLY:
       AccumulateCategorical(Telemetry::LABELS_DNS_LOOKUP_ALGORITHM::trrOnly);
       break;
-    case MODE_SHADOW:
-      AccumulateCategorical(Telemetry::LABELS_DNS_LOOKUP_ALGORITHM::trrShadow);
-      break;
     case MODE_RESERVED1:
       MOZ_DIAGNOSTIC_ASSERT(false, "MODE_RESERVED1 should not be used");
       break;
+    case MODE_RESERVED4:
+      MOZ_DIAGNOSTIC_ASSERT(false, "MODE_RESERVED4 should not be used");
+      break;
   }
 
   if (mTRRUsed && !mTRRSuccess && mNativeSuccess && gTRRService) {
@@ -1448,8 +1411,7 @@ nsresult nsHostResolver::NameLookup(nsHostRecord* rec) {
     rv = TrrLookup(rec);
   }
 
-  if (TRR_DISABLED(mode) || (mode == MODE_SHADOW) ||
-      ((mode == MODE_TRRFIRST) && NS_FAILED(rv)) ||
+  if (TRR_DISABLED(mode) || ((mode == MODE_TRRFIRST) && NS_FAILED(rv)) ||
       (mode == MODE_TRRONLY && skipTRR)) {
     if (!rec->IsAddrRecord()) {
       return rv;
@@ -1742,8 +1704,7 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookup(
       if (addrRec->mTRRSuccess == 1) {
         // Store the duration on first succesful TRR response.  We
         // don't know that there will be a second response nor can we
-        // tell which of two has useful data, especially in
-        // MODE_SHADOW where the actual results are discarded.
+        // tell which of two has useful data.
         addrRec->mTrrDuration = TimeStamp::Now() - addrRec->mTrrStart;
       }
     }
@@ -1759,7 +1720,7 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookup(
       addrRec->mFirstTRR.swap(newRRSet);  // autoPtr.swap()
       MOZ_ASSERT(addrRec->mFirstTRR && !newRRSet);
 
-      if (addrRec->mDidCallbacks || addrRec->mResolverMode == MODE_SHADOW) {
+      if (addrRec->mDidCallbacks) {
         return LOOKUP_OK;
       }
 
@@ -1828,8 +1789,7 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookup(
   // 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)) {
+  if (!mShutdown) {
     MutexAutoLock lock(addrRec->addr_info_lock);
     RefPtr<AddrInfo> old_addr_info;
     if (different_rrset(addrRec->addr_info, newRRSet)) {
@@ -1849,14 +1809,7 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookup(
 
   bool doCallbacks = true;
 
-  if (trrResult && (addrRec->mResolverMode == MODE_SHADOW) &&
-      !addrRec->mDidCallbacks) {
-    // don't report result based only on suppressed TRR info
-    doCallbacks = false;
-    LOG((
-        "nsHostResolver Suppressing TRR %s because it is first shadow result\n",
-        addrRec->host.get()));
-  } else if (trrResult && addrRec->mDidCallbacks) {
+  if (trrResult && addrRec->mDidCallbacks) {
     // already callback'ed on the first TRR response
     LOG(("nsHostResolver Suppressing callback for second TRR response for %s\n",
          addrRec->host.get()));

netwerk/dns/nsHostResolver.h

@@ -35,7 +35,7 @@ enum ResolverMode {
   MODE_RESERVED1,   // 1 - Reserved value. Used to be parallel resolve.
   MODE_TRRFIRST,    // 2 - fallback to native on TRR failure
   MODE_TRRONLY,     // 3 - don't even fallback
-  MODE_SHADOW,      // 4 - race for stats, but always use native result
+  MODE_RESERVED4,   // 4 - Reserved value. Used to be race TRR with native.
   MODE_TRROFF       // 5 - identical to MODE_NATIVEONLY but explicitly selected
 };
 }  // namespace net

netwerk/test/unit/test_trr.js

@@ -28,7 +28,7 @@ add_task(function setup() {
   // make all native resolve calls "secretly" resolve localhost instead
   Services.prefs.setBoolPref("network.dns.native-is-localhost", true);
 
-  // 0 - off, 1 - race, 2 TRR first, 3 TRR only, 4 shadow
+  // 0 - off, 1 - reserved, 2 - TRR first, 3  - TRR only, 4 - reserved
   Services.prefs.setIntPref("network.trr.mode", 2); // TRR first
   Services.prefs.setBoolPref("network.trr.wait-for-portal", false);
   // don't confirm that TRR is working, just go!
@@ -299,21 +299,29 @@ add_task(async function test13() {
   await new DNSListener("test13.example.com", "127.0.0.1");
 });
 
-// TRR-shadow gets a 404 back from DOH
+// Test that MODE_RESERVED4 (previously MODE_SHADOW) is treated as TRR off.
 add_task(async function test14() {
   dns.clearCache(true);
-  Services.prefs.setIntPref("network.trr.mode", 4); // TRR-shadow
+  Services.prefs.setIntPref("network.trr.mode", 4); // MODE_RESERVED4. Interpreted as TRR off.
   Services.prefs.setCharPref("network.trr.uri", `https://foo.example.com:${h2Port}/404`);
   await new DNSListener("test14.example.com", "127.0.0.1");
+
+  Services.prefs.setIntPref("network.trr.mode", 4); // MODE_RESERVED4. Interpreted as TRR off.
+  Services.prefs.setCharPref("network.trr.uri", `https://foo.example.com:${h2Port}/doh?responseIP=2.2.2.2`);
+  await new DNSListener("bar.example.com", "127.0.0.1");
 });
 
-// TRR-shadow and timed out TRR
+// Test that MODE_RESERVED4 (previously MODE_SHADOW) is treated as TRR off.
 add_task(async function test15() {
   dns.clearCache(true);
-  Services.prefs.setIntPref("network.trr.mode", 4); // TRR-shadow
+  Services.prefs.setIntPref("network.trr.mode", 4); // MODE_RESERVED4. Interpreted as TRR off.
   Services.prefs.setCharPref("network.trr.uri", `https://foo.example.com:${h2Port}/dns-750ms`);
   Services.prefs.setIntPref("network.trr.request-timeout", 10);
   await new DNSListener("test15.example.com", "127.0.0.1");
+
+  Services.prefs.setIntPref("network.trr.mode", 4); // MODE_RESERVED4. Interpreted as TRR off.
+  Services.prefs.setCharPref("network.trr.uri", `https://foo.example.com:${h2Port}/doh?responseIP=2.2.2.2`);
+  await new DNSListener("bar.example.com", "127.0.0.1");
 });
 
 // TRR-first and timed out TRR
@@ -363,12 +371,16 @@ add_task(async function test20() {
   await new DNSListener("test20.example.com", "127.0.0.1");
 });
 
-// TRR-shadow and a CNAME loop
+// Test that MODE_RESERVED4 (previously MODE_SHADOW) is treated as TRR off.
 add_task(async function test21() {
   dns.clearCache(true);
-  Services.prefs.setIntPref("network.trr.mode", 4); // shadow
+  Services.prefs.setIntPref("network.trr.mode", 4); // MODE_RESERVED4. Interpreted as TRR off.
   Services.prefs.setCharPref("network.trr.uri", `https://foo.example.com:${h2Port}/doh?responseIP=none&cnameloop=true`);
   await new DNSListener("test21.example.com", "127.0.0.1");
+
+  Services.prefs.setIntPref("network.trr.mode", 4); // MODE_RESERVED4. Interpreted as TRR off.
+  Services.prefs.setCharPref("network.trr.uri", `https://foo.example.com:${h2Port}/doh?responseIP=2.2.2.2`);
+  await new DNSListener("bar.example.com", "127.0.0.1");
 });
 
 // verify that basic A record name mismatch gets rejected.