Bug 1980812 - Avoid setting explicit h2 ALPN for connections, a=dmeehan

Original Revision: https://phabricator.services.mozilla.com/D265491

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

netwerk/dns/HTTPSSVC.cpp

@@ -359,6 +359,7 @@ static nsTArray<SVCBWrapper> FlattenRecords(const nsACString& aHost,
     if (alpnList.IsEmpty()) {
       result.AppendElement(SVCBWrapper(record));
     } else {
+      bool h1AlpnAdded = false;
       if (!hasNoDefaultAlpn) {
         // Consider two scenarios when "no-default-alpn" is not found:
         // 1. If echConfig is present in the record:
@@ -374,15 +375,32 @@ static nsTArray<SVCBWrapper> FlattenRecords(const nsACString& aHost,
         if (!aHost.Equals(record.mSvcDomainName) || record.mHasEchConfig) {
           alpnList.AppendElement(
               std::make_tuple(""_ns, SupportedAlpnRank::HTTP_1_1));
+          h1AlpnAdded = true;
         }
       }
       for (const auto& alpn : alpnList) {
-        SVCBWrapper wrapper(record);
+        const auto alpnRank = std::get<1>(alpn);
-        wrapper.mAlpn = Some(alpn);
+        if (IsHttp3(alpnRank)) {
-        if (IsHttp3(std::get<1>(alpn))) {
           aH3RecordCount++;
         }
-        result.AppendElement(wrapper);
+
+        // Skip explicit h2 if h1 is already present.
+        if (alpnRank == SupportedAlpnRank::HTTP_2 && h1AlpnAdded) {
+          continue;
+        }
+
+        // For h2, normalize the tuple to use an empty ALPN. If both h1 and h2
+        // are available, the ALPN negotiation will automatically select h2 when
+        // the server supports it, otherwise it will fall back to h1.
+        // However, if `no-default-alpn` is present, fallback to h1 is not
+        // possible, so we must explicitly set h2.
+        auto chosen =
+            (alpnRank == SupportedAlpnRank::HTTP_2 && !hasNoDefaultAlpn)
+                ? std::make_tuple(""_ns, alpnRank)
+                : alpn;
+        SVCBWrapper wrapper(record);
+        wrapper.mAlpn = Some(chosen);
+        result.AppendElement(std::move(wrapper));
       }
     }
   }

netwerk/test/unit/test_http3_fast_fallback.js

@@ -913,3 +913,64 @@ add_task(async function testFastfallbackToTheSameRecord() {
 
   await trrServer.stop();
 });
+
+// Similar to the previous test, but no ech.
+add_task(async function testFastfallbackToTheSameRecord1() {
+  trrServer = new TRRServer();
+  await trrServer.start();
+  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.http3_echconfig.enabled", true);
+
+  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.enable", true);
+
+  Services.prefs.setIntPref(
+    "network.dns.httpssvc.http3_fast_fallback_timeout",
+    1000
+  );
+
+  await trrServer.registerDoHAnswers("test.no_ech.org", "HTTPS", {
+    answers: [
+      {
+        name: "test.no_ech.org",
+        ttl: 55,
+        type: "HTTPS",
+        flush: false,
+        data: {
+          priority: 1,
+          name: "test.no_ech.org",
+          values: [
+            { key: "alpn", value: ["h3", "h2"] },
+            { key: "port", value: h2Port },
+          ],
+        },
+      },
+    ],
+  });
+
+  await trrServer.registerDoHAnswers("test.no_ech.org", "A", {
+    answers: [
+      {
+        name: "test.no_ech.org",
+        ttl: 55,
+        type: "A",
+        flush: false,
+        data: "127.0.0.1",
+      },
+    ],
+  });
+
+  let chan = makeChan(`https://test.no_ech.org/server-timing`);
+  let [req] = await channelOpenPromise(chan);
+  Assert.equal(req.protocolVersion, "h2");
+  let internal = req.QueryInterface(Ci.nsIHttpChannelInternal);
+  Assert.equal(internal.remotePort, h2Port);
+
+  await trrServer.stop();
+});

netwerk/test/unit/test_https_rr_ech_prefs.js

@@ -106,7 +106,7 @@ add_task(async function testEchConfigEnabled() {
   checkResult(inRecord, false, true, {
     expectedPriority: 2,
     expectedName: "test.bar_2.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, true, false, {
     expectedPriority: 1,
@@ -129,12 +129,12 @@ add_task(async function testEchConfigEnabled() {
   checkResult(inRecord, false, false, {
     expectedPriority: 2,
     expectedName: "test.bar_2.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, false, true, {
     expectedPriority: 2,
     expectedName: "test.bar_2.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, true, false, {
     expectedPriority: 2,
@@ -212,12 +212,12 @@ add_task(async function testTwoRecordsHaveEchConfig() {
   checkResult(inRecord, false, false, {
     expectedPriority: 2,
     expectedName: "test.foo_h2.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, false, true, {
     expectedPriority: 2,
     expectedName: "test.foo_h2.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, true, false, {
     expectedPriority: 2,
@@ -244,7 +244,7 @@ add_task(async function testTwoRecordsHaveEchConfig() {
   checkResult(inRecord, false, true, {
     expectedPriority: 2,
     expectedName: "test.foo_h2.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, true, false, {
     expectedPriority: 1,
@@ -429,7 +429,7 @@ add_task(async function testOneRecordsHasEchConfig() {
   checkResult(inRecord, false, true, {
     expectedPriority: 2,
     expectedName: "test.foo_h2.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, true, false, {
     expectedPriority: 1,
@@ -456,7 +456,7 @@ add_task(async function testOneRecordsHasEchConfig() {
   checkResult(inRecord, false, true, {
     expectedPriority: 2,
     expectedName: "test.foo_h2.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, true, false, {
     expectedPriority: 1,

netwerk/test/unit/test_https_rr_sorted_alpn.js

@@ -89,7 +89,7 @@ add_task(async function testSortedAlpnH3() {
   checkResult(inRecord, false, true, {
     expectedPriority: 1,
     expectedName: "test.alpn.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, true, false, {
     expectedPriority: 1,
@@ -106,12 +106,12 @@ add_task(async function testSortedAlpnH3() {
   checkResult(inRecord, false, false, {
     expectedPriority: 1,
     expectedName: "test.alpn.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, false, true, {
     expectedPriority: 1,
     expectedName: "test.alpn.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, true, false, {
     expectedPriority: 1,
@@ -130,12 +130,12 @@ add_task(async function testSortedAlpnH3() {
   checkResult(inRecord, false, false, {
     expectedPriority: 1,
     expectedName: "test.alpn.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, false, true, {
     expectedPriority: 1,
     expectedName: "test.alpn.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, true, false, {
     expectedPriority: 1,
@@ -158,7 +158,7 @@ add_task(async function testSortedAlpnH3() {
   checkResult(inRecord, false, true, {
     expectedPriority: 1,
     expectedName: "test.alpn.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, true, false, {
     expectedPriority: 1,
@@ -203,12 +203,12 @@ add_task(async function testSortedAlpnH2() {
   checkResult(inRecord, false, false, {
     expectedPriority: 1,
     expectedName: "test.alpn_2.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, false, true, {
     expectedPriority: 1,
     expectedName: "test.alpn_2.com",
-    expectedAlpn: "h2",
+    expectedAlpn: "",
   });
   checkResult(inRecord, true, false, {
     expectedPriority: 1,