Bug 1931349 - Enable network.dns.https_rr.check_record_with_cname and add probes for the outcome, r=necko-reviewers,valentin

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

modules/libpref/init/StaticPrefList.yaml

@@ -14283,7 +14283,7 @@
 # If not, the record will not be used.
 - name: network.dns.https_rr.check_record_with_cname
   type: RelaxedAtomicBool
-  value: false
+  value: @IS_EARLY_BETA_OR_EARLIER@
   mirror: always
 
 # This preference can be used to turn off IPv6 name lookups. See bug 68796.

netwerk/dns/HTTPSSVC.cpp

@@ -4,6 +4,7 @@
 
 #include "HTTPSSVC.h"
 #include "mozilla/net/DNS.h"
+#include "mozilla/glean/GleanMetrics.h"
 #include "mozilla/StaticPrefs_network.h"
 #include "nsHttp.h"
 #include "nsHttpHandler.h"
@@ -359,6 +360,12 @@ static nsTArray<SVCBWrapper> FlattenRecords(const nsTArray<SVCB>& aRecords) {
   return result;
 }
 
+static void TelemetryForServiceModeRecord(const nsACString& aKey) {
+#ifndef ANDROID
+  glean::networking::https_record_state.Get(aKey).Add(1);
+#endif
+}
+
 already_AddRefed<nsISVCBRecord>
 DNSHTTPSSVCRecordBase::GetServiceModeRecordInternal(
     bool aNoHttp2, bool aNoHttp3, const nsTArray<SVCB>& aRecords,
@@ -368,6 +375,7 @@ DNSHTTPSSVCRecordBase::GetServiceModeRecordInternal(
   RefPtr<SVCBRecord> h3RecordWithEchConfig;
   uint32_t recordHasNoDefaultAlpnCount = 0;
   uint32_t recordExcludedCount = 0;
+  uint32_t recordHasUnmatchedCname = 0;
   aRecordsAllExcluded = false;
   nsCOMPtr<nsIDNSService> dns = do_GetService(NS_DNSSERVICE_CONTRACTID);
   uint32_t h3ExcludedCount = 0;
@@ -376,6 +384,7 @@ DNSHTTPSSVCRecordBase::GetServiceModeRecordInternal(
   for (const auto& record : records) {
     if (record.mRecord.mSvcFieldPriority == 0) {
       // In ServiceMode, the SvcPriority should never be 0.
+      TelemetryForServiceModeRecord("invalid"_ns);
       return nullptr;
     }
 
@@ -389,6 +398,7 @@ DNSHTTPSSVCRecordBase::GetServiceModeRecordInternal(
     }
 
     if (!CheckRecordIsUsableWithCname(record.mRecord, aCname)) {
+      recordHasUnmatchedCname++;
       continue;
     }
 
@@ -427,11 +437,18 @@ DNSHTTPSSVCRecordBase::GetServiceModeRecordInternal(
   if (!selectedRecord && !h3RecordWithEchConfig) {
     // If all records indicate "no-default-alpn", we should not use this RRSet.
     if (recordHasNoDefaultAlpnCount == records.Length()) {
+      TelemetryForServiceModeRecord("no_default_alpn"_ns);
       return nullptr;
     }
 
     if (recordExcludedCount == records.Length()) {
       aRecordsAllExcluded = true;
+      TelemetryForServiceModeRecord("all_excluded"_ns);
+      return nullptr;
+    }
+
+    if (recordHasUnmatchedCname == records.Length()) {
+      TelemetryForServiceModeRecord("unmatched_cname"_ns);
       return nullptr;
     }
 
@@ -444,6 +461,7 @@ DNSHTTPSSVCRecordBase::GetServiceModeRecordInternal(
   }
 
   if (h3RecordWithEchConfig) {
+    TelemetryForServiceModeRecord("succeeded"_ns);
     if (selectedRecord && selectedRecord->mData.mHasEchConfig) {
       return selectedRecord.forget();
     }
@@ -451,6 +469,11 @@ DNSHTTPSSVCRecordBase::GetServiceModeRecordInternal(
     return h3RecordWithEchConfig.forget();
   }
 
+  if (selectedRecord) {
+    TelemetryForServiceModeRecord("succeeded"_ns);
+  } else {
+    TelemetryForServiceModeRecord("others"_ns);
+  }
   return selectedRecord.forget();
 }
 

netwerk/protocol/http/metrics.yaml

@@ -982,6 +982,32 @@ networking:
     expires: never
     telemetry_mirror: NETWORKING_DATA_TRANSFERRED_V3_KB
 
+  https_record_state:
+    type: labeled_counter
+    description: >
+      Gather the outcome of checking if a HTTPS record can be used:
+      - "invalid"
+      - "succeeded"
+      - "unmatched_cname"
+      - "all_excluded"
+      - "no_default_alpn"
+      - "others"
+    bugs:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1931349
+    data_reviews:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1931349
+    notification_emails:
+      - necko@mozilla.com
+      - kershaw@mozilla.com
+    expires: never
+    labels:
+      - invalid
+      - succeeded
+      - unmatched_cname
+      - all_excluded
+      - no_default_alpn
+      - others
+
 opaque.response.blocking:
   javascript_validation_count:
     type: counter

netwerk/test/unit/test_http3_fast_fallback.js

@@ -38,6 +38,10 @@ add_setup(async function setup() {
   Services.prefs.setIntPref("network.trr.mode", 2); // TRR first
   Services.prefs.setBoolPref("network.http.http3.enable", true);
   Services.prefs.setIntPref("network.http.speculative-parallel-limit", 6);
+  Services.prefs.setBoolPref(
+    "network.dns.https_rr.check_record_with_cname",
+    false
+  );
 
   registerCleanupFunction(async () => {
     trr_clear_prefs();
@@ -61,6 +65,9 @@ add_setup(async function setup() {
     Services.prefs.clearUserPref(
       "network.http.http3.parallel_fallback_conn_limit"
     );
+    Services.prefs.clearUserPref(
+      "network.dns.https_rr.check_record_with_cname"
+    );
     if (trrServer) {
       await trrServer.stop();
     }

netwerk/test/unit/test_httpssvc_https_upgrade.js

@@ -41,6 +41,10 @@ add_setup(async function setup() {
 
   Services.prefs.setBoolPref("network.dns.upgrade_with_https_rr", true);
   Services.prefs.setBoolPref("network.dns.use_https_rr_as_altsvc", true);
+  Services.prefs.setBoolPref(
+    "network.dns.https_rr.check_record_with_cname",
+    false
+  );
 
   Services.prefs.setBoolPref(
     "network.dns.use_https_rr_for_speculative_connection",
@@ -57,6 +61,9 @@ add_setup(async function setup() {
     Services.prefs.clearUserPref("network.dns.notifyResolution");
     Services.prefs.clearUserPref("network.dns.disablePrefetch");
     Services.prefs.clearUserPref("dom.security.https_first_for_custom_ports");
+    Services.prefs.clearUserPref(
+      "network.dns.https_rr.check_record_with_cname"
+    );
   });
 
   if (mozinfo.socketprocess_networking) {

netwerk/test/unit/test_httpssvc_iphint.js

@@ -23,12 +23,19 @@ add_setup(async function setup() {
 
   Services.prefs.setBoolPref("network.dns.upgrade_with_https_rr", true);
   Services.prefs.setBoolPref("network.dns.use_https_rr_as_altsvc", true);
+  Services.prefs.setBoolPref(
+    "network.dns.https_rr.check_record_with_cname",
+    false
+  );
 
   registerCleanupFunction(async () => {
     trr_clear_prefs();
     Services.prefs.clearUserPref("network.dns.upgrade_with_https_rr");
     Services.prefs.clearUserPref("network.dns.use_https_rr_as_altsvc");
     Services.prefs.clearUserPref("network.dns.disablePrefetch");
+    Services.prefs.clearUserPref(
+      "network.dns.https_rr.check_record_with_cname"
+    );
     await trrServer.stop();
   });
 

netwerk/test/unit/test_trr_https_fallback.js

@@ -35,6 +35,10 @@ add_setup(async function setup() {
   Services.prefs.setBoolPref("network.dns.upgrade_with_https_rr", true);
   Services.prefs.setBoolPref("network.dns.use_https_rr_as_altsvc", true);
   Services.prefs.setBoolPref("network.dns.echconfig.enabled", true);
+  Services.prefs.setBoolPref(
+    "network.dns.https_rr.check_record_with_cname",
+    false
+  );
 
   registerCleanupFunction(async () => {
     trr_clear_prefs();
@@ -52,6 +56,9 @@ add_setup(async function setup() {
     Services.prefs.clearUserPref("network.http.speculative-parallel-limit");
     Services.prefs.clearUserPref("network.dns.localDomains");
     Services.prefs.clearUserPref("network.dns.http3_echconfig.enabled");
+    Services.prefs.clearUserPref(
+      "network.dns.https_rr.check_record_with_cname"
+    );
     if (trrServer) {
       await trrServer.stop();
     }

netwerk/test/unit/test_use_httpssvc.js

@@ -22,11 +22,18 @@ add_setup(async function setup() {
 
   Services.prefs.setBoolPref("network.dns.upgrade_with_https_rr", true);
   Services.prefs.setBoolPref("network.dns.use_https_rr_as_altsvc", true);
+  Services.prefs.setBoolPref(
+    "network.dns.https_rr.check_record_with_cname",
+    false
+  );
 
   registerCleanupFunction(() => {
     trr_clear_prefs();
     Services.prefs.clearUserPref("network.dns.upgrade_with_https_rr");
     Services.prefs.clearUserPref("network.dns.use_https_rr_as_altsvc");
+    Services.prefs.clearUserPref(
+      "network.dns.https_rr.check_record_with_cname"
+    );
   });
 
   if (mozinfo.socketprocess_networking) {

testing/mozbase/mozprofile/mozprofile/permissions.py

@@ -244,6 +244,9 @@ class Permissions(object):
                 user_prefs.append(("network.trr.uri", trrUri))
                 user_prefs.append(("network.trr.bootstrapAddr", "127.0.0.1"))
                 user_prefs.append(("network.dns.force_use_https_rr", True))
+                user_prefs.append(
+                    ("network.dns.https_rr.check_record_with_cname", False)
+                )
             else:
                 user_prefs = self.pac_prefs(proxy)
         else:

testing/mozbase/mozprofile/tests/test_permissions.py

@@ -52,12 +52,13 @@ def test_nw_prefs(perms):
 
     prefs, user_prefs = perms.network_prefs({"dohServerPort": 443})
     print(user_prefs)
-    assert len(user_prefs) == 5
+    assert len(user_prefs) == 6
     assert user_prefs[0] == ("network.proxy.type", 0)
     assert user_prefs[1] == ("network.trr.mode", 3)
     assert user_prefs[2] == ("network.trr.uri", "https://foo.example.com:443/dns-query")
     assert user_prefs[3] == ("network.trr.bootstrapAddr", "127.0.0.1")
     assert user_prefs[4] == ("network.dns.force_use_https_rr", True)
+    assert user_prefs[5] == ("network.dns.https_rr.check_record_with_cname", False)
 
 
 if __name__ == "__main__":

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

@@ -1168,7 +1168,7 @@ function handleRequest(req, res) {
           flush: false,
           data: {
             priority,
-            name: "foo.example.com",
+            name: packet.questions[0].name,
             values: [
               { key: "alpn", value: "h2" },
               { key: "port", value: serverPort },