From 584002d1c89e79223f8bf5f973294b96bdfa15ab Mon Sep 17 00:00:00 2001 From: jri Date: Mon, 8 Sep 2014 17:51:28 -0700 Subject: [PATCH] Adds ability to disable connection pooling in QUIC. BUG= Review URL: https://codereview.chromium.org/551903002 Cr-Commit-Position: refs/heads/master@{#293823} --- chrome/browser/io_thread.cc | 12 ++ chrome/browser/io_thread.h | 5 + chrome/browser/io_thread_unittest.cc | 11 ++ net/http/http_network_session.cc | 2 + net/http/http_network_session.h | 1 + net/quic/quic_stream_factory.cc | 5 + net/quic/quic_stream_factory.h | 4 + net/quic/quic_stream_factory_test.cc | 206 ++++++++++++++++++++++++++- 8 files changed, 245 insertions(+), 1 deletion(-) diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc index 3abfe74886bc1a..e72ebd1c7284fd 100644 --- a/chrome/browser/io_thread.cc +++ b/chrome/browser/io_thread.cc @@ -1038,6 +1038,8 @@ void IOThread::InitializeNetworkSessionParamsFromGlobals( ¶ms->enable_quic_time_based_loss_detection); globals.quic_always_require_handshake_confirmation.CopyToIfSet( ¶ms->quic_always_require_handshake_confirmation); + globals.quic_disable_connection_pooling.CopyToIfSet( + ¶ms->quic_disable_connection_pooling); globals.enable_quic_port_selection.CopyToIfSet( ¶ms->enable_quic_port_selection); globals.quic_max_packet_length.CopyToIfSet(¶ms->quic_max_packet_length); @@ -1153,6 +1155,8 @@ void IOThread::ConfigureQuicGlobals( quic_trial_params)); globals->quic_always_require_handshake_confirmation.set( ShouldQuicAlwaysRequireHandshakeConfirmation(quic_trial_params)); + globals->quic_disable_connection_pooling.set( + ShouldQuicDisableConnectionPooling(quic_trial_params)); globals->enable_quic_port_selection.set( ShouldEnableQuicPortSelection(command_line)); globals->quic_connection_options = @@ -1336,6 +1340,14 @@ bool IOThread::ShouldQuicAlwaysRequireHandshakeConfirmation( "true"); } +// static +bool IOThread::ShouldQuicDisableConnectionPooling( + const VariationParameters& quic_trial_params) { + return LowerCaseEqualsASCII( + GetVariationParam(quic_trial_params, "disable_connection_pooling"), + "true"); +} + // static size_t IOThread::GetQuicMaxPacketLength( const CommandLine& command_line, diff --git a/chrome/browser/io_thread.h b/chrome/browser/io_thread.h index a989118415d9e0..68eb7cf68cf9ff 100644 --- a/chrome/browser/io_thread.h +++ b/chrome/browser/io_thread.h @@ -188,6 +188,7 @@ class IOThread : public content::BrowserThreadDelegate { Optional enable_quic_time_based_loss_detection; Optional enable_quic_port_selection; Optional quic_always_require_handshake_confirmation; + Optional quic_disable_connection_pooling; Optional quic_max_packet_length; net::QuicTagVector quic_connection_options; Optional quic_user_agent_id; @@ -358,6 +359,10 @@ class IOThread : public content::BrowserThreadDelegate { static bool ShouldQuicAlwaysRequireHandshakeConfirmation( const VariationParameters& quic_trial_params); + // Returns true if QUIC should disable connection pooling. + static bool ShouldQuicDisableConnectionPooling( + const VariationParameters& quic_trial_params); + // Returns the maximum length for QUIC packets, based on any flags in // |command_line| or the field trial. Returns 0 if there is an error // parsing any of the options, or if the default value should be used. diff --git a/chrome/browser/io_thread_unittest.cc b/chrome/browser/io_thread_unittest.cc index 173ff58bc6aa62..252474a9157128 100644 --- a/chrome/browser/io_thread_unittest.cc +++ b/chrome/browser/io_thread_unittest.cc @@ -133,6 +133,7 @@ TEST_F(IOThreadTest, EnableQuicFromFieldTrialGroup) { params.quic_supported_versions); EXPECT_EQ(net::QuicTagVector(), params.quic_connection_options); EXPECT_FALSE(params.quic_always_require_handshake_confirmation); + EXPECT_FALSE(params.quic_disable_connection_pooling); } TEST_F(IOThreadTest, EnableQuicFromCommandLine) { @@ -331,4 +332,14 @@ TEST_F(IOThreadTest, EXPECT_TRUE(params.quic_always_require_handshake_confirmation); } +TEST_F(IOThreadTest, + QuicDisableConnectionPoolingFromFieldTrialParams) { + field_trial_group_ = "Enabled"; + field_trial_params_["disable_connection_pooling"] = "true"; + ConfigureQuicGlobals(); + net::HttpNetworkSession::Params params; + InitializeNetworkSessionParams(¶ms); + EXPECT_TRUE(params.quic_disable_connection_pooling); +} + } // namespace test diff --git a/net/http/http_network_session.cc b/net/http/http_network_session.cc index 776df1525ab156..b7a4b747b08e8b 100644 --- a/net/http/http_network_session.cc +++ b/net/http/http_network_session.cc @@ -91,6 +91,7 @@ HttpNetworkSession::Params::Params() enable_quic_port_selection(true), enable_quic_time_based_loss_detection(false), quic_always_require_handshake_confirmation(false), + quic_disable_connection_pooling(false), quic_clock(NULL), quic_random(NULL), quic_max_packet_length(kDefaultMaxPacketSize), @@ -132,6 +133,7 @@ HttpNetworkSession::HttpNetworkSession(const Params& params) params.enable_quic_port_selection, params.enable_quic_time_based_loss_detection, params.quic_always_require_handshake_confirmation, + params.quic_disable_connection_pooling, params.quic_connection_options), spdy_session_pool_(params.host_resolver, params.ssl_config_service, diff --git a/net/http/http_network_session.h b/net/http/http_network_session.h index da8ac72a28a908..d1e4234a5b8d8d 100644 --- a/net/http/http_network_session.h +++ b/net/http/http_network_session.h @@ -112,6 +112,7 @@ class NET_EXPORT HttpNetworkSession bool enable_quic_port_selection; bool enable_quic_time_based_loss_detection; bool quic_always_require_handshake_confirmation; + bool quic_disable_connection_pooling; HostPortPair origin_to_force_quic_on; QuicClock* quic_clock; // Will be owned by QuicStreamFactory. QuicRandom* quic_random; diff --git a/net/quic/quic_stream_factory.cc b/net/quic/quic_stream_factory.cc index 51b3b9df919ffb..545db5b3928a08 100644 --- a/net/quic/quic_stream_factory.cc +++ b/net/quic/quic_stream_factory.cc @@ -486,6 +486,7 @@ QuicStreamFactory::QuicStreamFactory( bool enable_port_selection, bool enable_time_based_loss_detection, bool always_require_handshake_confirmation, + bool disable_connection_pooling, const QuicTagVector& connection_options) : require_confirmation_(true), host_resolver_(host_resolver), @@ -503,6 +504,7 @@ QuicStreamFactory::QuicStreamFactory( enable_port_selection_(enable_port_selection), always_require_handshake_confirmation_( always_require_handshake_confirmation), + disable_connection_pooling_(disable_connection_pooling), port_seed_(random_generator_->RandUint64()), weak_factory_(this) { DCHECK(transport_security_state_); @@ -584,6 +586,9 @@ bool QuicStreamFactory::OnResolution( const QuicServerId& server_id, const AddressList& address_list) { DCHECK(!HasActiveSession(server_id)); + if (disable_connection_pooling_) { + return false; + } for (size_t i = 0; i < address_list.size(); ++i) { const IPEndPoint& address = address_list[i]; const IpAliasKey ip_alias_key(address, server_id.is_https()); diff --git a/net/quic/quic_stream_factory.h b/net/quic/quic_stream_factory.h index 330f5f53ea803f..c5aa835a4b8869 100644 --- a/net/quic/quic_stream_factory.h +++ b/net/quic/quic_stream_factory.h @@ -104,6 +104,7 @@ class NET_EXPORT_PRIVATE QuicStreamFactory bool enable_port_selection, bool enable_time_based_loss_detection, bool always_require_handshake_confirmation, + bool disable_connection_pooling, const QuicTagVector& connection_options); virtual ~QuicStreamFactory(); @@ -281,6 +282,9 @@ class NET_EXPORT_PRIVATE QuicStreamFactory // introduce at least one RTT for the handshake before the client sends data. bool always_require_handshake_confirmation_; + // Set if we do not want connection pooling. + bool disable_connection_pooling_; + // Each profile will (probably) have a unique port_seed_ value. This value is // used to help seed a pseudo-random number generator (PortSuggester) so that // we consistently (within this profile) suggest the same ephemeral port when diff --git a/net/quic/quic_stream_factory_test.cc b/net/quic/quic_stream_factory_test.cc index 76501847436fec..374eda4cc74899 100644 --- a/net/quic/quic_stream_factory_test.cc +++ b/net/quic/quic_stream_factory_test.cc @@ -84,6 +84,10 @@ class QuicStreamFactoryPeer { } return false; } + + static void DisableConnectionPooling(QuicStreamFactory* factory) { + factory->disable_connection_pooling_ = true; + } }; class QuicStreamFactoryTest : public ::testing::TestWithParam { @@ -110,7 +114,8 @@ class QuicStreamFactoryTest : public ::testing::TestWithParam { SupportedVersions(GetParam()), /*enable_port_selection=*/true, /*enable_time_based_loss_detection=*/true, - /*always_require_handshake_confirmation=*/true, + /*always_require_handshake_confirmation=*/false, + /*disable_connection_pooling=*/false, QuicTagVector()), host_port_pair_(kDefaultServerHostName, kDefaultServerPort), is_https_(false), @@ -418,6 +423,61 @@ TEST_P(QuicStreamFactoryTest, Pooling) { EXPECT_TRUE(socket_data.at_write_eof()); } +TEST_P(QuicStreamFactoryTest, NoPoolingIfDisabled) { + MockRead reads[] = { + MockRead(ASYNC, OK, 0) // EOF + }; + DeterministicSocketData socket_data1(reads, arraysize(reads), NULL, 0); + DeterministicSocketData socket_data2(reads, arraysize(reads), NULL, 0); + socket_factory_.AddSocketDataProvider(&socket_data1); + socket_factory_.AddSocketDataProvider(&socket_data2); + socket_data1.StopAfter(1); + socket_data2.StopAfter(1); + + HostPortPair server2("mail.google.com", kDefaultServerPort); + host_resolver_.set_synchronous_mode(true); + host_resolver_.rules()->AddIPLiteralRule( + kDefaultServerHostName, "192.168.0.1", ""); + host_resolver_.rules()->AddIPLiteralRule( + "mail.google.com", "192.168.0.1", ""); + + // Disable connection pooling. + QuicStreamFactoryPeer::DisableConnectionPooling(&factory_); + + QuicStreamRequest request(&factory_); + EXPECT_EQ(OK, + request.Request(host_port_pair_, + is_https_, + privacy_mode_, + "GET", + net_log_, + callback_.callback())); + scoped_ptr stream = request.ReleaseStream(); + EXPECT_TRUE(stream.get()); + + TestCompletionCallback callback; + QuicStreamRequest request2(&factory_); + EXPECT_EQ(OK, + request2.Request(server2, + is_https_, + privacy_mode_, + "GET", + net_log_, + callback.callback())); + scoped_ptr stream2 = request2.ReleaseStream(); + EXPECT_TRUE(stream2.get()); + + EXPECT_NE( + QuicStreamFactoryPeer::GetActiveSession( + &factory_, host_port_pair_, is_https_), + QuicStreamFactoryPeer::GetActiveSession(&factory_, server2, is_https_)); + + EXPECT_TRUE(socket_data1.at_read_eof()); + EXPECT_TRUE(socket_data1.at_write_eof()); + EXPECT_TRUE(socket_data2.at_read_eof()); + EXPECT_TRUE(socket_data2.at_write_eof()); +} + TEST_P(QuicStreamFactoryTest, NoPoolingAfterGoAway) { MockRead reads[] = { MockRead(ASYNC, OK, 0) // EOF @@ -548,6 +608,75 @@ TEST_P(QuicStreamFactoryTest, HttpsPooling) { EXPECT_TRUE(socket_data.at_write_eof()); } +TEST_P(QuicStreamFactoryTest, NoHttpsPoolingIfDisabled) { + MockRead reads[] = { + MockRead(ASYNC, OK, 0) // EOF + }; + DeterministicSocketData socket_data1(reads, arraysize(reads), NULL, 0); + DeterministicSocketData socket_data2(reads, arraysize(reads), NULL, 0); + socket_factory_.AddSocketDataProvider(&socket_data1); + socket_factory_.AddSocketDataProvider(&socket_data2); + socket_data1.StopAfter(1); + socket_data2.StopAfter(1); + + HostPortPair server1("www.example.org", 443); + HostPortPair server2("mail.example.org", 443); + + // Load a cert that is valid for: + // www.example.org (server1) + // mail.example.org (server2) + // www.example.com + base::FilePath certs_dir = GetTestCertsDirectory(); + scoped_refptr test_cert( + ImportCertFromFile(certs_dir, "spdy_pooling.pem")); + ASSERT_NE(static_cast(NULL), test_cert.get()); + ProofVerifyDetailsChromium verify_details; + verify_details.cert_verify_result.verified_cert = test_cert; + verify_details.cert_verify_result.is_issued_by_known_root = true; + crypto_client_stream_factory_.set_proof_verify_details(&verify_details); + + host_resolver_.set_synchronous_mode(true); + host_resolver_.rules()->AddIPLiteralRule(server1.host(), "192.168.0.1", ""); + host_resolver_.rules()->AddIPLiteralRule(server2.host(), "192.168.0.1", ""); + + // Disable connection pooling. + QuicStreamFactoryPeer::DisableConnectionPooling(&factory_); + + QuicStreamRequest request(&factory_); + is_https_ = true; + EXPECT_EQ(OK, + request.Request(server1, + is_https_, + privacy_mode_, + "GET", + net_log_, + callback_.callback())); + scoped_ptr stream = request.ReleaseStream(); + EXPECT_TRUE(stream.get()); + + TestCompletionCallback callback; + QuicStreamRequest request2(&factory_); + EXPECT_EQ(OK, + request2.Request(server2, + is_https_, + privacy_mode_, + "GET", + net_log_, + callback_.callback())); + scoped_ptr stream2 = request2.ReleaseStream(); + EXPECT_TRUE(stream2.get()); + + EXPECT_NE(QuicStreamFactoryPeer::GetActiveSession( + &factory_, server1, is_https_), + QuicStreamFactoryPeer::GetActiveSession( + &factory_, server2, is_https_)); + + EXPECT_TRUE(socket_data1.at_read_eof()); + EXPECT_TRUE(socket_data1.at_write_eof()); + EXPECT_TRUE(socket_data2.at_read_eof()); + EXPECT_TRUE(socket_data2.at_write_eof()); +} + TEST_P(QuicStreamFactoryTest, NoHttpsPoolingWithCertMismatch) { MockRead reads[] = { MockRead(ASYNC, OK, 0) // EOF @@ -682,6 +811,81 @@ TEST_P(QuicStreamFactoryTest, HttpsPoolingWithMatchingPins) { EXPECT_TRUE(socket_data.at_write_eof()); } +TEST_P(QuicStreamFactoryTest, NoHttpsPoolingWithMatchingPinsIfDisabled) { + MockRead reads[] = { + MockRead(ASYNC, OK, 0) // EOF + }; + DeterministicSocketData socket_data1(reads, arraysize(reads), NULL, 0); + DeterministicSocketData socket_data2(reads, arraysize(reads), NULL, 0); + socket_factory_.AddSocketDataProvider(&socket_data1); + socket_factory_.AddSocketDataProvider(&socket_data2); + socket_data1.StopAfter(1); + socket_data2.StopAfter(1); + + HostPortPair server1("www.example.org", 443); + HostPortPair server2("mail.example.org", 443); + uint8 primary_pin = 1; + uint8 backup_pin = 2; + test::AddPin(&transport_security_state_, "mail.example.org", primary_pin, + backup_pin); + + // Load a cert that is valid for: + // www.example.org (server1) + // mail.example.org (server2) + base::FilePath certs_dir = GetTestCertsDirectory(); + scoped_refptr test_cert( + ImportCertFromFile(certs_dir, "spdy_pooling.pem")); + ASSERT_NE(static_cast(NULL), test_cert.get()); + ProofVerifyDetailsChromium verify_details; + verify_details.cert_verify_result.verified_cert = test_cert; + verify_details.cert_verify_result.is_issued_by_known_root = true; + verify_details.cert_verify_result.public_key_hashes.push_back( + test::GetTestHashValue(primary_pin)); + crypto_client_stream_factory_.set_proof_verify_details(&verify_details); + + + host_resolver_.set_synchronous_mode(true); + host_resolver_.rules()->AddIPLiteralRule(server1.host(), "192.168.0.1", ""); + host_resolver_.rules()->AddIPLiteralRule(server2.host(), "192.168.0.1", ""); + + // Disable connection pooling. + QuicStreamFactoryPeer::DisableConnectionPooling(&factory_); + + QuicStreamRequest request(&factory_); + is_https_ = true; + EXPECT_EQ(OK, + request.Request(server1, + is_https_, + privacy_mode_, + "GET", + net_log_, + callback_.callback())); + scoped_ptr stream = request.ReleaseStream(); + EXPECT_TRUE(stream.get()); + + TestCompletionCallback callback; + QuicStreamRequest request2(&factory_); + EXPECT_EQ(OK, + request2.Request(server2, + is_https_, + privacy_mode_, + "GET", + net_log_, + callback_.callback())); + scoped_ptr stream2 = request2.ReleaseStream(); + EXPECT_TRUE(stream2.get()); + + EXPECT_NE(QuicStreamFactoryPeer::GetActiveSession( + &factory_, server1, is_https_), + QuicStreamFactoryPeer::GetActiveSession( + &factory_, server2, is_https_)); + + EXPECT_TRUE(socket_data1.at_read_eof()); + EXPECT_TRUE(socket_data1.at_write_eof()); + EXPECT_TRUE(socket_data2.at_read_eof()); + EXPECT_TRUE(socket_data2.at_write_eof()); +} + TEST_P(QuicStreamFactoryTest, NoHttpsPoolingWithDifferentPins) { MockRead reads[] = { MockRead(ASYNC, OK, 0) // EOF