Bug 1552176 - Captive portal domain should not be automatically excluded from TRR r=mayhemer

Previously we had no way from excluding just one channel from TRR mode3.
The solution was to add the captive portal domain to the exclusion list.
Now the captive portal channel is marked with nsIRequest.DISABLE_TRR_MODE so
the exclusion is not necessary anymore.

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

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

netwerk/dns/TRRService.cpp

@@ -21,7 +21,6 @@ static const char kOpenCaptivePortalLoginEvent[] = "captive-portal-login";
 static const char kClearPrivateData[] = "clear-private-data";
 static const char kPurge[] = "browser:purge-session-history";
 static const char kDisableIpv6Pref[] = "network.dns.disableIPv6";
-static const char kCaptivedetectCanonicalURL[] = "captivedetect.canonicalURL";
 static const char kPrefSkipTRRParentalControl[] =
     "network.dns.skipTRR-when-parental-control-enabled";
 
@@ -81,7 +80,6 @@ nsresult TRRService::Init() {
   if (prefBranch) {
     prefBranch->AddObserver(TRR_PREF_PREFIX, this, true);
     prefBranch->AddObserver(kDisableIpv6Pref, this, true);
-    prefBranch->AddObserver(kCaptivedetectCanonicalURL, this, true);
     prefBranch->AddObserver(kPrefSkipTRRParentalControl, this, true);
   }
   nsCOMPtr<nsICaptivePortalService> captivePortalService =
@@ -312,8 +310,7 @@ nsresult TRRService::ReadPrefs(const char* name) {
     }
   }
   if (!name || !strcmp(name, TRR_PREF("excluded-domains")) ||
-      !strcmp(name, TRR_PREF("builtin-excluded-domains")) ||
-      !strcmp(name, kCaptivedetectCanonicalURL)) {
+      !strcmp(name, TRR_PREF("builtin-excluded-domains"))) {
     mExcludedDomains.Clear();
 
     auto parseExcludedDomains = [this](const char* aPrefName) {
@@ -335,19 +332,6 @@ nsresult TRRService::ReadPrefs(const char* name) {
     parseExcludedDomains(TRR_PREF("excluded-domains"));
     parseExcludedDomains(TRR_PREF("builtin-excluded-domains"));
     clearEntireCache = true;
-
-    nsAutoCString canonicalSiteURL;
-    Preferences::GetCString(kCaptivedetectCanonicalURL, canonicalSiteURL);
-
-    nsCOMPtr<nsIURI> uri;
-    nsresult rv = NS_NewURI(getter_AddRefs(uri), canonicalSiteURL,
-                            UTF_8_ENCODING, nullptr);
-    if (NS_SUCCEEDED(rv)) {
-      nsAutoCString host;
-      uri->GetHost(host);
-      LOG(("TRRService::ReadPrefs captive portal URL:[%s]\n", host.get()));
-      mExcludedDomains.PutEntry(host);
-    }
   }
 
   if (!name || !strcmp(name, kPrefSkipTRRParentalControl)) {

netwerk/dns/nsHostResolver.cpp

@@ -1442,6 +1442,10 @@ nsresult nsHostResolver::NameLookup(nsHostRecord* rec) {
   }
 
   nsIRequest::TRRMode effectiveRequestMode = rec->EffectiveTRRMode();
+
+  LOG(("NameLookup: %s effectiveTRRmode: %d", rec->host.get(),
+       effectiveRequestMode));
+
   if (effectiveRequestMode != nsIRequest::TRR_DISABLED_MODE &&
       !((rec->flags & RES_DISABLE_TRR))) {
     rv = TrrLookup(rec);

netwerk/test/unit/test_trr.js

@@ -874,15 +874,61 @@ add_task(async function test24e() {
   await new DNSListener("bar.example.com", "127.0.0.1");
 });
 
+function observerPromise(topic) {
+  return new Promise(resolve => {
+    let observer = {
+      QueryInterface: ChromeUtils.generateQI([Ci.nsIObserver]),
+      observe(aSubject, aTopic, aData) {
+        dump(`observe: ${aSubject}, ${aTopic}, ${aData} \n`);
+        if (aTopic == topic) {
+          Services.obs.removeObserver(observer, topic);
+          resolve(aData);
+        }
+      },
+    };
+    Services.obs.addObserver(observer, topic);
+  });
+}
+
 // TRR-first check that captivedetect.canonicalURL is resolved via native DNS
 add_task(async function test24f() {
   dns.clearCache(true);
+
+  const cpServer = new HttpServer();
+  cpServer.registerPathHandler("/cp", function handleRawData(
+    request,
+    response
+  ) {
+    response.setHeader("Content-Type", "text/plain", false);
+    response.setHeader("Cache-Control", "no-cache", false);
+    response.bodyOutputStream.write("data", 4);
+  });
+  cpServer.start(-1);
+  cpServer.identity.setPrimary(
+    "http",
+    "detectportal.firefox.com",
+    cpServer.identity.primaryPort
+  );
+  let cpPromise = observerPromise("captive-portal-login");
+
   Services.prefs.setCharPref(
     "captivedetect.canonicalURL",
-    "http://test.detectportal.com/success.txt"
+    `http://detectportal.firefox.com:${cpServer.identity.primaryPort}/cp`
   );
+  Services.prefs.setBoolPref("network.captive-portal-service.testMode", true);
+  Services.prefs.setBoolPref("network.captive-portal-service.enabled", true);
 
-  await new DNSListener("test.detectportal.com", "127.0.0.1");
+  // The captive portal has to have used native DNS, otherwise creating
+  // a socket to a non-local IP would trigger a crash.
+  await cpPromise;
+  // Simply resolving the captive portal domain should still use TRR
+  await new DNSListener("detectportal.firefox.com", "192.192.192.192");
+
+  Services.prefs.clearUserPref("network.captive-portal-service.enabled");
+  Services.prefs.clearUserPref("network.captive-portal-service.testMode");
+  Services.prefs.clearUserPref("captivedetect.canonicalURL");
+
+  await new Promise(resolve => cpServer.stop(resolve));
 });
 
 // TRR-first check that a domain is resolved via native DNS when parental control is enabled.
@@ -995,16 +1041,46 @@ add_task(async function test25d() {
 add_task(async function test25e() {
   dns.clearCache(true);
   Services.prefs.setIntPref("network.trr.mode", 3); // TRR-only
-  Services.prefs.setCharPref(
-    "captivedetect.canonicalURL",
-    "http://test.detectportal.com/success.txt"
-  );
   Services.prefs.setCharPref(
     "network.trr.uri",
     `https://foo.example.com:${h2Port}/doh?responseIP=192.192.192.192`
   );
 
-  await new DNSListener("test.detectportal.com", "127.0.0.1");
+  const cpServer = new HttpServer();
+  cpServer.registerPathHandler("/cp", function handleRawData(
+    request,
+    response
+  ) {
+    response.setHeader("Content-Type", "text/plain", false);
+    response.setHeader("Cache-Control", "no-cache", false);
+    response.bodyOutputStream.write("data", 4);
+  });
+  cpServer.start(-1);
+  cpServer.identity.setPrimary(
+    "http",
+    "detectportal.firefox.com",
+    cpServer.identity.primaryPort
+  );
+  let cpPromise = observerPromise("captive-portal-login");
+
+  Services.prefs.setCharPref(
+    "captivedetect.canonicalURL",
+    `http://detectportal.firefox.com:${cpServer.identity.primaryPort}/cp`
+  );
+  Services.prefs.setBoolPref("network.captive-portal-service.testMode", true);
+  Services.prefs.setBoolPref("network.captive-portal-service.enabled", true);
+
+  // The captive portal has to have used native DNS, otherwise creating
+  // a socket to a non-local IP would trigger a crash.
+  await cpPromise;
+  // // Simply resolving the captive portal domain should still use TRR
+  await new DNSListener("detectportal.firefox.com", "192.192.192.192");
+
+  Services.prefs.clearUserPref("network.captive-portal-service.enabled");
+  Services.prefs.clearUserPref("network.captive-portal-service.testMode");
+  Services.prefs.clearUserPref("captivedetect.canonicalURL");
+
+  await new Promise(resolve => cpServer.stop(resolve));
 });
 
 // TRR-only check that a domain is resolved via native DNS when parental control is enabled.