Bug 1420777 - Coalesce connections less aggresively r=necko-reviewers,kershaw

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

modules/libpref/init/StaticPrefList.yaml

@@ -12847,6 +12847,16 @@
   value: true
   mirror: always
 
+# When true, origin A and origin B will be coalesced if they have an overlap
+# in IP addresses as advertized by DNS, regardless if the existing connection
+# to origin A is not to an IP present in B's DNS response.
+# When false, an existing connection will only be reused if the
+# connection's remote IP is also present in B's DNS response.
+- name: network.http.http2.aggressive_coalescing
+  type: RelaxedAtomicBool
+  value: false
+  mirror: always
+
 - name: network.http.http2.ping-threshold
   type: RelaxedAtomicInt32
   value: 58

netwerk/protocol/http/ConnectionEntry.cpp

@@ -124,6 +124,7 @@ void ConnectionEntry::DisallowHttp2() {
 
   // Can't coalesce if we're not using spdy
   mCoalescingKeys.Clear();
+  mAddresses.Clear();
 }
 
 void ConnectionEntry::DontReuseHttp3Conn() {
@@ -137,6 +138,7 @@ void ConnectionEntry::DontReuseHttp3Conn() {
 
   // Can't coalesce if we're not using http3
   mCoalescingKeys.Clear();
+  mAddresses.Clear();
 }
 
 void ConnectionEntry::RecordIPFamilyPreference(uint16_t family) {
@@ -447,6 +449,7 @@ void ConnectionEntry::ClosePersistentConnections() {
   }
 
   mCoalescingKeys.Clear();
+  mAddresses.Clear();
 }
 
 uint32_t ConnectionEntry::PruneDeadConnections() {
@@ -969,18 +972,16 @@ bool ConnectionEntry::MaybeProcessCoalescingKeys(nsIDNSAddrRecord* dnsRecord,
     return false;
   }
 
-  nsTArray<NetAddr> addressSet;
-  nsresult rv = dnsRecord->GetAddresses(addressSet);
-
-  if (NS_FAILED(rv) || addressSet.IsEmpty()) {
+  nsresult rv = dnsRecord->GetAddresses(mAddresses);
+  if (NS_FAILED(rv) || mAddresses.IsEmpty()) {
     return false;
   }
 
-  for (uint32_t i = 0; i < addressSet.Length(); ++i) {
-    if ((addressSet[i].raw.family == AF_INET && addressSet[i].inet.ip == 0) ||
-        (addressSet[i].raw.family == AF_INET6 &&
-         addressSet[i].inet6.ip.u64[0] == 0 &&
-         addressSet[i].inet6.ip.u64[1] == 0)) {
+  for (uint32_t i = 0; i < mAddresses.Length(); ++i) {
+    if ((mAddresses[i].raw.family == AF_INET && mAddresses[i].inet.ip == 0) ||
+        (mAddresses[i].raw.family == AF_INET6 &&
+         mAddresses[i].inet6.ip.u64[0] == 0 &&
+         mAddresses[i].inet6.ip.u64[1] == 0)) {
       // Bug 1680249 - Don't create the coalescing key if the ip address is
       // `0.0.0.0` or `::`.
       LOG(
@@ -991,7 +992,7 @@ bool ConnectionEntry::MaybeProcessCoalescingKeys(nsIDNSAddrRecord* dnsRecord,
     }
     nsCString* newKey = mCoalescingKeys.AppendElement(nsCString());
     newKey->SetLength(kIPv6CStrBufSize + 26);
-    addressSet[i].ToStringBuffer(newKey->BeginWriting(), kIPv6CStrBufSize);
+    mAddresses[i].ToStringBuffer(newKey->BeginWriting(), kIPv6CStrBufSize);
     newKey->SetLength(strlen(newKey->BeginReading()));
     if (mConnInfo->GetAnonymous()) {
       newKey->AppendLiteral("~A:");

netwerk/protocol/http/ConnectionEntry.h

@@ -121,8 +121,14 @@ class ConnectionEntry {
   // combined with the Anonymous flag and OA from the connection information
   // to build the hash key for hosts in the same ip pool.
   //
+
   nsTArray<nsCString> mCoalescingKeys;
 
+  // This is a list of addresses matching the coalescing keys.
+  // This is necessary to check if the origin's DNS entries
+  // contain the IP address of the active connection.
+  nsTArray<NetAddr> mAddresses;
+
   // To have the UsingSpdy flag means some host with the same connection
   // entry has done NPN=spdy/* at some point. It does not mean every
   // connection is currently using spdy.

netwerk/protocol/http/nsHttpConnectionMgr.cpp

@@ -817,10 +817,35 @@ HttpConnectionBase* nsHttpConnectionMgr::FindCoalescableConnection(
   for (uint32_t i = 0; i < keyLen; ++i) {
     conn = FindCoalescableConnectionByHashKey(ent, ent->mCoalescingKeys[i],
                                               justKidding, aNoHttp2, aNoHttp3);
+
+    auto usableEntry = [&](HttpConnectionBase* conn) {
+      // This is allowed by the spec, but other browsers don't coalesce
+      // so agressively, which surprises developers. See bug 1420777.
+      if (StaticPrefs::network_http_http2_aggressive_coalescing()) {
+        return true;
+      }
+
+      // Make sure that the connection's IP address is one that is in
+      // the set of IP addresses in the entry's DNS response.
+      NetAddr addr;
+      nsresult rv = conn->GetPeerAddr(&addr);
+      if (NS_FAILED(rv)) {
+        // Err on the side of not coalescing
+        return false;
+      }
+      // We don't care about remote port when matching entries.
+      addr.inet.port = 0;
+      return ent->mAddresses.Contains(addr);
+    };
+
     if (conn) {
-      LOG(("FindCoalescableConnection(%s) match conn %p on dns key %s\n",
-           ci->HashKey().get(), conn, ent->mCoalescingKeys[i].get()));
-      return conn;
+      LOG(("Found connection with matching hash"));
+      if (usableEntry(conn)) {
+        LOG(("> coalescing"));
+        return conn;
+      } else {
+        LOG(("> not coalescing as remote address not present in DNS records"));
+      }
     }
   }
 

netwerk/test/unit/test_connection_coalescing.js

@@ -0,0 +1,194 @@
+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* 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";
+
+const override = Cc["@mozilla.org/network/native-dns-override;1"].getService(
+  Ci.nsINativeDNSResolverOverride
+);
+
+let certdb = Cc["@mozilla.org/security/x509certdb;1"].getService(
+  Ci.nsIX509CertDB
+);
+addCertFromFile(certdb, "http2-ca.pem", "CTu,u,u");
+
+async function createServer() {
+  let server = new NodeHTTP2Server();
+  await server.start();
+  registerCleanupFunction(async () => {
+    await server.stop();
+  });
+  await server.registerPathHandler("/", (req, resp) => {
+    let content = `hello from ${req.authority} | ${req.socket.remotePort}`;
+    resp.writeHead(200, {
+      "Content-Type": "text/plain",
+      "Content-Length": `${content.length}`,
+    });
+    resp.end(content);
+  });
+  return server;
+}
+
+let IP1 = "127.0.0.1";
+let IP2 = "127.0.0.2";
+if (AppConstants.platform == "macosx") {
+  // OSX doesn't use 127.0.0.2 as a local interface
+  IP2 = "::1";
+} else if (AppConstants.platform == "android") {
+  IP2 = "10.0.2.2";
+}
+
+async function openChan(uri) {
+  let chan = NetUtil.newChannel({
+    uri,
+    loadUsingSystemPrincipal: true,
+  }).QueryInterface(Ci.nsIHttpChannel);
+  chan.loadFlags = Ci.nsIChannel.LOAD_INITIAL_DOCUMENT_URI;
+
+  let { req, buffer } = await new Promise(resolve => {
+    function finish(r, b) {
+      resolve({ req: r, buffer: b });
+    }
+    chan.asyncOpen(new ChannelListener(finish, null, CL_ALLOW_UNKNOWN_CL));
+  });
+
+  return {
+    buffer,
+    port: buffer.split("|")[1],
+    addr: req.QueryInterface(Ci.nsIHttpChannelInternal).remoteAddress,
+    status: req.QueryInterface(Ci.nsIHttpChannel).responseStatus,
+  };
+}
+
+add_task(async function test_dontCoalesce() {
+  let server = await createServer();
+  Services.prefs.setBoolPref("network.http.http2.aggressive_coalescing", false);
+  override.clearOverrides();
+  Services.dns.clearCache(true);
+
+  override.addIPOverride("foo.example.com", IP1);
+  override.addIPOverride("foo.example.com", IP2);
+  override.addIPOverride("alt1.example.com", IP2);
+
+  let { addr: addr1 } = await openChan(
+    `https://foo.example.com:${server.port()}/`
+  );
+  let { addr: addr2 } = await openChan(
+    `https://alt1.example.com:${server.port()}/`
+  );
+
+  Assert.notEqual(addr1, addr2);
+  await server.stop();
+});
+
+add_task(async function test_doCoalesce() {
+  let server = await createServer();
+  Services.prefs.setBoolPref("network.http.http2.aggressive_coalescing", false);
+  override.clearOverrides();
+  Services.dns.clearCache(true);
+
+  override.addIPOverride("foo.example.com", IP1);
+  override.addIPOverride("foo.example.com", IP2);
+  override.addIPOverride("alt2.example.com", IP1);
+  override.addIPOverride("alt2.example.com", IP2);
+
+  let { port: port1, addr: addr1 } = await openChan(
+    `https://foo.example.com:${server.port()}/`
+  );
+  let { port: port2, addr: addr2 } = await openChan(
+    `https://alt2.example.com:${server.port()}/`
+  );
+
+  Assert.equal(addr1, addr2);
+  Assert.equal(port1, port2);
+  await server.stop();
+});
+
+add_task(async function test_doCoalesceAggresive() {
+  let server = await createServer();
+
+  Services.prefs.setBoolPref("network.http.http2.aggressive_coalescing", true);
+  override.clearOverrides();
+  Services.dns.clearCache(true);
+
+  override.addIPOverride("foo.example.com", IP1);
+  override.addIPOverride("foo.example.com", IP2);
+  override.addIPOverride("alt1.example.com", IP2);
+
+  let { port: port1, addr: addr1 } = await openChan(
+    `https://foo.example.com:${server.port()}/`
+  );
+  let { port: port2, addr: addr2 } = await openChan(
+    `https://alt1.example.com:${server.port()}/`
+  );
+
+  Assert.equal(addr1, addr2);
+  Assert.equal(port1, port2);
+  await server.stop();
+});
+
+// On android because of the way networking is set up the
+// localAddress is always ::ffff:127.0.0.1 so it can't be
+// used to make a decision.
+add_task(
+  { skip_if: () => AppConstants.platform == "android" },
+  async function test_doCoalesceAggresive421() {
+    let server = await createServer();
+
+    await server.execute(`global.rightIP = "${IP2}"`);
+
+    await server.registerPathHandler("/", (req, resp) => {
+      let content = `hello from ${req.authority} | ${req.socket.remotePort}`;
+      // Check that returning 421 when aggresively coalescing
+      // makes Firefox not coalesce the connections.
+      if (
+        req.authority.startsWith("alt1.example.com") &&
+        req.socket.localAddress != global.rightIP &&
+        req.socket.localAddress != `::ffff:${global.rightIP}`
+      ) {
+        resp.writeHead(421, {
+          "Content-Type": "text/plain",
+          "Content-Length": `${content.length}`,
+        });
+        resp.end(content);
+        return;
+      }
+      resp.writeHead(200, {
+        "Content-Type": "text/plain",
+        "Content-Length": `${content.length}`,
+      });
+      resp.end(content);
+    });
+
+    Services.prefs.setBoolPref(
+      "network.http.http2.aggressive_coalescing",
+      true
+    );
+    override.clearOverrides();
+    Services.dns.clearCache(true);
+
+    override.addIPOverride("foo.example.com", IP1);
+    override.addIPOverride("foo.example.com", IP2);
+    override.addIPOverride("alt1.example.com", IP2);
+
+    let {
+      addr: addr1,
+      status: status1,
+      port: port1,
+    } = await openChan(`https://foo.example.com:${server.port()}/`);
+    Assert.equal(status1, 200);
+    Assert.equal(addr1, IP1);
+    let {
+      addr: addr2,
+      status: status2,
+      port: port2,
+    } = await openChan(`https://alt1.example.com:${server.port()}/`);
+
+    Assert.equal(status2, 200);
+    Assert.equal(addr2, IP2);
+    Assert.notEqual(port1, port2);
+    await server.stop();
+  }
+);

netwerk/test/unit/xpcshell.toml

@@ -1277,4 +1277,6 @@ skip-if = [
 
 ["test_xmlhttprequest.js"]
 
+["test_connection_coalescing.js"]
+
 ["test_default_uri_bypass.js"]