Bug 1750413 - Give http3 and http2 more priority when selecting alpn, r=necko-reviewers,dragana

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

modules/libpref/init/StaticPrefList.yaml

@@ -10644,12 +10644,6 @@
   value: 50
   mirror: always
 
-# Whether to use https rr for speculative connections.
-- name: network.dns.use_https_rr_for_speculative_connection
-  type: RelaxedAtomicBool
-  value: false
-  mirror: always
-
 # Whether to force a transaction to wait https rr.
 - name: network.dns.force_waiting_https_rr
   type: RelaxedAtomicBool

netwerk/dns/HTTPSSVC.cpp

@@ -200,18 +200,33 @@ void SVCB::GetIPHints(CopyableTArray<mozilla::net::NetAddr>& aAddresses) const {
   }
 }
 
-nsTArray<nsCString> SVCB::GetAllAlpn() const {
-  nsTArray<nsCString> alpnList;
+class AlpnComparator {
+ public:
+  bool Equals(const Tuple<nsCString, SupportedAlpnRank>& aA,
+              const Tuple<nsCString, SupportedAlpnRank>& aB) const {
+    return Get<1>(aA) == Get<1>(aB);
+  }
+  bool LessThan(const Tuple<nsCString, SupportedAlpnRank>& aA,
+                const Tuple<nsCString, SupportedAlpnRank>& aB) const {
+    return Get<1>(aA) > Get<1>(aB);
+  }
+};
+
+nsTArray<Tuple<nsCString, SupportedAlpnRank>> SVCB::GetAllAlpn() const {
+  nsTArray<Tuple<nsCString, SupportedAlpnRank>> alpnList;
   for (const auto& value : mSvcFieldValue) {
     if (value.mValue.is<SvcParamAlpn>()) {
-      alpnList.AppendElements(value.mValue.as<SvcParamAlpn>().mValue);
+      for (const auto& alpn : value.mValue.as<SvcParamAlpn>().mValue) {
+        alpnList.AppendElement(MakeTuple(alpn, IsAlpnSupported(alpn)));
+      }
     }
   }
+  alpnList.Sort(AlpnComparator());
   return alpnList;
 }
 
 SVCBRecord::SVCBRecord(const SVCB& data,
-                       Maybe<Tuple<nsCString, SupportedAlpnType>> aAlpn)
+                       Maybe<Tuple<nsCString, SupportedAlpnRank>> aAlpn)
     : mData(data), mAlpn(aAlpn) {
   mPort = mData.GetPort();
 }
@@ -228,7 +243,7 @@ NS_IMETHODIMP SVCBRecord::GetName(nsACString& aName) {
 
 Maybe<uint16_t> SVCBRecord::GetPort() { return mPort; }
 
-Maybe<Tuple<nsCString, SupportedAlpnType>> SVCBRecord::GetAlpn() {
+Maybe<Tuple<nsCString, SupportedAlpnRank>> SVCBRecord::GetAlpn() {
   return mAlpn;
 }
 
@@ -287,21 +302,21 @@ static bool CheckRecordIsUsable(const SVCB& aRecord, nsIDNSService* aDNSService,
   return true;
 }
 
-static bool CheckAlpnIsUsable(SupportedAlpnType aAlpnType, bool aNoHttp2,
+static bool CheckAlpnIsUsable(SupportedAlpnRank aAlpnType, bool aNoHttp2,
                               bool aNoHttp3, bool aCheckHttp3ExcludedList,
                               const nsACString& aTargetName,
                               uint32_t& aExcludedCount) {
   // Skip if this alpn is not supported.
-  if (aAlpnType == SupportedAlpnType::NOT_SUPPORTED) {
+  if (aAlpnType == SupportedAlpnRank::NOT_SUPPORTED) {
     return false;
   }
 
   // Skip if we don't want to use http2.
-  if (aNoHttp2 && aAlpnType == SupportedAlpnType::HTTP_2) {
+  if (aNoHttp2 && aAlpnType == SupportedAlpnRank::HTTP_2) {
     return false;
   }
 
-  if (aAlpnType == SupportedAlpnType::HTTP_3) {
+  if (IsHttp3(aAlpnType)) {
     if (aCheckHttp3ExcludedList && gHttpHandler->IsHttp3Excluded(aTargetName)) {
       aExcludedCount++;
       return false;
@@ -318,13 +333,14 @@ static bool CheckAlpnIsUsable(SupportedAlpnType aAlpnType, bool aNoHttp2,
 static nsTArray<SVCBWrapper> FlattenRecords(const nsTArray<SVCB>& aRecords) {
   nsTArray<SVCBWrapper> result;
   for (const auto& record : aRecords) {
-    nsTArray<nsCString> alpnList = record.GetAllAlpn();
+    nsTArray<Tuple<nsCString, SupportedAlpnRank>> alpnList =
+        record.GetAllAlpn();
     if (alpnList.IsEmpty()) {
       result.AppendElement(SVCBWrapper(record));
     } else {
       for (const auto& alpn : alpnList) {
         SVCBWrapper wrapper(record);
-        wrapper.mAlpn.emplace(MakeTuple(alpn, IsAlpnSupported(alpn)));
+        wrapper.mAlpn = Some(alpn);
         result.AppendElement(wrapper);
       }
     }
@@ -367,7 +383,7 @@ DNSHTTPSSVCRecordBase::GetServiceModeRecordInternal(
         continue;
       }
 
-      if (Get<1>(*(record.mAlpn)) == SupportedAlpnType::HTTP_3) {
+      if (IsHttp3(Get<1>(*(record.mAlpn)))) {
         // If the selected alpn is h3 and ech for h3 is disabled, we want
         // to find out if there is another non-h3 record that has
         // echConfig. If yes, we'll use the non-h3 record with echConfig

netwerk/dns/HTTPSSVC.h

@@ -100,7 +100,7 @@ struct SVCB {
   Maybe<uint16_t> GetPort() const;
   bool NoDefaultAlpn() const;
   void GetIPHints(CopyableTArray<mozilla::net::NetAddr>& aAddresses) const;
-  nsTArray<nsCString> GetAllAlpn() const;
+  nsTArray<Tuple<nsCString, SupportedAlpnRank>> GetAllAlpn() const;
   uint16_t mSvcFieldPriority = 0;
   nsCString mSvcDomainName;
   nsCString mEchConfig;
@@ -112,7 +112,7 @@ struct SVCB {
 
 struct SVCBWrapper {
   explicit SVCBWrapper(const SVCB& aRecord) : mRecord(aRecord) {}
-  Maybe<Tuple<nsCString, SupportedAlpnType>> mAlpn;
+  Maybe<Tuple<nsCString, SupportedAlpnRank>> mAlpn;
   const SVCB& mRecord;
 };
 
@@ -123,7 +123,7 @@ class SVCBRecord : public nsISVCBRecord {
   explicit SVCBRecord(const SVCB& data)
       : mData(data), mPort(Nothing()), mAlpn(Nothing()) {}
   explicit SVCBRecord(const SVCB& data,
-                      Maybe<Tuple<nsCString, SupportedAlpnType>> aAlpn);
+                      Maybe<Tuple<nsCString, SupportedAlpnRank>> aAlpn);
 
  private:
   friend class DNSHTTPSSVCRecordBase;
@@ -132,7 +132,7 @@ class SVCBRecord : public nsISVCBRecord {
 
   SVCB mData;
   Maybe<uint16_t> mPort;
-  Maybe<Tuple<nsCString, SupportedAlpnType>> mAlpn;
+  Maybe<Tuple<nsCString, SupportedAlpnRank>> mAlpn;
 };
 
 class DNSHTTPSSVCRecordBase {

netwerk/dns/nsIDNSByTypeRecord.idl

@@ -30,7 +30,7 @@ namespace net {
 native TypeResult(mozilla::net::TypeRecordResultType);
 
 native MaybePort(mozilla::Maybe<uint16_t>);
-native MaybeAlpnTuple(mozilla::Maybe<mozilla::Tuple<nsCString, mozilla::net::SupportedAlpnType>>);
+native MaybeAlpnTuple(mozilla::Maybe<mozilla::Tuple<nsCString, mozilla::net::SupportedAlpnRank>>);
 
 [scriptable, uuid(5d13241b-9d46-448a-90d8-77c418491026)]
 interface nsIDNSByTypeRecord : nsIDNSRecord

netwerk/protocol/http/nsHttp.cpp

@@ -970,10 +970,21 @@ nsresult HttpProxyResponseToErrorCode(uint32_t aStatusCode) {
   return rv;
 }
 
-SupportedAlpnType IsAlpnSupported(const nsACString& aAlpn) {
+SupportedAlpnRank H3VersionToRank(const nsACString& aVersion) {
+  for (uint32_t i = 0; i < kHttp3VersionCount; i++) {
+    if (aVersion.Equals(kHttp3Versions[i])) {
+      return static_cast<SupportedAlpnRank>(
+          static_cast<uint32_t>(SupportedAlpnRank::HTTP_3_DRAFT_29) + i);
+    }
+  }
+
+  return SupportedAlpnRank::NOT_SUPPORTED;
+}
+
+SupportedAlpnRank IsAlpnSupported(const nsACString& aAlpn) {
   if (StaticPrefs::network_http_http3_enable() &&
       gHttpHandler->IsHttp3VersionSupported(aAlpn)) {
-    return SupportedAlpnType::HTTP_3;
+    return H3VersionToRank(aAlpn);
   }
 
   if (gHttpHandler->IsSpdyEnabled()) {
@@ -981,15 +992,15 @@ SupportedAlpnType IsAlpnSupported(const nsACString& aAlpn) {
     SpdyInformation* spdyInfo = gHttpHandler->SpdyInfo();
     if (NS_SUCCEEDED(spdyInfo->GetNPNIndex(aAlpn, &spdyIndex)) &&
         spdyInfo->ProtocolEnabled(spdyIndex)) {
-      return SupportedAlpnType::HTTP_2;
+      return SupportedAlpnRank::HTTP_2;
     }
   }
 
   if (aAlpn.LowerCaseEqualsASCII("http/1.1")) {
-    return SupportedAlpnType::HTTP_1_1;
+    return SupportedAlpnRank::HTTP_1_1;
   }
 
-  return SupportedAlpnType::NOT_SUPPORTED;
+  return SupportedAlpnRank::NOT_SUPPORTED;
 }
 
 bool SecurityErrorToBeHandledByTransaction(nsresult aReason) {

netwerk/protocol/http/nsHttp.h

@@ -37,13 +37,22 @@ enum class HttpVersion {
 
 enum class SpdyVersion { NONE = 0, HTTP_2 = 5 };
 
-enum class SupportedAlpnType : uint8_t {
-  HTTP_3 = 0,
-  HTTP_2,
-  HTTP_1_1,
-  NOT_SUPPORTED
+enum class SupportedAlpnRank : uint8_t {
+  NOT_SUPPORTED = 0,
+  HTTP_1_1 = 1,
+  HTTP_2 = 2,
+  // Note that the order here MUST be the same as the order in kHttp3Versions.
+  HTTP_3_DRAFT_29 = 3,
+  HTTP_3_DRAFT_30 = 4,
+  HTTP_3_DRAFT_31 = 5,
+  HTTP_3_DRAFT_32 = 6,
+  HTTP_3_VER_1 = 7,
 };
 
+inline bool IsHttp3(SupportedAlpnRank aRank) {
+  return aRank >= SupportedAlpnRank::HTTP_3_DRAFT_29;
+}
+
 extern const uint32_t kHttp3VersionCount;
 extern const nsCString kHttp3Versions[];
 
@@ -389,7 +398,7 @@ void LogHeaders(const char* lineStart);
 nsresult HttpProxyResponseToErrorCode(uint32_t aStatusCode);
 
 // Convert an alpn string to SupportedAlpnType.
-SupportedAlpnType IsAlpnSupported(const nsACString& aAlpn);
+SupportedAlpnRank IsAlpnSupported(const nsACString& aAlpn);
 
 static inline bool AllowedErrorForHTTPSRRFallback(nsresult aError) {
   return psm::IsNSSErrorCode(-1 * NS_ERROR_GET_CODE(aError)) ||

netwerk/protocol/http/nsHttpChannel.cpp

@@ -7020,8 +7020,8 @@ static void ReportHTTPSRRTelemetry(
                                                 getter_AddRefs(svcbRecord)))) {
     MOZ_ASSERT(svcbRecord);
 
-    Maybe<Tuple<nsCString, SupportedAlpnType>> alpn = svcbRecord->GetAlpn();
-    bool isHttp3 = alpn ? Get<1>(*alpn) == SupportedAlpnType::HTTP_3 : false;
+    Maybe<Tuple<nsCString, SupportedAlpnRank>> alpn = svcbRecord->GetAlpn();
+    bool isHttp3 = alpn ? IsHttp3(Get<1>(*alpn)) : false;
     Telemetry::Accumulate(Telemetry::HTTPS_RR_WITH_HTTP3_PRESENTED, isHttp3);
   }
 }

netwerk/protocol/http/nsHttpConnectionInfo.cpp

@@ -334,10 +334,10 @@ nsHttpConnectionInfo::CloneAndAdoptHTTPSSVCRecord(
   // Try to get the port and Alpn. If this record has SvcParamKeyPort defined,
   // the new port will be used as mRoutedPort.
   Maybe<uint16_t> port = aRecord->GetPort();
-  Maybe<Tuple<nsCString, SupportedAlpnType>> alpn = aRecord->GetAlpn();
+  Maybe<Tuple<nsCString, SupportedAlpnRank>> alpn = aRecord->GetAlpn();
 
   // Let the new conn info learn h3 will be used.
-  bool isHttp3 = alpn ? Get<1>(*alpn) == SupportedAlpnType::HTTP_3 : false;
+  bool isHttp3 = alpn ? mozilla::net::IsHttp3(Get<1>(*alpn)) : false;
 
   LOG(("HTTPSSVC: use new routed host (%s) and new npnToken (%s)", name.get(),
        alpn ? Get<0>(*alpn).get() : "None"));

netwerk/test/unit/test_https_rr_sorted_alpn.js

@@ -0,0 +1,177 @@
+/* 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";
+
+ChromeUtils.import("resource://gre/modules/NetUtil.jsm");
+
+let trrServer;
+
+const dns = Cc["@mozilla.org/network/dns-service;1"].getService(
+  Ci.nsIDNSService
+);
+
+function setup() {
+  trr_test_setup();
+}
+
+setup();
+registerCleanupFunction(async () => {
+  trr_clear_prefs();
+  Services.prefs.clearUserPref("network.http.http3.support_version1");
+  if (trrServer) {
+    await trrServer.stop();
+  }
+});
+
+function checkResult(inRecord, noHttp2, noHttp3, result) {
+  if (!result) {
+    Assert.throws(
+      () => {
+        inRecord
+          .QueryInterface(Ci.nsIDNSHTTPSSVCRecord)
+          .GetServiceModeRecord(noHttp2, noHttp3);
+      },
+      /NS_ERROR_NOT_AVAILABLE/,
+      "Should get an error"
+    );
+    return;
+  }
+
+  let record = inRecord
+    .QueryInterface(Ci.nsIDNSHTTPSSVCRecord)
+    .GetServiceModeRecord(noHttp2, noHttp3);
+  Assert.equal(record.priority, result.expectedPriority);
+  Assert.equal(record.name, result.expectedName);
+  Assert.equal(record.selectedAlpn, result.expectedAlpn);
+}
+
+add_task(async function testSortedAlpnH3() {
+  dns.clearCache(true);
+
+  trrServer = new TRRServer();
+  await trrServer.start();
+
+  Services.prefs.setIntPref("network.trr.mode", 3);
+  Services.prefs.setCharPref(
+    "network.trr.uri",
+    `https://foo.example.com:${trrServer.port}/dns-query`
+  );
+  Services.prefs.setBoolPref("network.http.http3.support_version1", true);
+  await trrServer.registerDoHAnswers("test.alpn.com", "HTTPS", {
+    answers: [
+      {
+        name: "test.alpn.com",
+        ttl: 55,
+        type: "HTTPS",
+        flush: false,
+        data: {
+          priority: 1,
+          name: "test.alpn.com",
+          values: [{ key: "alpn", value: ["h2", "http/1.1", "h3-30", "h3"] }],
+        },
+      },
+    ],
+  });
+
+  let { inRecord } = await new TRRDNSListener("test.alpn.com", {
+    type: dns.RESOLVE_TYPE_HTTPSSVC,
+  });
+
+  checkResult(inRecord, false, false, {
+    expectedPriority: 1,
+    expectedName: "test.alpn.com",
+    expectedAlpn: "h3",
+  });
+  checkResult(inRecord, false, true, {
+    expectedPriority: 1,
+    expectedName: "test.alpn.com",
+    expectedAlpn: "h2",
+  });
+  checkResult(inRecord, true, false, {
+    expectedPriority: 1,
+    expectedName: "test.alpn.com",
+    expectedAlpn: "h3",
+  });
+  checkResult(inRecord, true, true, {
+    expectedPriority: 1,
+    expectedName: "test.alpn.com",
+    expectedAlpn: "http/1.1",
+  });
+
+  Services.prefs.setBoolPref("network.http.http3.support_version1", false);
+  checkResult(inRecord, false, false, {
+    expectedPriority: 1,
+    expectedName: "test.alpn.com",
+    expectedAlpn: "h3-30",
+  });
+  checkResult(inRecord, false, true, {
+    expectedPriority: 1,
+    expectedName: "test.alpn.com",
+    expectedAlpn: "h2",
+  });
+  checkResult(inRecord, true, false, {
+    expectedPriority: 1,
+    expectedName: "test.alpn.com",
+    expectedAlpn: "h3-30",
+  });
+  checkResult(inRecord, true, true, {
+    expectedPriority: 1,
+    expectedName: "test.alpn.com",
+    expectedAlpn: "http/1.1",
+  });
+});
+
+add_task(async function testSortedAlpnH2() {
+  dns.clearCache(true);
+
+  Services.prefs.setIntPref("network.trr.mode", 3);
+  Services.prefs.setCharPref(
+    "network.trr.uri",
+    `https://foo.example.com:${trrServer.port}/dns-query`
+  );
+  await trrServer.registerDoHAnswers("test.alpn_2.com", "HTTPS", {
+    answers: [
+      {
+        name: "test.alpn_2.com",
+        ttl: 55,
+        type: "HTTPS",
+        flush: false,
+        data: {
+          priority: 1,
+          name: "test.alpn_2.com",
+          values: [{ key: "alpn", value: ["http/1.1", "h2"] }],
+        },
+      },
+    ],
+  });
+
+  let { inRecord } = await new TRRDNSListener("test.alpn_2.com", {
+    type: dns.RESOLVE_TYPE_HTTPSSVC,
+  });
+
+  checkResult(inRecord, false, false, {
+    expectedPriority: 1,
+    expectedName: "test.alpn_2.com",
+    expectedAlpn: "h2",
+  });
+  checkResult(inRecord, false, true, {
+    expectedPriority: 1,
+    expectedName: "test.alpn_2.com",
+    expectedAlpn: "h2",
+  });
+  checkResult(inRecord, true, false, {
+    expectedPriority: 1,
+    expectedName: "test.alpn_2.com",
+    expectedAlpn: "http/1.1",
+  });
+  checkResult(inRecord, true, true, {
+    expectedPriority: 1,
+    expectedName: "test.alpn_2.com",
+    expectedAlpn: "http/1.1",
+  });
+
+  await trrServer.stop();
+  trrServer = null;
+});

netwerk/test/unit/xpcshell.ini

@@ -590,3 +590,6 @@ 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]
+[test_https_rr_sorted_alpn.js]
+skip-if = os == "android"
+run-sequentially = node server exceptions dont replay well