diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index 9d6cfd7f919e..5924588279c1 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/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 diff --git a/netwerk/protocol/http/ConnectionEntry.cpp b/netwerk/protocol/http/ConnectionEntry.cpp index 7b59011cd3dc..dc0dfd103c5c 100644 --- a/netwerk/protocol/http/ConnectionEntry.cpp +++ b/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 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:"); diff --git a/netwerk/protocol/http/ConnectionEntry.h b/netwerk/protocol/http/ConnectionEntry.h index 8ccc126503df..6a265e58fceb 100644 --- a/netwerk/protocol/http/ConnectionEntry.h +++ b/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 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 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. diff --git a/netwerk/protocol/http/nsHttpConnectionMgr.cpp b/netwerk/protocol/http/nsHttpConnectionMgr.cpp index c7c385a42a71..dbbd8fe0cacd 100644 --- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp +++ b/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")); + } } } diff --git a/netwerk/test/unit/test_connection_coalescing.js b/netwerk/test/unit/test_connection_coalescing.js new file mode 100644 index 000000000000..a61098e73eb8 --- /dev/null +++ b/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(); + } +); diff --git a/netwerk/test/unit/xpcshell.toml b/netwerk/test/unit/xpcshell.toml index f0b5dfda4432..c86a1759e7c0 100644 --- a/netwerk/test/unit/xpcshell.toml +++ b/netwerk/test/unit/xpcshell.toml @@ -1277,4 +1277,6 @@ skip-if = [ ["test_xmlhttprequest.js"] +["test_connection_coalescing.js"] + ["test_default_uri_bypass.js"]