Backed out 2 changesets (bug 1743022) for causing xpc failures in test_trr_blocklist. CLOSED TREE

Backed out changeset 1acf0c8e8663 (bug 1743022)
Backed out changeset e8822e38828f (bug 1743022)

modules/libpref/init/StaticPrefList.yaml

@@ -10149,13 +10149,6 @@
   value: true
   mirror: always
 
-# TRR blocklist entry expire time (in seconds). Default is one minute.
-# Meant to survive basically a page load.
-- name: network.trr.temp_blocklist_duration_sec
-  type: RelaxedAtomicUint32
-  value: 60
-  mirror: always
-
 # Single TRR request timeout, in milliseconds
 - name: network.trr.request_timeout_ms
   type: RelaxedAtomicUint32

modules/libpref/init/all.js

@@ -3949,6 +3949,9 @@ pref("network.trr.custom_uri", "");
 // Before TRR is widely used the NS record for this host is fetched
 // from the DOH end point to ensure proper configuration
 pref("network.trr.confirmationNS", "example.com");
+// TRR blacklist entry expire time (in seconds). Default is one minute.
+// Meant to survive basically a page load.
+pref("network.trr.blacklist-duration", 60);
 // Comma separated list of domains that we should not use TRR for
 pref("network.trr.excluded-domains", "");
 pref("network.trr.builtin-excluded-domains", "localhost,local");

netwerk/dns/DNSPacket.cpp

@@ -1014,10 +1014,6 @@ nsresult DNSPacket::DecodeInternal(
     return NS_ERROR_ILLEGAL_VALUE;
   }
 
-  if (aType == TRRTYPE_NS && rcode != 0) {
-    return NS_ERROR_UNKNOWN_HOST;
-  }
-
   if ((aType != TRRTYPE_NS) && aCname.IsEmpty() && aResp.mAddresses.IsEmpty() &&
       aTypeResult.is<TypeRecordEmpty>()) {
     // no entries were stored!

netwerk/dns/TRR.h

@@ -95,7 +95,7 @@ class TRR : public Runnable,
   // FailData() must be called to signal that the asynch TRR resolve is
   // completed. For failed name resolves ("no such host"), the 'error' it
   // passses on in its argument must be NS_ERROR_UNKNOWN_HOST. Other errors
-  // (if host was blocklisted, there as a bad content-type received, etc)
+  // (if host was blacklisted, there as a bad content-type received, etc)
   // other error codes must be used. This distinction is important for the
   // subsequent logic to separate the error reasons.
   nsresult FailData(nsresult error);

netwerk/dns/TRRService.cpp

@@ -35,7 +35,8 @@ static const char kDisableIpv6Pref[] = "network.dns.disableIPv6";
 #define TRR_PREF_PREFIX "network.trr."
 #define TRR_PREF(x) TRR_PREF_PREFIX x
 
-namespace mozilla::net {
+namespace mozilla {
+namespace net {
 
 StaticRefPtr<nsIThread> sTRRBackgroundThread;
 static Atomic<TRRService*> sTRRServicePtr;
@@ -355,6 +356,14 @@ nsresult TRRService::ReadPrefs(const char* name) {
     Preferences::GetCString(TRR_PREF("bootstrapAddr"), mBootstrapAddr);
     clearEntireCache = true;
   }
+  if (!name || !strcmp(name, TRR_PREF("blacklist-duration"))) {
+    // prefs is given in number of seconds
+    uint32_t secs;
+    if (NS_SUCCEEDED(
+            Preferences::GetUint(TRR_PREF("blacklist-duration"), &secs))) {
+      mBlocklistDurationSeconds = secs;
+    }
+  }
   if (!name || !strcmp(name, kDisableIpv6Pref)) {
     bool tmp;
     if (NS_SUCCEEDED(Preferences::GetBool(kDisableIpv6Pref, &tmp))) {
@@ -842,8 +851,7 @@ bool TRRService::IsDomainBlocked(const nsACString& aHost,
   // use a unified casing for the hashkey
   nsAutoCString hashkey(aHost + aOriginSuffix);
   if (auto val = bl->Lookup(hashkey)) {
-    int32_t until =
-        *val + int32_t(StaticPrefs::network_trr_temp_blocklist_duration_sec());
+    int32_t until = *val + mBlocklistDurationSeconds;
     int32_t expire = NowInSeconds();
     if (until > expire) {
       LOG(("Host [%s] is TRR blocklisted\n", nsCString(aHost).get()));
@@ -1306,4 +1314,5 @@ void TRRService::InitTRRConnectionInfo() {
   }
 }
 
-}  // namespace mozilla::net
+}  // namespace net
+}  // namespace mozilla

netwerk/dns/TRRService.h

@@ -119,6 +119,8 @@ class TRRService : public TRRServiceBase,
   void AddEtcHosts(const nsTArray<nsCString>&);
 
   bool mInitialized{false};
+  Atomic<uint32_t, Relaxed> mBlocklistDurationSeconds{60};
+
   Mutex mLock{"TRRService"};
 
   nsCString mPrivateCred;  // main thread only

netwerk/docs/dns/dns-over-https-trr.rst

@@ -46,7 +46,7 @@ a DoH or a Do53 request. First it checks the effective TRR mode of the request
 is as requests could have a different mode from the global one.
 If the request may use TRR, then we dispatch a request in nsHostResolver::TrrLookup.
 Since we usually reolve both IPv4 and IPv6 names, a **TRRQuery** object is
-created to perform and combine both responses.
+created to perform and combine both responses. 
 
 Once done, nsHostResolver::CompleteLookup is called. If the DoH server returned a
 valid response we use it, otherwise we report a failure in TRR-only mode, or
@@ -62,9 +62,7 @@ main thread.
 Dynamic Blocklist
 -----------------
 
-In order to improve performance TRR service manages a dynamic blocklist for host names that can't be resolved with DoH but work with the native resolver. Blocklisted entries will not be retried over DoH for one minute (See `network.trr.temp_blocklist_duration_sec` pref).
-When a domain is added to the blocklist, we also check if there is an NS record for its parent domain, in which case we add that to the blocklist.
-This feature is controlled by the `network.trr.temp_blocklist` pref.
+In order to improve performance TRR service manages a dynamic persistent blocklist for host names that can't be resolved with DoH but works with the native resolver. Blocklisted entries will not be retried over DoH for one minute.
 
 TRR confirmation
 ----------------

netwerk/test/unit/head_trr.js

@@ -81,7 +81,7 @@ function trr_clear_prefs() {
   Services.prefs.clearUserPref("network.trr.useGET");
   Services.prefs.clearUserPref("network.trr.confirmationNS");
   Services.prefs.clearUserPref("network.trr.bootstrapAddr");
-  Services.prefs.clearUserPref("network.trr.temp_blocklist_duration_sec");
+  Services.prefs.clearUserPref("network.trr.blacklist-duration");
   Services.prefs.clearUserPref("network.trr.request_timeout_ms");
   Services.prefs.clearUserPref("network.trr.request_timeout_mode_trronly_ms");
   Services.prefs.clearUserPref("network.trr.disable-ECS");

netwerk/test/unit/test_trr_blocklist.js

@@ -1,81 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-"use strict";
-
-const dns = Cc["@mozilla.org/network/dns-service;1"].getService(
-  Ci.nsIDNSService
-);
-const override = Cc["@mozilla.org/network/native-dns-override;1"].getService(
-  Ci.nsINativeDNSResolverOverride
-);
-
-function setup() {
-  trr_test_setup();
-  Services.prefs.setBoolPref("network.trr.temp_blocklist", true);
-}
-setup();
-
-add_task(async function checkBlocklisting() {
-  let trrServer = new TRRServer();
-  registerCleanupFunction(async () => {
-    await trrServer.stop();
-  });
-  await trrServer.start();
-  info(`port = ${trrServer.port}\n`);
-
-  dns.clearCache(true);
-  Services.prefs.setCharPref(
-    "network.trr.uri",
-    `https://foo.example.com:${trrServer.port}/dns-query`
-  );
-  Services.prefs.setIntPref("network.trr.mode", Ci.nsIDNSService.MODE_TRRFIRST);
-
-  await trrServer.registerDoHAnswers("top.test.com", "NS", {});
-
-  override.addIPOverride("sub.top.test.com", "2.2.2.2");
-  override.addIPOverride("sub2.top.test.com", "2.2.2.2");
-  await new TRRDNSListener("sub.top.test.com", {
-    expectedAnswer: "2.2.2.2",
-  });
-  equal(await trrServer.requestCount("sub.top.test.com", "A"), 1);
-
-  // Clear the cache so that we need to consult the blocklist and not simply
-  // return the cached DNS record.
-  dns.clearCache(true);
-  await new TRRDNSListener("sub.top.test.com", {
-    expectedAnswer: "2.2.2.2",
-  });
-  equal(
-    await trrServer.requestCount("sub.top.test.com", "A"),
-    1,
-    "Request should go directly to native because result is still in blocklist"
-  );
-
-  // XXX(valentin): if this ever starts intermittently failing we need to add
-  // a sleep here. But the check for the parent NS should normally complete
-  // before the second subdomain request.
-  equal(
-    await trrServer.requestCount("top.test.com", "NS"),
-    1,
-    "Should have checked parent domain"
-  );
-  await new TRRDNSListener("sub2.top.test.com", {
-    expectedAnswer: "2.2.2.2",
-  });
-  equal(await trrServer.requestCount("sub2.top.test.com", "A"), 0);
-
-  // The blocklist should instantly expire.
-  Services.prefs.setIntPref("network.trr.temp_blocklist_duration_sec", 0);
-  dns.clearCache(true);
-  await new TRRDNSListener("sub.top.test.com", {
-    expectedAnswer: "2.2.2.2",
-  });
-  // blocklist expired. Do another check.
-  equal(
-    await trrServer.requestCount("sub.top.test.com", "A"),
-    2,
-    "We should do another TRR request because the bloclist expired"
-  );
-});

netwerk/test/unit/xpcshell.ini

@@ -595,4 +595,3 @@ skip-if = os == "android"
 head = head_channels.js head_cache.js head_cookies.js head_trr.js trr_common.js
 skip-if = os == "android"
 run-sequentially = node server exceptions dont replay well
-[test_trr_blocklist.js]

netwerk/test/unit_ipc/test_trr_httpssvc_wrap.js

@@ -61,7 +61,7 @@ registerCleanupFunction(() => {
   prefs.clearUserPref("network.trr.useGET");
   prefs.clearUserPref("network.trr.confirmationNS");
   prefs.clearUserPref("network.trr.bootstrapAddr");
-  prefs.clearUserPref("network.trr.temp_blocklist_duration_sec");
+  prefs.clearUserPref("network.trr.blacklist-duration");
   prefs.clearUserPref("network.trr.request-timeout");
   prefs.clearUserPref("network.trr.clear-cache-on-pref-change");
 });

tools/lint/rejected-words.yml

@@ -215,9 +215,15 @@ avoid-blacklist-and-whitelist:
         - netwerk/base/nsURLHelper.cpp
         - netwerk/cookie/CookieCommons.h
         - netwerk/dns/nsHostRecord.cpp
+        - netwerk/dns/nsHostResolver.cpp
         - netwerk/dns/nsIDNService.cpp
         - netwerk/dns/nsIDNService.h
         - netwerk/dns/TRR.cpp
+        - netwerk/dns/TRR.h
+        - netwerk/dns/TRRServiceChild.cpp
+        - netwerk/dns/TRRService.cpp
+        - netwerk/dns/TRRService.h
+        - netwerk/dns/TRRServiceParent.cpp
         - netwerk/ipc/DocumentLoadListener.cpp
         - netwerk/protocol/about/nsAboutProtocolHandler.cpp
         - netwerk/protocol/http/Http2Session.cpp
@@ -234,13 +240,21 @@ avoid-blacklist-and-whitelist:
         - netwerk/protocol/websocket/BaseWebSocketChannel.cpp
         - netwerk/socket/nsSOCKSSocketProvider.cpp
         - netwerk/test/gtest/TestCookie.cpp
+        - netwerk/test/unit/head_trr.js
+        - netwerk/test/unit_ipc/test_dns_by_type_resolve_wrap.js
+        - netwerk/test/unit_ipc/test_trr_httpssvc_wrap.js
         - netwerk/test/unit/test_bug396389.js
         - netwerk/test/unit/test_bug427957.js
         - netwerk/test/unit/test_bug464591.js
         - netwerk/test/unit/test_bug479413.js
         - netwerk/test/unit/test_cookie_blacklist.js
+        - netwerk/test/unit/test_dns_by_type_resolve.js
         - netwerk/test/unit/test_idn_blacklist.js
         - netwerk/test/unit/test_idn_urls.js
+        - netwerk/test/unit/test_odoh.js
+        - netwerk/test/unit/test_trr_httpssvc.js
+        - netwerk/test/unit/test_trr.js
+        - netwerk/test/unit/test_use_httpssvc.js
         - netwerk/url-classifier/AsyncUrlChannelClassifier.cpp
         - netwerk/url-classifier/nsChannelClassifier.cpp
         - netwerk/url-classifier/nsChannelClassifier.h