Bug 1671885 - Add a test case for ech retry r=necko-reviewers,dragana

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

netwerk/protocol/http/ConnectionEntry.cpp

@@ -936,5 +936,27 @@ bool ConnectionEntry::RemoveTransFromPendingQ(nsHttpTransaction* aTrans) {
   return true;
 }
 
+void ConnectionEntry::MaybeUpdateEchConfig(nsHttpConnectionInfo* aConnInfo) {
+  if (!mConnInfo->HashKey().Equals(aConnInfo->HashKey())) {
+    return;
+  }
+
+  const nsCString& echConfig = aConnInfo->GetEchConfig();
+  if (mConnInfo->GetEchConfig().Equals(echConfig)) {
+    return;
+  }
+
+  LOG(("ConnectionEntry::MaybeUpdateEchConfig [ci=%s]\n",
+       mConnInfo->HashKey().get()));
+
+  mConnInfo->SetEchConfig(echConfig);
+
+  // If echConfig is changed, we should close all half opens and idle
+  // connections. This is to make sure the new echConfig will be used for the
+  // next connection.
+  CloseAllHalfOpens();
+  CloseIdleConnections();
+}
+
 }  // namespace net
 }  // namespace mozilla

netwerk/protocol/http/ConnectionEntry.h

@@ -185,6 +185,8 @@ class ConnectionEntry {
 
   bool RemoveTransFromPendingQ(nsHttpTransaction* aTrans);
 
+  void MaybeUpdateEchConfig(nsHttpConnectionInfo* aConnInfo);
+
  private:
   void InsertIntoIdleConnections_internal(nsHttpConnection* conn);
   void RemoveFromIdleConnectionsIndex(size_t inx);

netwerk/protocol/http/nsHttpConnectionInfo.cpp

@@ -378,7 +378,7 @@ nsHttpConnectionInfo::CloneAndAdoptHTTPSSVCRecord(
     clone = new nsHttpConnectionInfo(
         mOrigin, mOriginPort, alpn ? Get<0>(*alpn) : EmptyCString(), mUsername,
         mTopWindowOrigin, mProxyInfo, mOriginAttributes, name,
-        port ? *port : mRoutedPort, mIsolated, isHttp3);
+        port ? *port : mOriginPort, mIsolated, isHttp3);
   }
 
   // Make sure the anonymous, insecure-scheme, and private flags are transferred

netwerk/protocol/http/nsHttpConnectionMgr.cpp

@@ -1627,6 +1627,10 @@ nsresult nsHttpConnectionMgr::ProcessNewTransaction(nsHttpTransaction* trans) {
       trans->Caps() & NS_HTTP_DISALLOW_HTTP3);
   MOZ_ASSERT(ent);
 
+  if (gHttpHandler->EchConfigEnabled()) {
+    ent->MaybeUpdateEchConfig(ci);
+  }
+
   ReportProxyTelemetry(ent);
 
   // Check if the transaction already has a sticky reference to a connection.
@@ -3444,36 +3448,25 @@ void nsHttpConnectionMgr::MoveToWildCardConnEntry(
   ent->MoveConnection(proxyConn, wcEnt);
 }
 
-bool nsHttpConnectionMgr::MoveTransToNewConnEntry(nsHttpTransaction* aTrans,
-                                                  nsHttpConnectionInfo* aNewCI,
-                                                  bool aNoHttp3ForNewEntry) {
+bool nsHttpConnectionMgr::MoveTransToNewConnEntry(
+    nsHttpTransaction* aTrans, nsHttpConnectionInfo* aNewCI) {
   MOZ_ASSERT(OnSocketThread(), "not on socket thread");
 
-  LOG(
-      ("nsHttpConnectionMgr::MoveTransToNewConnEntry: trans=%p aNewCI=%s "
-       "aNoHttp3ForNewEntry=%d",
-       aTrans, aNewCI->HashKey().get(), aNoHttp3ForNewEntry));
-
-  bool prohibitWildCard = !!aTrans->TunnelProvider();
-  bool noHttp3 = aTrans->Caps() & NS_HTTP_DISALLOW_HTTP3;
-  bool noHttp2 = aTrans->Caps() & NS_HTTP_DISALLOW_SPDY;
-  // Step 1: Check if the new entry is the same as the old one.
-  ConnectionEntry* oldEntry = GetOrCreateConnectionEntry(
-      aTrans->ConnectionInfo(), prohibitWildCard, noHttp2, noHttp3);
+  LOG(("nsHttpConnectionMgr::MoveTransToNewConnEntry: trans=%p aNewCI=%s",
+       aTrans, aNewCI->HashKey().get()));
 
-  ConnectionEntry* newEntry = GetOrCreateConnectionEntry(
-      aNewCI, prohibitWildCard, noHttp2, noHttp3 || aNoHttp3ForNewEntry);
-
-  if (oldEntry == newEntry) {
-    return true;
+  // Step 1: Get the transaction's connection entry.
+  ConnectionEntry* entry = mCT.GetWeak(aTrans->ConnectionInfo()->HashKey());
+  if (!entry) {
+    return false;
   }
 
   // Step 2: Try to find the undispatched transaction.
-  if (!oldEntry->RemoveTransFromPendingQ(aTrans)) {
+  if (!entry->RemoveTransFromPendingQ(aTrans)) {
     return false;
   }
 
-  // Step 3: Add the transaction to the new entry.
+  // Step 3: Add the transaction.
   aTrans->UpdateConnectionInfo(aNewCI);
   Unused << ProcessNewTransaction(aTrans);
   return true;

netwerk/protocol/http/nsHttpConnectionMgr.h

@@ -75,8 +75,7 @@ class nsHttpConnectionMgr final : public HttpConnectionMgrShell,
   // one. Returns true if the transaction is moved successfully, otherwise
   // returns false.
   bool MoveTransToNewConnEntry(nsHttpTransaction* aTrans,
-                               nsHttpConnectionInfo* aNewCI,
-                               bool aNoHttp3ForNewEntry = false);
+                               nsHttpConnectionInfo* aNewCI);
 
   // This is used to force an idle connection to be closed and removed from
   // the idle connection list. It is called when the idle connection detects

netwerk/protocol/http/nsHttpTransaction.cpp

@@ -64,11 +64,6 @@ static NS_DEFINE_CID(kMultiplexInputStream, NS_MULTIPLEXINPUTSTREAM_CID);
 // looking for a response header
 #define MAX_INVALID_RESPONSE_BODY_SIZE (1024 * 128)
 
-// TODO: These values should be removed when bug 1654332 is landed.
-#define MOCK_SSL_ERROR_ECH_RETRY_WITH_ECH (SSL_ERROR_BASE + 188)
-#define MOCK_SSL_ERROR_ECH_RETRY_WITHOUT_ECH (SSL_ERROR_BASE + 189)
-#define MOCK_SSL_ERROR_ECH_FAILED (SSL_ERROR_BASE + 190)
-
 using namespace mozilla::net;
 
 namespace mozilla {
@@ -1262,17 +1257,16 @@ void nsHttpTransaction::PrepareConnInfoForRetry(nsresult aReason) {
     }
   });
 
-  if (aReason ==
-      psm::GetXPCOMFromNSSError(MOCK_SSL_ERROR_ECH_RETRY_WITHOUT_ECH)) {
+  if (aReason == psm::GetXPCOMFromNSSError(SSL_ERROR_ECH_RETRY_WITHOUT_ECH)) {
     LOG((" Got SSL_ERROR_ECH_RETRY_WITHOUT_ECH, use empty echConfig to retry"));
     failedConnInfo->SetEchConfig(EmptyCString());
     failedConnInfo.swap(mConnInfo);
     id = Telemetry::TRANSACTION_ECH_RETRY_WITHOUT_ECH_COUNT;
     return;
   }
 
-  if (aReason == psm::GetXPCOMFromNSSError(MOCK_SSL_ERROR_ECH_RETRY_WITH_ECH)) {
-    LOG((" Got SSL_ERROR_ECH_RETRY_WITHOUT_ECH, use retry echConfig"));
+  if (aReason == psm::GetXPCOMFromNSSError(SSL_ERROR_ECH_RETRY_WITH_ECH)) {
+    LOG((" Got SSL_ERROR_ECH_RETRY_WITH_ECH, use retry echConfig"));
     MOZ_ASSERT(mConnection);
 
     nsCOMPtr<nsISupports> secInfo;
@@ -1297,10 +1291,10 @@ void nsHttpTransaction::PrepareConnInfoForRetry(nsresult aReason) {
 
   // Note that we retry the connection not only for SSL_ERROR_ECH_FAILED, but
   // also for all failure cases.
-  if (aReason == psm::GetXPCOMFromNSSError(MOCK_SSL_ERROR_ECH_FAILED) ||
+  if (aReason == psm::GetXPCOMFromNSSError(SSL_ERROR_ECH_FAILED) ||
       NS_FAILED(aReason)) {
     LOG((" Got SSL_ERROR_ECH_FAILED, try other records"));
-    if (aReason == psm::GetXPCOMFromNSSError(MOCK_SSL_ERROR_ECH_FAILED)) {
+    if (aReason == psm::GetXPCOMFromNSSError(SSL_ERROR_ECH_FAILED)) {
       id = Telemetry::TRANSACTION_ECH_RETRY_ECH_FAILED_COUNT;
     }
     if (mRecordsForRetry.IsEmpty()) {
@@ -1347,6 +1341,28 @@ void nsHttpTransaction::PrepareConnInfoForRetry(nsresult aReason) {
   }
 }
 
+void nsHttpTransaction::MaybeReportFailedSVCDomain(
+    nsresult aReason, nsHttpConnectionInfo* aFailedConnInfo) {
+  if (aReason == psm::GetXPCOMFromNSSError(SSL_ERROR_ECH_RETRY_WITHOUT_ECH) ||
+      aReason != psm::GetXPCOMFromNSSError(SSL_ERROR_ECH_RETRY_WITH_ECH)) {
+    return;
+  }
+
+  Telemetry::Accumulate(Telemetry::DNS_HTTPSSVC_CONNECTION_FAILED_REASON,
+                        ErrorCodeToFailedReason(aReason));
+
+  nsCOMPtr<nsIDNSService> dns = do_GetService(NS_DNSSERVICE_CONTRACTID);
+  if (dns) {
+    const nsCString& failedHost = aFailedConnInfo->GetRoutedHost().IsEmpty()
+                                      ? aFailedConnInfo->GetOrigin()
+                                      : aFailedConnInfo->GetRoutedHost();
+    LOG(("add failed domain name [%s] -> [%s] to exclusion list",
+         aFailedConnInfo->GetOrigin().get(), failedHost.get()));
+    Unused << dns->ReportFailedSVCDomainName(aFailedConnInfo->GetOrigin(),
+                                             failedHost);
+  }
+}
+
 void nsHttpTransaction::Close(nsresult reason) {
   LOG(("nsHttpTransaction::Close [this=%p reason=%" PRIx32 "]\n", this,
        static_cast<uint32_t>(reason)));
@@ -1512,16 +1528,7 @@ void nsHttpTransaction::Close(nsresult reason) {
                             !reallySentData || connReused)) ||
         restartToFallbackConnInfo) {
       if (restartToFallbackConnInfo) {
-        Telemetry::Accumulate(Telemetry::DNS_HTTPSSVC_CONNECTION_FAILED_REASON,
-                              ErrorCodeToFailedReason(reason));
-        nsCOMPtr<nsIDNSService> dns = do_GetService(NS_DNSSERVICE_CONTRACTID);
-        if (dns) {
-          LOG(("add failed domain name [%s] -> [%s] to exclusion list",
-               mConnInfo->GetOrigin().get(), mConnInfo->GetRoutedHost().get()));
-          Unused << dns->ReportFailedSVCDomainName(mConnInfo->GetOrigin(),
-                                                   mConnInfo->GetRoutedHost());
-        }
-
+        MaybeReportFailedSVCDomain(reason, mConnInfo);
         PrepareConnInfoForRetry(reason);
         mDontRetryWithDirectRoute = true;
         LOG(
@@ -3293,8 +3300,8 @@ void nsHttpTransaction::HandleFallback(
   LOG(("nsHttpTransaction %p HandleFallback to connInfo[%s]", this,
        aFallbackConnInfo->HashKey().get()));
 
-  bool foundInPendingQ = gHttpHandler->ConnMgr()->MoveTransToNewConnEntry(
-      this, aFallbackConnInfo, true);
+  bool foundInPendingQ =
+      gHttpHandler->ConnMgr()->MoveTransToNewConnEntry(this, aFallbackConnInfo);
   if (!foundInPendingQ) {
     MOZ_ASSERT(false, "transaction not in entry");
     return;

netwerk/protocol/http/nsHttpTransaction.h

@@ -228,6 +228,9 @@ class nsHttpTransaction final : public nsAHttpTransaction,
   already_AddRefed<nsHttpConnectionInfo> PrepareFastFallbackConnInfo(
       bool aEchConfigUsed);
 
+  void MaybeReportFailedSVCDomain(nsresult aReason,
+                                  nsHttpConnectionInfo* aFailedConnInfo);
+
   already_AddRefed<Http2PushedStreamWrapper> TakePushedStreamById(
       uint32_t aStreamId);
 

netwerk/test/unit/head_channels.js

@@ -351,3 +351,106 @@ function deserialize_from_escaped_string(str) {
   objectInStream.setInputStream(pipe.inputStream);
   return objectInStream.readObject(true);
 }
+
+// Copied from head_psm.js.
+function add_tls_server_setup(serverBinName, certsPath, addDefaultRoot = true) {
+  add_test(function() {
+    _setupTLSServerTest(serverBinName, certsPath, addDefaultRoot);
+  });
+}
+
+// Do not call this directly; use add_tls_server_setup
+function _setupTLSServerTest(serverBinName, certsPath, addDefaultRoot) {
+  asyncStartTLSTestServer(serverBinName, certsPath, addDefaultRoot).then(
+    run_next_test
+  );
+}
+
+async function asyncStartTLSTestServer(
+  serverBinName,
+  certsPath,
+  addDefaultRoot
+) {
+  const { HttpServer } = ChromeUtils.import(
+    "resource://testing-common/httpd.js"
+  );
+  let certdb = Cc["@mozilla.org/security/x509certdb;1"].getService(
+    Ci.nsIX509CertDB
+  );
+  // The trusted CA that is typically used for "good" certificates.
+  if (addDefaultRoot) {
+    addCertFromFile(certdb, `${certsPath}/test-ca.pem`, "CTu,u,u");
+  }
+
+  const CALLBACK_PORT = 8444;
+
+  let envSvc = Cc["@mozilla.org/process/environment;1"].getService(
+    Ci.nsIEnvironment
+  );
+  let greBinDir = Services.dirsvc.get("GreBinD", Ci.nsIFile);
+  envSvc.set("DYLD_LIBRARY_PATH", greBinDir.path);
+  // TODO(bug 1107794): Android libraries are in /data/local/xpcb, but "GreBinD"
+  // does not return this path on Android, so hard code it here.
+  envSvc.set("LD_LIBRARY_PATH", greBinDir.path + ":/data/local/xpcb");
+  envSvc.set("MOZ_TLS_SERVER_DEBUG_LEVEL", "3");
+  envSvc.set("MOZ_TLS_SERVER_CALLBACK_PORT", CALLBACK_PORT);
+
+  let httpServer = new HttpServer();
+  let serverReady = new Promise(resolve => {
+    httpServer.registerPathHandler("/", function handleServerCallback(
+      aRequest,
+      aResponse
+    ) {
+      aResponse.setStatusLine(aRequest.httpVersion, 200, "OK");
+      aResponse.setHeader("Content-Type", "text/plain");
+      let responseBody = "OK!";
+      aResponse.bodyOutputStream.write(responseBody, responseBody.length);
+      executeSoon(function() {
+        httpServer.stop(resolve);
+      });
+    });
+    httpServer.start(CALLBACK_PORT);
+  });
+
+  let serverBin = _getBinaryUtil(serverBinName);
+  let process = Cc["@mozilla.org/process/util;1"].createInstance(Ci.nsIProcess);
+  process.init(serverBin);
+  let certDir = do_get_file(certsPath, false);
+  Assert.ok(certDir.exists(), `certificate folder (${certsPath}) should exist`);
+  // Using "sql:" causes the SQL DB to be used so we can run tests on Android.
+  process.run(false, ["sql:" + certDir.path, Services.appinfo.processID], 2);
+
+  registerCleanupFunction(function() {
+    process.kill();
+  });
+
+  await serverReady;
+}
+
+function _getBinaryUtil(binaryUtilName) {
+  let utilBin = Services.dirsvc.get("GreD", Ci.nsIFile);
+  // On macOS, GreD is .../Contents/Resources, and most binary utilities
+  // are located there, but certutil is in GreBinD (or .../Contents/MacOS),
+  // so we have to change the path accordingly.
+  if (binaryUtilName === "certutil") {
+    utilBin = Services.dirsvc.get("GreBinD", Ci.nsIFile);
+  }
+  utilBin.append(binaryUtilName + mozinfo.bin_suffix);
+  // If we're testing locally, the above works. If not, the server executable
+  // is in another location.
+  if (!utilBin.exists()) {
+    utilBin = Services.dirsvc.get("CurWorkD", Ci.nsIFile);
+    while (utilBin.path.includes("xpcshell")) {
+      utilBin = utilBin.parent;
+    }
+    utilBin.append("bin");
+    utilBin.append(binaryUtilName + mozinfo.bin_suffix);
+  }
+  // But maybe we're on Android, where binaries are in /data/local/xpcb.
+  if (!utilBin.exists()) {
+    utilBin.initWithPath("/data/local/xpcb/");
+    utilBin.append(binaryUtilName);
+  }
+  Assert.ok(utilBin.exists(), `Binary util ${binaryUtilName} should exist`);
+  return utilBin;
+}