Bug 1816677 - Make sure we rewind the request stream when retrying a transaction for different IP address, r=necko-reviewers,jesup

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

modules/libpref/init/StaticPrefList.yaml

@@ -12424,7 +12424,7 @@
 # When a Http/3 connection failed, whether to retry with a different IP address.
 - name: network.http.http3.retry_different_ip_family
   type: RelaxedAtomicBool
-  value: false
+  value: @IS_EARLY_BETA_OR_EARLIER@
   mirror: always
 
 # This is for testing purpose. When true, nsUDPSocket::SendWithAddress will

netwerk/protocol/http/Http3Session.cpp

@@ -264,17 +264,13 @@ void Http3Session::Shutdown() {
           mNetAddr->GetNetAddr(&addr);
           gHttpHandler->ConnMgr()->SetRetryDifferentIPFamilyForHttp3(
               mConnInfo, addr.raw.family);
-          nsHttpTransaction* trans =
-              stream->Transaction()->QueryHttpTransaction();
-          if (trans) {
-            // This is a bit hacky. We redispatch the transaction here to avoid
-            // touching the complicated retry logic in nsHttpTransaction.
-            trans->RemoveConnection();
-            Unused << gHttpHandler->InitiateTransaction(trans,
-                                                        trans->Priority());
-          } else {
-            stream->Close(NS_ERROR_NET_RESET);
-          }
+          // We want the transaction to be restarted with the same connection
+          // info.
+          stream->Transaction()->DoNotRemoveAltSvc();
+          // We already set the preference in SetRetryDifferentIPFamilyForHttp3,
+          // so we want to keep it for the next retry.
+          stream->Transaction()->DoNotResetIPFamilyPreference();
+          stream->Close(NS_ERROR_NET_RESET);
           // Since Http3Session::Shutdown can be called multiple times, we set
           // mDontExclude for not putting this domain into the excluded list.
           mDontExclude = true;

netwerk/protocol/http/nsAHttpTransaction.h

@@ -194,6 +194,10 @@ class nsAHttpTransaction : public nsSupportsWeakReference {
   // want to use the alt-svc on the restart.
   virtual void DoNotRemoveAltSvc() {}
 
+  // We call this function if we do want to reset IP family preference again on
+  // the next restart.
+  virtual void DoNotResetIPFamilyPreference() {}
+
   // Returns true if early-data is possible and transaction will remember
   // that it is in 0RTT mode (to know should it rewide transaction or not
   // in the case of an error).

netwerk/protocol/http/nsHttpTransaction.cpp

@@ -1415,8 +1415,13 @@ void nsHttpTransaction::Close(nsresult reason) {
   }
   mConnected = false;
 
+  // When mDoNotRemoveAltSvc is true, this means we want to keep the AltSvc in
+  // in the conncetion info. In this case, let's not apply HTTPS RR retry logic
+  // to make sure this transaction can be restarted with the same conncetion
+  // info.
   bool shouldRestartTransactionForHTTPSRR =
-      mOrigConnInfo && AllowedErrorForHTTPSRRFallback(reason);
+      mOrigConnInfo && AllowedErrorForHTTPSRRFallback(reason) &&
+      !mDoNotRemoveAltSvc;
 
   //
   // if the connection was reset or closed before we wrote any part of the
@@ -1850,9 +1855,11 @@ nsresult nsHttpTransaction::Restart() {
   // Use TRANSACTION_RESTART_OTHERS as a catch-all.
   SetRestartReason(TRANSACTION_RESTART_OTHERS);
 
-  // Reset the IP family preferences, so the new connection can try to use
-  // another IPv4 or IPv6 address.
-  gHttpHandler->ConnMgr()->ResetIPFamilyPreference(mConnInfo);
+  if (!mDoNotResetIPFamilyPreference) {
+    // Reset the IP family preferences, so the new connection can try to use
+    // another IPv4 or IPv6 address.
+    gHttpHandler->ConnMgr()->ResetIPFamilyPreference(mConnInfo);
+  }
 
   return gHttpHandler->InitiateTransaction(this, mPriority);
 }

netwerk/protocol/http/nsHttpTransaction.h

@@ -137,6 +137,9 @@ class nsHttpTransaction final : public nsAHttpTransaction,
   void DisableSpdy() override;
   void DisableHttp2ForProxy() override;
   void DoNotRemoveAltSvc() override { mDoNotRemoveAltSvc = true; }
+  void DoNotResetIPFamilyPreference() override {
+    mDoNotResetIPFamilyPreference = true;
+  }
   void DisableHttp3(bool aAllowRetryHTTPSRR) override;
 
   nsHttpTransaction* QueryHttpTransaction() override { return this; }
@@ -457,6 +460,7 @@ class nsHttpTransaction final : public nsAHttpTransaction,
   bool mDeferredSendProgress{false};
   bool mWaitingOnPipeOut{false};
   bool mDoNotRemoveAltSvc{false};
+  bool mDoNotResetIPFamilyPreference{false};
   bool mIsHttp2Websocket{false};
 
   // mClosed           := transaction has been explicitly closed

netwerk/test/unit/test_http3_dns_retry.js

@@ -12,9 +12,6 @@ let h2Port;
 let h3Port;
 let trrServer;
 
-const { TestUtils } = ChromeUtils.importESModule(
-  "resource://testing-common/TestUtils.sys.mjs"
-);
 const certOverrideService = Cc[
   "@mozilla.org/security/certoverride;1"
 ].getService(Ci.nsICertOverrideService);
@@ -31,8 +28,12 @@ add_setup(async function setup() {
   trr_test_setup();
 
   if (mozinfo.socketprocess_networking) {
+    Cc["@mozilla.org/network/protocol;1?name=http"].getService(
+      Ci.nsIHttpProtocolHandler
+    );
     Services.dns; // Needed to trigger socket process.
-    await TestUtils.waitForCondition(() => Services.io.socketProcessLaunched);
+    // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+    await new Promise(resolve => setTimeout(resolve, 1000));
   }
 
   Services.prefs.setIntPref("network.trr.mode", 2); // TRR first
@@ -42,9 +43,24 @@ add_setup(async function setup() {
     "network.http.http3.block_loopback_ipv6_addr",
     true
   );
+  Services.prefs.setBoolPref(
+    "network.http.http3.retry_different_ip_family",
+    true
+  );
+
+  certOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData(
+    true
+  );
 
   registerCleanupFunction(async () => {
+    certOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData(
+      false
+    );
     trr_clear_prefs();
+    Services.prefs.clearUserPref(
+      "network.http.http3.retry_different_ip_family"
+    );
+    Services.prefs.clearUserPref("network.http.speculative-parallel-limit");
     Services.prefs.clearUserPref("network.http.http3.block_loopback_ipv6_addr");
     if (trrServer) {
       await trrServer.stop();
@@ -67,15 +83,9 @@ function channelOpenPromise(chan, flags) {
   return new Promise(async resolve => {
     function finish(req, buffer) {
       resolve([req, buffer]);
-      certOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData(
-        false
-      );
     }
     let internal = chan.QueryInterface(Ci.nsIHttpChannelInternal);
     internal.setWaitForHTTPSSVCRecord();
-    certOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData(
-      true
-    );
 
     chan.asyncOpen(new ChannelListener(finish, null, flags));
   });
@@ -139,7 +149,7 @@ add_task(async function test_retry_with_ipv4() {
         priority: 1,
         name: host,
         values: [
-          { key: "alpn", value: "h3-29" },
+          { key: "alpn", value: "h3" },
           { key: "port", value: h3Port },
         ],
       },
@@ -150,7 +160,7 @@ add_task(async function test_retry_with_ipv4() {
 
   let chan = makeChan(`https://${host}`);
   let [req] = await channelOpenPromise(chan);
-  Assert.equal(req.protocolVersion, "h3-29");
+  Assert.equal(req.protocolVersion, "h3");
 
   await trrServer.stop();
 });
@@ -187,7 +197,7 @@ add_task(async function test_retry_with_ipv4_disabled() {
         priority: 1,
         name: host,
         values: [
-          { key: "alpn", value: "h3-29" },
+          { key: "alpn", value: "h3" },
           { key: "port", value: h3Port },
         ],
       },
@@ -240,7 +250,7 @@ add_task(async function test_retry_with_ipv4_failed() {
         priority: 1,
         name: host,
         values: [
-          { key: "alpn", value: "h3-29" },
+          { key: "alpn", value: "h3" },
           { key: "port", value: h3Port },
         ],
       },
@@ -281,3 +291,65 @@ add_task(async function test_retry_with_ipv4_failed() {
   await new Promise(resolve => setTimeout(resolve, 3000));
   await trrServer.stop();
 });
+
+add_task(async function test_retry_with_0rtt() {
+  let host = "test.http3_retry_0rtt.com";
+  let ipv4answers = [
+    {
+      name: host,
+      ttl: 55,
+      type: "A",
+      flush: false,
+      data: "127.0.0.1",
+    },
+  ];
+  // The UDP socket will return connection refused error because we set
+  // "network.http.http3.block_loopback_ipv6_addr" to true.
+  let ipv6answers = [
+    {
+      name: host,
+      ttl: 55,
+      type: "AAAA",
+      flush: false,
+      data: "::1",
+    },
+  ];
+  let httpsRecord = [
+    {
+      name: host,
+      ttl: 55,
+      type: "HTTPS",
+      flush: false,
+      data: {
+        priority: 1,
+        name: host,
+        values: [
+          { key: "alpn", value: "h3" },
+          { key: "port", value: h3Port },
+        ],
+      },
+    },
+  ];
+
+  await registerDoHAnswers(host, ipv4answers, ipv6answers, httpsRecord);
+
+  let chan = makeChan(`https://${host}`);
+  chan.QueryInterface(Ci.nsIHttpChannelInternal);
+  chan.setIPv6Disabled();
+
+  let [req] = await channelOpenPromise(chan);
+  Assert.equal(req.protocolVersion, "h3");
+
+  // Make sure the h3 connection created by the previous test is cleared.
+  Services.obs.notifyObservers(null, "net:cancel-all-connections");
+  // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+  await new Promise(resolve => setTimeout(resolve, 1000));
+
+  chan = makeChan(`https://${host}`);
+  chan.QueryInterface(Ci.nsIHttpChannelInternal);
+
+  [req] = await channelOpenPromise(chan);
+  Assert.equal(req.protocolVersion, "h3");
+
+  await trrServer.stop();
+});

netwerk/test/unit/xpcshell.ini

@@ -775,7 +775,6 @@ skip-if =
 skip-if =
   os == 'android'
   os == 'win' && msix
-  true
 run-sequentially = node server exceptions dont replay well
 [test_non_ipv4_hostname_ending_in_number_cookie_db.js]
 [test_verify_traffic.js]