Bug 1552176 - Captive portal domain should not be automatically excluded from TRR r=mayhemer
☠☠ backed out by 4fb5404dcbef ☠ ☠
authorValentin Gosu <valentin.gosu@gmail.com>
Sun, 10 Nov 2019 17:11:54 +0000
changeset 501427 c8e692a40cd34db3a29a4238257c61da2825f397
parent 501426 68882d1eccac58bde4e2eba08b799c8ea2fbebfd
child 501428 de7aa0eaf4c8a01b0caa4249a8af80c5e36f5e33
push id114170
push usermalexandru@mozilla.com
push dateTue, 12 Nov 2019 21:58:32 +0000
treeherdermozilla-inbound@9e3f44e87a1a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer
bugs1552176
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
netwerk/dns/TRRService.cpp
netwerk/dns/nsHostResolver.cpp
netwerk/test/unit/test_trr.js
--- a/netwerk/dns/TRRService.cpp
+++ b/netwerk/dns/TRRService.cpp
@@ -16,17 +16,16 @@
 #include "mozilla/Preferences.h"
 #include "mozilla/StaticPrefs_network.h"
 #include "mozilla/Tokenizer.h"
 
 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";
 
 #define TRR_PREF_PREFIX "network.trr."
 #define TRR_PREF(x) TRR_PREF_PREFIX x
 
 namespace mozilla {
 namespace net {
@@ -76,17 +75,16 @@ nsresult TRRService::Init() {
     observerService->AddObserver(this, kPurge, true);
     observerService->AddObserver(this, NS_NETWORK_LINK_TOPIC, true);
   }
   nsCOMPtr<nsIPrefBranch> prefBranch;
   GetPrefBranch(getter_AddRefs(prefBranch));
   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 =
       do_GetService(NS_CAPTIVEPORTAL_CID);
   if (captivePortalService) {
     int32_t captiveState;
     MOZ_ALWAYS_SUCCEEDS(captivePortalService->GetState(&captiveState));
 
@@ -307,18 +305,17 @@ nsresult TRRService::ReadPrefs(const cha
   }
   if (!name || !strcmp(name, TRR_PREF("max-fails"))) {
     uint32_t fails;
     if (NS_SUCCEEDED(Preferences::GetUint(TRR_PREF("max-fails"), &fails))) {
       mDisableAfterFails = fails;
     }
   }
   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) {
       nsAutoCString excludedDomains;
       Preferences::GetCString(aPrefName, excludedDomains);
       if (excludedDomains.IsEmpty()) {
         return;
       }
@@ -330,29 +327,16 @@ nsresult TRRService::ReadPrefs(const cha
         LOG(("TRRService::ReadPrefs %s host:[%s]\n", aPrefName, token.get()));
         mExcludedDomains.PutEntry(token);
       }
     };
 
     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)) {
     bool tmp;
     if (NS_SUCCEEDED(Preferences::GetBool(kPrefSkipTRRParentalControl, &tmp))) {
       mSkipTRRWhenParentalControlEnabled = tmp;
     }
   }
--- a/netwerk/dns/nsHostResolver.cpp
+++ b/netwerk/dns/nsHostResolver.cpp
@@ -1437,16 +1437,20 @@ nsresult nsHostResolver::NameLookup(nsHo
     addrRec->mNativeSuccess = false;
     addrRec->mTRRSuccess = 0;
     addrRec->mDidCallbacks = false;
     addrRec->mTrrAUsed = AddrHostRecord::INIT;
     addrRec->mTrrAAAAUsed = AddrHostRecord::INIT;
   }
 
   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);
   }
 
   if (effectiveRequestMode == nsIRequest::TRR_DISABLED_MODE ||
       (effectiveRequestMode == nsIRequest::TRR_FIRST_MODE &&
        (rec->flags & RES_DISABLE_TRR) && NS_FAILED(rv))) {
--- a/netwerk/test/unit/test_trr.js
+++ b/netwerk/test/unit/test_trr.js
@@ -868,25 +868,71 @@ add_task(async function test24e() {
   dns.clearCache(true);
   Services.prefs.setCharPref(
     "network.trr.excluded-domains",
     "bar.example.com, foo.test.com"
   );
   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.
 add_task(async function test24g() {
   dns.clearCache(true);
   await SetParentalControlEnabled(true);
   await new DNSListener("www.example.com", "127.0.0.1");
   await SetParentalControlEnabled(false);
@@ -990,25 +1036,55 @@ add_task(async function test25d() {
   await new DNSListener("domain.other", "127.0.0.1");
 });
 
 // TRR-only check that captivedetect.canonicalURL is resolved via native DNS
 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.
 add_task(async function test25f() {
   dns.clearCache(true);
   Services.prefs.setIntPref("network.trr.mode", 3); // TRR-only
   await SetParentalControlEnabled(true);
   await new DNSListener("www.example.com", "127.0.0.1");