Bug 1689113 - Don't skip TRR unless confirmation explicitly failed. r=necko-reviewers,valentin

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

modules/libpref/init/StaticPrefList.yaml

@@ -494,7 +494,7 @@
     type: RelaxedAtomicBool
     value: true
     mirror: always
-    
+
 -   name: apz.gtk.touchpad_pinch.enabled
     type: RelaxedAtomicBool
     value: false
@@ -8702,6 +8702,14 @@
   value: false
   mirror: always
 
+# If we should wait for TRR service confirmation to complete before enabling
+# TRR for lookups when fallback is enabled. Confirmation is always skipped when
+# global mode is TRR-only (no fallback).
+- name: network.trr.wait-for-confirmation
+  type: RelaxedAtomicBool
+  value: false
+  mirror: always
+
 # Use GET (rather than POST)
 - name: network.trr.useGET
   type: RelaxedAtomicBool

netwerk/dns/TRRService.cpp

@@ -230,14 +230,16 @@ void TRRService::SetDetectedTrrURI(const nsACString& aURI) {
   mURISetByDetection = MaybeSetPrivateURI(aURI);
 }
 
-bool TRRService::Enabled(nsIRequest::TRRMode aMode) {
-  if (mMode == nsIDNSService::MODE_TRROFF) {
+bool TRRService::Enabled(nsIRequest::TRRMode aRequestMode) {
+  if (mMode == nsIDNSService::MODE_TRROFF ||
+      aRequestMode == nsIRequest::TRR_DISABLED_MODE) {
     return false;
   }
+
   if (mConfirmationState == CONFIRM_INIT &&
       (!StaticPrefs::network_trr_wait_for_portal() || mCaptiveIsPassed ||
        (mMode == nsIDNSService::MODE_TRRONLY ||
-        aMode == nsIRequest::TRR_ONLY_MODE))) {
+        aRequestMode == nsIRequest::TRR_ONLY_MODE))) {
     LOG(("TRRService::Enabled => CONFIRM_TRYING\n"));
     mConfirmationState = CONFIRM_TRYING;
   }
@@ -245,14 +247,38 @@ bool TRRService::Enabled(nsIRequest::TRRMode aMode) {
   if (mConfirmationState == CONFIRM_TRYING) {
     LOG(("TRRService::Enabled MaybeConfirm()\n"));
     MaybeConfirm();
+    if (mMode == nsIDNSService::MODE_TRRONLY) {
+      MOZ_ASSERT(mConfirmationState == CONFIRM_OK,
+                 "Global mode is trr-only, but confirmation failed?");
+    }
   }
 
-  if (mConfirmationState != CONFIRM_OK) {
-    LOG(("TRRService::Enabled mConfirmationState=%d mCaptiveIsPassed=%d\n",
-         (int)mConfirmationState, (int)mCaptiveIsPassed));
+  LOG(("TRRService::Enabled mConfirmationState=%d mCaptiveIsPassed=%d\n",
+       (int)mConfirmationState, (int)mCaptiveIsPassed));
+
+  if (mConfirmationState == CONFIRM_OK) {
+    return true;
   }
 
-  return (mConfirmationState == CONFIRM_OK);
+  if (StaticPrefs::network_trr_wait_for_confirmation()) {
+    return false;
+  }
+
+  if ((aRequestMode == nsIRequest::TRR_DEFAULT_MODE &&
+       mMode == nsIDNSService::MODE_TRRONLY) ||
+      aRequestMode == nsIRequest::TRR_ONLY_MODE) {
+    // For TRR-only requests, or if the global mode is TRR-only, just say we're
+    // enabled.
+    return true;
+  }
+
+  if ((aRequestMode == nsIRequest::TRR_DEFAULT_MODE &&
+       mMode == nsIDNSService::MODE_TRRFIRST) ||
+      aRequestMode == nsIRequest::TRR_FIRST_MODE) {
+    return mConfirmationState != CONFIRM_FAILED;
+  }
+
+  return false;
 }
 
 void TRRService::GetPrefBranch(nsIPrefBranch** result) {
@@ -691,7 +717,7 @@ bool TRRService::MaybeBootstrap(const nsACString& aPossible,
 bool TRRService::IsDomainBlocked(const nsACString& aHost,
                                  const nsACString& aOriginSuffix,
                                  bool aPrivateBrowsing) {
-  if (!Enabled(nsIRequest::TRR_DEFAULT_MODE)) {
+  if (!Enabled()) {
     return true;
   }
 

netwerk/dns/TRRService.h

@@ -38,7 +38,7 @@ class TRRService : public TRRServiceBase,
   TRRService();
   nsresult Init();
   nsresult Start();
-  bool Enabled(nsIRequest::TRRMode aMode = nsIRequest::TRR_FIRST_MODE);
+  bool Enabled(nsIRequest::TRRMode aRequestMode = nsIRequest::TRR_DEFAULT_MODE);
   bool IsConfirmed() { return mConfirmationState == CONFIRM_OK; }
 
   bool DisableIPv6() { return mDisableIPv6; }

netwerk/dns/nsHostResolver.cpp

@@ -1637,7 +1637,8 @@ nsresult nsHostResolver::NameLookup(nsHostRecord* rec) {
     rv = TrrLookup(rec);
   }
 
-  bool serviceNotReady = !gTRRService || !gTRRService->IsConfirmed();
+  bool serviceNotReady =
+      !gTRRService || !gTRRService->Enabled(rec->mEffectiveTRRMode);
 
   if (rec->mEffectiveTRRMode == nsIRequest::TRR_DISABLED_MODE ||
       (rec->mEffectiveTRRMode == nsIRequest::TRR_FIRST_MODE &&

netwerk/test/unit/test_trr.js

@@ -195,6 +195,38 @@ class DNSListener {
   }
 }
 
+async function waitForConfirmation(expectedResponseIP, confirmationShouldFail) {
+  // Check that the confirmation eventually completes.
+  let count = 100;
+  while (count > 0) {
+    if (count == 50 || count == 10) {
+      // At these two points we do a longer timeout to account for a slow
+      // response on the server side. This is usually a problem on the Android
+      // because of the increased delay between the emulator and host.
+      await new Promise(resolve => do_timeout(100 * (100 / count), resolve));
+    }
+    let [, inRecord] = await new DNSListener(
+      `ip${count}.example.org`,
+      undefined,
+      false
+    );
+    inRecord.QueryInterface(Ci.nsIDNSAddrRecord);
+    let responseIP = inRecord.getNextAddrAsString();
+    Assert.ok(true, responseIP);
+    if (responseIP == expectedResponseIP) {
+      break;
+    }
+    count--;
+  }
+
+  if (confirmationShouldFail) {
+    Assert.equal(count, 0, "Confirmation did not finish after 100 iterations");
+    return;
+  }
+
+  Assert.greater(count, 0, "Finished confirmation before 100 iterations");
+}
+
 add_task(async function test0_nodeExecute() {
   // This test checks that moz-http2.js running in node is working.
   // This should always be the first test in this file (except for setup)
@@ -1672,46 +1704,71 @@ add_task(async function test_redirect_post() {
 });
 
 // confirmationNS set without confirmed NS yet
-// checks that we properly fall back to DNS is confirmation is not ready yet
-add_task(async function test_resolve_not_confirmed() {
+// checks that we properly fall back to DNS is confirmation is not ready yet,
+// and wait-for-confirmation pref is true
+add_task(async function test_resolve_not_confirmed_wait_for_confirmation() {
   dns.clearCache(true);
+  Services.prefs.setBoolPref("network.trr.wait-for-confirmation", true);
+  Services.prefs.setIntPref("network.trr.mode", 2); // TRR-first
+  Services.prefs.setCharPref(
+    "network.trr.confirmationNS",
+    "confirm.example.com"
+  );
+  Services.prefs.setCharPref(
+    "network.trr.uri",
+    `https://foo.example.com:${h2Port}/doh?responseIP=7.7.7.7&slowConfirm=true`
+  );
+
+  await new DNSListener("example.org", "127.0.0.1");
+  await new Promise(resolve => do_timeout(1000, resolve));
+  await waitForConfirmation("7.7.7.7");
+
+  Services.prefs.setCharPref("network.trr.confirmationNS", "skip");
+  Services.prefs.clearUserPref("network.trr.wait-for-confirmation");
+});
+
+// checks that we don't fall back to DNS is confirmation is not ready yet, and
+// wait-for-confirmation pref is false
+add_task(async function test_resolve_confirmation_pending() {
+  dns.clearCache(true);
+  Services.prefs.setBoolPref("network.trr.wait-for-confirmation", false);
+  Services.prefs.setIntPref("network.trr.mode", 2); // TRR-first
+  Services.prefs.setCharPref(
+    "network.trr.confirmationNS",
+    "confirm.example.com"
+  );
+  Services.prefs.setCharPref(
+    "network.trr.uri",
+    `https://foo.example.com:${h2Port}/doh?responseIP=7.7.7.7&slowConfirm=true`
+  );
+
+  // DoH available immediately
+  await new DNSListener("example.org", "7.7.7.7");
+
+  Services.prefs.setCharPref("network.trr.confirmationNS", "skip");
+  Services.prefs.clearUserPref("network.trr.wait-for-confirmation");
+});
+
+// checks that we properly fall back to DNS if confirmation failed
+add_task(async function test_resolve_confirm_failed() {
+  dns.clearCache(true);
+  Services.prefs.setBoolPref("network.trr.wait-for-confirmation", true);
   Services.prefs.setIntPref("network.trr.mode", 2); // TRR-first
   Services.prefs.setCharPref(
     "network.trr.uri",
-    `https://foo.example.com:${h2Port}/doh?responseIP=7.7.7.7`
+    `https://foo.example.com:${h2Port}/404`
   );
   Services.prefs.setCharPref(
     "network.trr.confirmationNS",
     "confirm.example.com"
   );
 
+  await waitForConfirmation("7.7.7.7", true);
+
   await new DNSListener("example.org", "127.0.0.1");
 
-  // Check that the confirmation eventually completes.
-  let count = 100;
-  while (count > 0) {
-    if (count == 50 || count == 10) {
-      // At these two points we do a longer timeout to account for a slow
-      // response on the server side. This is usually a problem on the Android
-      // because of the increased delay between the emulator and host.
-      await new Promise(resolve => do_timeout(100 * (100 / count), resolve));
-    }
-    let [, inRecord] = await new DNSListener(
-      `ip${count}.example.org`,
-      undefined,
-      false
-    );
-    inRecord.QueryInterface(Ci.nsIDNSAddrRecord);
-    let responseIP = inRecord.getNextAddrAsString();
-    if (responseIP == "7.7.7.7") {
-      break;
-    }
-    count--;
-  }
-
-  Assert.greater(count, 0, "Finished confirmation before 100 iterations");
-
   Services.prefs.setCharPref("network.trr.confirmationNS", "skip");
+  Services.prefs.clearUserPref("network.trr.wait-for-confirmation");
 });
 
 // Tests that we handle FQDN encoding and decoding properly

testing/xpcshell/moz-http2/moz-http2.js

@@ -782,11 +782,15 @@ function handleRequest(req, res) {
         }
       }
 
-      let delay = undefined;
+      let delay = 0;
       if (packet.questions[0].type == "A") {
-        delay = u.query.delayIPv4;
+        delay = parseInt(u.query.delayIPv4);
       } else if (packet.questions[0].type == "AAAA") {
-        delay = u.query.delayIPv6;
+        delay = parseInt(u.query.delayIPv6);
+      }
+
+      if (u.query.slowConfirm && responseType() == "NS") {
+        delay += 1000;
       }
 
       if (delay) {
@@ -794,7 +798,7 @@ function handleRequest(req, res) {
           arg => {
             writeResponse(arg[0], arg[1]);
           },
-          parseInt(delay),
+          delay,
           [response, buf]
         );
         return;