From b8e80fb5d475ac2b98fd87b25a111d3c4b0e28b0 Mon Sep 17 00:00:00 2001 From: rtenneti Date: Sun, 15 May 2016 17:12:09 -0700 Subject: [PATCH] Revert of QUIC - enable "delay_tcp_race" parameter by default. This feature showed (patchset #3 id:60001 of https://codereview.chromium.org/1916783003/ ) Reason for revert: Reverting this CL so that we could experiment delay_tcp_race. Original issue's description: > QUIC - enable "delay_tcp_race" parameter by default. This feature showed > positive results in stable channel and it will be enabled by default > via finch trial in Stable and Beta (M50 and M51). > > R=rch@chromium.org,xunjieli@chromium.org > > Committed: https://crrev.com/634050c3f304a48f62c6ceb022a7f83945ea1968 > Cr-Commit-Position: refs/heads/master@{#390468} TBR=rch@chromium.org,xunjieli@chromium.org Review-Url: https://codereview.chromium.org/1977303002 Cr-Commit-Position: refs/heads/master@{#393778} --- chrome/browser/io_thread.cc | 8 +++ chrome/browser/io_thread.h | 4 ++ chrome/browser/io_thread_unittest.cc | 12 ++++ .../src/org/chromium/net/QuicTest.java | 1 + .../cronet/url_request_context_config.cc | 6 ++ .../url_request_context_config_unittest.cc | 4 ++ ios/chrome/browser/ios_chrome_io_thread.h | 5 ++ ios/chrome/browser/ios_chrome_io_thread.mm | 8 +++ net/http/http_network_session.cc | 4 +- net/http/http_network_session.h | 3 + net/http/http_stream_factory_impl_unittest.cc | 4 +- net/quic/quic_network_transaction_unittest.cc | 55 +++++++++++++++++-- net/quic/quic_stream_factory.cc | 4 +- net/quic/quic_stream_factory.h | 6 ++ net/quic/quic_stream_factory_test.cc | 15 ++++- .../test_tools/quic_stream_factory_peer.cc | 9 +++ .../test_tools/quic_stream_factory_peer.h | 4 ++ .../url_request_context_builder.cc | 3 + net/url_request/url_request_context_builder.h | 5 ++ 19 files changed, 150 insertions(+), 10 deletions(-) diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc index 78ce6c8a35a8bc..5d6949b978786d 100644 --- a/chrome/browser/io_thread.cc +++ b/chrome/browser/io_thread.cc @@ -1199,6 +1199,7 @@ void IOThread::NetworkSessionConfigurator::ConfigureQuicParams( if (receive_buffer_size != 0) { params->quic_socket_receive_buffer_size = receive_buffer_size; } + params->quic_delay_tcp_race = ShouldQuicDelayTcpRace(quic_trial_params); float load_server_info_timeout_srtt_multiplier = GetQuicLoadServerInfoTimeoutSrttMultiplier(quic_trial_params); if (load_server_info_timeout_srtt_multiplier != 0) { @@ -1447,6 +1448,13 @@ int IOThread::NetworkSessionConfigurator::GetQuicSocketReceiveBufferSize( return 0; } +// static +bool IOThread::NetworkSessionConfigurator::ShouldQuicDelayTcpRace( + const VariationParameters& quic_trial_params) { + return base::LowerCaseEqualsASCII( + GetVariationParam(quic_trial_params, "delay_tcp_race"), "true"); +} + // static bool IOThread::NetworkSessionConfigurator::ShouldQuicCloseSessionsOnIpChange( const VariationParameters& quic_trial_params) { diff --git a/chrome/browser/io_thread.h b/chrome/browser/io_thread.h index b9a190dafc1cc7..85210ba1abdb46 100644 --- a/chrome/browser/io_thread.h +++ b/chrome/browser/io_thread.h @@ -330,6 +330,10 @@ class IOThread : public content::BrowserThreadDelegate { static int GetQuicSocketReceiveBufferSize( const VariationParameters& quic_trial_params); + // Returns true if QUIC should delay TCP connection when QUIC works. + static bool ShouldQuicDelayTcpRace( + const VariationParameters& quic_trial_params); + // Returns true if QUIC should close sessions when any of the client's IP // addresses change. static bool ShouldQuicCloseSessionsOnIpChange( diff --git a/chrome/browser/io_thread_unittest.cc b/chrome/browser/io_thread_unittest.cc index ebc273e58dfc21..dca30be5f2ae7a 100644 --- a/chrome/browser/io_thread_unittest.cc +++ b/chrome/browser/io_thread_unittest.cc @@ -251,6 +251,7 @@ TEST_F(NetworkSessionConfiguratorTest, EnableQuicFromFieldTrialGroup) { EXPECT_FALSE(params_.enable_alternative_service_with_different_host); EXPECT_EQ(0, params_.quic_max_number_of_lossy_connections); EXPECT_EQ(1.0f, params_.quic_packet_loss_threshold); + EXPECT_FALSE(params_.quic_delay_tcp_race); EXPECT_FALSE(params_.quic_close_sessions_on_ip_change); EXPECT_EQ(net::kIdleConnectionTimeoutSeconds, params_.quic_idle_connection_timeout_seconds); @@ -576,6 +577,17 @@ TEST_F(NetworkSessionConfiguratorTest, QuicReceiveBufferSize) { EXPECT_EQ(2097152, params_.quic_socket_receive_buffer_size); } +TEST_F(NetworkSessionConfiguratorTest, QuicDelayTcpConnection) { + std::map field_trial_params; + field_trial_params["delay_tcp_race"] = "true"; + variations::AssociateVariationParams("QUIC", "Enabled", field_trial_params); + base::FieldTrialList::CreateFieldTrial("QUIC", "Enabled"); + + ParseFieldTrials(); + + EXPECT_TRUE(params_.quic_delay_tcp_race); +} + TEST_F(NetworkSessionConfiguratorTest, QuicOriginsToForceQuicOn) { base::CommandLine::ForCurrentProcess()->AppendSwitch("enable-quic"); base::CommandLine::ForCurrentProcess()->AppendSwitchASCII( diff --git a/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java b/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java index 5dba044acf655e..0e220e26967e58 100644 --- a/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java +++ b/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java @@ -43,6 +43,7 @@ protected void setUp() throws Exception { .put("connection_options", "PACE,IW10,FOO,DEADBEEF") .put("host_whitelist", "test.example.com") .put("max_server_configs_stored_in_properties", 2) + .put("delay_tcp_race", true) .put("max_number_of_lossy_connections", 10) .put("packet_loss_threshold", 0.5) .put("idle_connection_timeout_seconds", 300) diff --git a/components/cronet/url_request_context_config.cc b/components/cronet/url_request_context_config.cc index dae813dbf51f7a..570173b1504c03 100644 --- a/components/cronet/url_request_context_config.cc +++ b/components/cronet/url_request_context_config.cc @@ -34,6 +34,7 @@ const char kQuicStoreServerConfigsInProperties[] = "store_server_configs_in_properties"; const char kQuicMaxServerConfigsStoredInProperties[] = "max_server_configs_stored_in_properties"; +const char kQuicDelayTcpRace[] = "delay_tcp_race"; const char kQuicMaxNumberOfLossyConnections[] = "max_number_of_lossy_connections"; const char kQuicPacketLossThreshold[] = "packet_loss_threshold"; @@ -108,6 +109,11 @@ void ParseAndSetExperimentalOptions( static_cast(quic_max_server_configs_stored_in_properties)); } + bool quic_delay_tcp_race = false; + if (quic_args->GetBoolean(kQuicDelayTcpRace, &quic_delay_tcp_race)) { + context_builder->set_quic_delay_tcp_race(quic_delay_tcp_race); + } + int quic_max_number_of_lossy_connections = 0; if (quic_args->GetInteger(kQuicMaxNumberOfLossyConnections, &quic_max_number_of_lossy_connections)) { diff --git a/components/cronet/url_request_context_config_unittest.cc b/components/cronet/url_request_context_config_unittest.cc index cd114aff46e3e8..c129a8fe236bd4 100644 --- a/components/cronet/url_request_context_config_unittest.cc +++ b/components/cronet/url_request_context_config_unittest.cc @@ -39,6 +39,7 @@ TEST(URLRequestContextConfigTest, SetQuicExperimentalOptions) { "fake agent", // JSON encoded experimental options. "{\"QUIC\":{\"max_server_configs_stored_in_properties\":2," + "\"delay_tcp_race\":true," "\"max_number_of_lossy_connections\":10," "\"prefer_aes\":true," "\"user_agent_id\":\"Custom QUIC UAID\"," @@ -80,6 +81,9 @@ TEST(URLRequestContextConfigTest, SetQuicExperimentalOptions) { // Check max_server_configs_stored_in_properties. EXPECT_EQ(2u, params->quic_max_server_configs_stored_in_properties); + // Check delay_tcp_race. + EXPECT_TRUE(params->quic_delay_tcp_race); + // Check prefer_aes. EXPECT_TRUE(params->quic_prefer_aes); diff --git a/ios/chrome/browser/ios_chrome_io_thread.h b/ios/chrome/browser/ios_chrome_io_thread.h index 3b4749ee1c43d0..dc45330ea70d45 100644 --- a/ios/chrome/browser/ios_chrome_io_thread.h +++ b/ios/chrome/browser/ios_chrome_io_thread.h @@ -155,6 +155,7 @@ class IOSChromeIOThread : public web::WebThreadDelegate { Optional quic_max_number_of_lossy_connections; Optional quic_packet_loss_threshold; Optional quic_socket_receive_buffer_size; + Optional quic_delay_tcp_race; Optional quic_max_packet_length; net::QuicTagVector quic_connection_options; Optional quic_user_agent_id; @@ -318,6 +319,10 @@ class IOSChromeIOThread : public web::WebThreadDelegate { static int GetQuicSocketReceiveBufferSize( const VariationParameters& quic_trial_params); + // Returns true if QUIC should delay TCP connection when QUIC works. + static bool ShouldQuicDelayTcpRace( + const VariationParameters& quic_trial_params); + // Returns true if QUIC should close sessions when any of the client's // IP addresses change. static bool ShouldQuicCloseSessionsOnIpChange( diff --git a/ios/chrome/browser/ios_chrome_io_thread.mm b/ios/chrome/browser/ios_chrome_io_thread.mm index f998fd68b371e0..fa32840b393a0f 100644 --- a/ios/chrome/browser/ios_chrome_io_thread.mm +++ b/ios/chrome/browser/ios_chrome_io_thread.mm @@ -663,6 +663,7 @@ void OnNetworkChanged( ¶ms->quic_packet_loss_threshold); globals.quic_socket_receive_buffer_size.CopyToIfSet( ¶ms->quic_socket_receive_buffer_size); + globals.quic_delay_tcp_race.CopyToIfSet(¶ms->quic_delay_tcp_race); params->enable_quic_port_selection = false; globals.quic_max_packet_length.CopyToIfSet(¶ms->quic_max_packet_length); globals.quic_user_agent_id.CopyToIfSet(¶ms->quic_user_agent_id); @@ -768,6 +769,7 @@ void OnNetworkChanged( if (receive_buffer_size != 0) { globals->quic_socket_receive_buffer_size.set(receive_buffer_size); } + globals->quic_delay_tcp_race.set(ShouldQuicDelayTcpRace(quic_trial_params)); float load_server_info_timeout_srtt_multiplier = GetQuicLoadServerInfoTimeoutSrttMultiplier(quic_trial_params); if (load_server_info_timeout_srtt_multiplier != 0) { @@ -930,6 +932,12 @@ void OnNetworkChanged( return 0; } +bool IOSChromeIOThread::ShouldQuicDelayTcpRace( + const VariationParameters& quic_trial_params) { + return base::LowerCaseEqualsASCII( + GetVariationParam(quic_trial_params, "delay_tcp_race"), "true"); +} + bool IOSChromeIOThread::ShouldQuicCloseSessionsOnIpChange( const VariationParameters& quic_trial_params) { return base::LowerCaseEqualsASCII( diff --git a/net/http/http_network_session.cc b/net/http/http_network_session.cc index a452f1e5a26dd7..cb6ec489a4b880 100644 --- a/net/http/http_network_session.cc +++ b/net/http/http_network_session.cc @@ -113,6 +113,7 @@ HttpNetworkSession::Params::Params() quic_max_number_of_lossy_connections(0), quic_packet_loss_threshold(1.0f), quic_socket_receive_buffer_size(kQuicSocketReceiveBufferSize), + quic_delay_tcp_race(false), quic_max_server_configs_stored_in_properties(0u), quic_clock(NULL), quic_random(NULL), @@ -178,6 +179,7 @@ HttpNetworkSession::HttpNetworkSession(const Params& params) params.quic_threshold_public_resets_post_handshake, params.quic_threshold_timeouts_streams_open, params.quic_socket_receive_buffer_size, + params.quic_delay_tcp_race, params.quic_max_server_configs_stored_in_properties, params.quic_close_sessions_on_ip_change, params.disable_quic_on_timeout_with_open_streams, @@ -331,7 +333,7 @@ std::unique_ptr HttpNetworkSession::QuicInfoToValue() const { dict->SetInteger("max_number_of_lossy_connections", params_.quic_max_number_of_lossy_connections); dict->SetDouble("packet_loss_threshold", params_.quic_packet_loss_threshold); - dict->SetBoolean("delay_tcp_race", true); + dict->SetBoolean("delay_tcp_race", params_.quic_delay_tcp_race); dict->SetInteger("max_server_configs_stored_in_properties", params_.quic_max_server_configs_stored_in_properties); dict->SetInteger("idle_connection_timeout_seconds", diff --git a/net/http/http_network_session.h b/net/http/http_network_session.h index d122775116bacf..41e172bedb7c8b 100644 --- a/net/http/http_network_session.h +++ b/net/http/http_network_session.h @@ -138,6 +138,9 @@ class NET_EXPORT HttpNetworkSession float quic_packet_loss_threshold; // Size in bytes of the QUIC DUP socket receive buffer. int quic_socket_receive_buffer_size; + // Delay starting a TCP connection when QUIC believes it can speak + // 0-RTT to a server. + bool quic_delay_tcp_race; // Maximum number of server configs that are to be stored in // HttpServerProperties, instead of the disk cache. size_t quic_max_server_configs_stored_in_properties; diff --git a/net/http/http_stream_factory_impl_unittest.cc b/net/http/http_stream_factory_impl_unittest.cc index 348a94e0264866..b50fcaf894cd88 100644 --- a/net/http/http_stream_factory_impl_unittest.cc +++ b/net/http/http_stream_factory_impl_unittest.cc @@ -1641,9 +1641,9 @@ TEST_P(HttpStreamFactoryBidirectionalQuicTest, EXPECT_EQ(OK, stream_impl->ReadData(buffer.get(), 1)); EXPECT_EQ(kProtoQUIC1SPDY3, stream_impl->GetProtocol()); EXPECT_EQ("200", delegate.response_headers().find(":status")->second); - EXPECT_EQ(0, GetSocketPoolGroupCount(session()->GetTransportSocketPool( + EXPECT_EQ(1, GetSocketPoolGroupCount(session()->GetTransportSocketPool( HttpNetworkSession::NORMAL_SOCKET_POOL))); - EXPECT_EQ(0, GetSocketPoolGroupCount(session()->GetSSLSocketPool( + EXPECT_EQ(1, GetSocketPoolGroupCount(session()->GetSSLSocketPool( HttpNetworkSession::NORMAL_SOCKET_POOL))); EXPECT_EQ(0, GetSocketPoolGroupCount(session()->GetTransportSocketPool( HttpNetworkSession::WEBSOCKET_SOCKET_POOL))); diff --git a/net/quic/quic_network_transaction_unittest.cc b/net/quic/quic_network_transaction_unittest.cc index ed7c5f458e1f30..19d528d65b1eed 100644 --- a/net/quic/quic_network_transaction_unittest.cc +++ b/net/quic/quic_network_transaction_unittest.cc @@ -706,6 +706,10 @@ TEST_P(QuicNetworkTransactionTest, ForceQuic) { mock_quic_data.AddSocketDataToFactory(&socket_factory_); + // The non-alternate protocol job needs to hang in order to guarantee that + // the alternate-protocol job will "win". + AddHangingNonAlternateProtocolSocketData(); + params_.enable_alternative_service_with_different_host = false; CreateSession(); @@ -827,6 +831,7 @@ TEST_P(QuicNetworkTransactionTest, QuicProxyWithCert) { crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details2); request_.url = GURL("http://" + origin_host); + AddHangingNonAlternateProtocolSocketData(); CreateSession(); AddQuicAlternateProtocolMapping(MockCryptoClientStream::CONFIRM_HANDSHAKE); SendRequestAndExpectQuicResponseFromProxyOnPort("hello!", 70); @@ -868,6 +873,7 @@ TEST_P(QuicNetworkTransactionTest, AlternativeServicesDifferentHost) { request_.url = GURL("https://" + origin.host()); AddQuicRemoteAlternativeServiceMapping( MockCryptoClientStream::CONFIRM_HANDSHAKE, alternative); + AddHangingNonAlternateProtocolSocketData(); CreateSession(); SendRequestAndExpectQuicResponse("hello!"); @@ -951,6 +957,7 @@ TEST_P(QuicNetworkTransactionTest, UseAlternativeServiceForQuic) { mock_quic_data.AddSocketDataToFactory(&socket_factory_); + AddHangingNonAlternateProtocolSocketData(); CreateSession(); SendRequestAndExpectHttpResponse("hello world"); @@ -985,6 +992,7 @@ TEST_P(QuicNetworkTransactionTest, mock_quic_data.AddSocketDataToFactory(&socket_factory_); + AddHangingNonAlternateProtocolSocketData(); CreateSession(); SendRequestAndExpectHttpResponse("hello world"); @@ -1085,6 +1093,7 @@ TEST_P(QuicNetworkTransactionTest, UseAlternativeServiceQuicSupportedVersion) { mock_quic_data.AddSocketDataToFactory(&socket_factory_); + AddHangingNonAlternateProtocolSocketData(); CreateSession(); SendRequestAndExpectHttpResponse("hello world"); @@ -1230,6 +1239,7 @@ TEST_P(QuicNetworkTransactionTest, UseExistingAlternativeServiceForQuic) { mock_quic_data.AddSocketDataToFactory(&socket_factory_); + AddHangingNonAlternateProtocolSocketData(); CreateSession(); SendRequestAndExpectHttpResponse("hello world"); @@ -1451,6 +1461,7 @@ TEST_P(QuicNetworkTransactionTest, mock_quic_data.AddSocketDataToFactory(&socket_factory_); + AddHangingNonAlternateProtocolSocketData(); CreateSession(); // Send two HTTP requests, responses set up alt-svc lists for the origins. @@ -1482,6 +1493,7 @@ TEST_P(QuicNetworkTransactionTest, AlternativeServiceDifferentPort) { socket_factory_.AddSocketDataProvider(&http_data); socket_factory_.AddSSLSocketDataProvider(&ssl_data_); + AddHangingNonAlternateProtocolSocketData(); CreateSession(); SendRequestAndExpectHttpResponse("hello world"); @@ -1522,6 +1534,7 @@ TEST_P(QuicNetworkTransactionTest, ConfirmAlternativeService) { mock_quic_data.AddSocketDataToFactory(&socket_factory_); + AddHangingNonAlternateProtocolSocketData(); CreateSession(); AlternativeService alternative_service(QUIC, @@ -1598,6 +1611,10 @@ TEST_P(QuicNetworkTransactionTest, UseAlternateProtocolForQuic) { mock_quic_data.AddSocketDataToFactory(&socket_factory_); + // The non-alternate protocol job needs to hang in order to guarantee that + // the alternate-protocol job will "win". + AddHangingNonAlternateProtocolSocketData(); + params_.parse_alternative_services = false; params_.parse_alternative_services = false; CreateSession(); @@ -1633,6 +1650,10 @@ TEST_P(QuicNetworkTransactionTest, UseAlternateProtocolWithProbabilityForQuic) { mock_quic_data.AddSocketDataToFactory(&socket_factory_); + // The non-alternate protocol job needs to hang in order to guarantee that + // the alternate-protocol job will "win". + AddHangingNonAlternateProtocolSocketData(); + params_.parse_alternative_services = false; params_.parse_alternative_services = false; CreateSession(); @@ -1654,6 +1675,10 @@ TEST_P(QuicNetworkTransactionTest, AlternateProtocolDifferentPort) { socket_factory_.AddSocketDataProvider(&http_data); socket_factory_.AddSSLSocketDataProvider(&ssl_data_); + // The non-alternate protocol job needs to hang in order to guarantee that + // the alternate-protocol job will "win". + AddHangingNonAlternateProtocolSocketData(); + params_.parse_alternative_services = false; CreateSession(); @@ -1695,6 +1720,10 @@ TEST_P(QuicNetworkTransactionTest, ConfirmAlternateProtocol) { mock_quic_data.AddSocketDataToFactory(&socket_factory_); + // The non-alternate protocol job needs to hang in order to guarantee that + // the alternate-protocol job will "win". + AddHangingNonAlternateProtocolSocketData(); + params_.parse_alternative_services = false; CreateSession(); @@ -1739,7 +1768,10 @@ TEST_P(QuicNetworkTransactionTest, UseAlternateProtocolForQuicForHttps) { mock_quic_data.AddSocketDataToFactory(&socket_factory_); + // The non-alternate protocol job needs to hang in order to guarantee that + // the alternate-protocol job will "win". AddHangingNonAlternateProtocolSocketData(); + CreateSession(); // TODO(rtenneti): Test QUIC over HTTPS, GetSSLInfo(). @@ -1813,6 +1845,10 @@ TEST_P(QuicNetworkTransactionTest, ZeroRTTWithHttpRace) { mock_quic_data.AddSocketDataToFactory(&socket_factory_); + // The non-alternate protocol job needs to hang in order to guarantee that + // the alternate-protocol job will "win". + AddHangingNonAlternateProtocolSocketData(); + CreateSession(); AddQuicAlternateProtocolMapping(MockCryptoClientStream::ZERO_RTT); SendRequestAndExpectQuicResponse("hello!"); @@ -1843,6 +1879,7 @@ TEST_P(QuicNetworkTransactionTest, ZeroRTTWithNoHttpRace) { host_resolver_.Resolve(info, DEFAULT_PRIORITY, &address, CompletionCallback(), nullptr, net_log_.bound()); + AddHangingNonAlternateProtocolSocketData(); CreateSession(); AddQuicAlternateProtocolMapping(MockCryptoClientStream::ZERO_RTT); SendRequestAndExpectQuicResponse("hello!"); @@ -1896,6 +1933,10 @@ TEST_P(QuicNetworkTransactionTest, ZeroRTTWithConfirmationRequired) { mock_quic_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING); // No more data to read mock_quic_data.AddSocketDataToFactory(&socket_factory_); + // The non-alternate protocol job needs to hang in order to guarantee that + // the alternate-protocol job will "win". + AddHangingNonAlternateProtocolSocketData(); + // In order for a new QUIC session to be established via alternate-protocol // without racing an HTTP connection, we need the host resolution to happen // synchronously. Of course, even though QUIC *could* perform a 0-RTT @@ -1909,10 +1950,6 @@ TEST_P(QuicNetworkTransactionTest, ZeroRTTWithConfirmationRequired) { host_resolver_.Resolve(info, DEFAULT_PRIORITY, &address, CompletionCallback(), nullptr, net_log_.bound()); - // The non-alternate protocol job needs to hang in order to guarantee that - // the alternate-protocol job will "win". - AddHangingNonAlternateProtocolSocketData(); - CreateSession(); session_->quic_stream_factory()->set_require_confirmation(true); AddQuicAlternateProtocolMapping(MockCryptoClientStream::ZERO_RTT); @@ -2111,6 +2148,7 @@ TEST_P(QuicNetworkTransactionTest, NoBrokenAlternateProtocolIfTcpFails) { socket_factory_.AddSocketDataProvider(&http_data); socket_factory_.AddSSLSocketDataProvider(&ssl_data_); + AddHangingNonAlternateProtocolSocketData(); CreateSession(); AddQuicAlternateProtocolMapping(MockCryptoClientStream::COLD_START); @@ -2132,6 +2170,8 @@ TEST_P(QuicNetworkTransactionTest, FailedZeroRttBrokenAlternateProtocol) { 0); socket_factory_.AddSocketDataProvider(&quic_data); + AddHangingNonAlternateProtocolSocketData(); + // Second Alternate-protocol job which will race with the TCP job. StaticSocketDataProvider quic_data2(quic_reads, arraysize(quic_reads), nullptr, 0); @@ -2148,7 +2188,6 @@ TEST_P(QuicNetworkTransactionTest, FailedZeroRttBrokenAlternateProtocol) { socket_factory_.AddSocketDataProvider(&http_data); socket_factory_.AddSSLSocketDataProvider(&ssl_data_); - AddHangingNonAlternateProtocolSocketData(); CreateSession(); AddQuicAlternateProtocolMapping(MockCryptoClientStream::ZERO_RTT); @@ -2180,6 +2219,7 @@ TEST_P(QuicNetworkTransactionTest, DISABLED_HangingZeroRttFallback) { 0); socket_factory_.AddSocketDataProvider(&http_data); + AddHangingNonAlternateProtocolSocketData(); CreateSession(); AddQuicAlternateProtocolMapping(MockCryptoClientStream::ZERO_RTT); @@ -2266,6 +2306,7 @@ TEST_P(QuicNetworkTransactionTest, SecureResourceOverSecureQuic) { mock_quic_data.AddSocketDataToFactory(&socket_factory_); request_.url = GURL("https://www.example.org:443"); + AddHangingNonAlternateProtocolSocketData(); CreateSession(); AddQuicAlternateProtocolMapping(MockCryptoClientStream::CONFIRM_HANDSHAKE); SendRequestAndExpectQuicResponse("hello!"); @@ -2283,6 +2324,10 @@ TEST_P(QuicNetworkTransactionTest, QuicUpload) { arraysize(writes)); socket_factory_.AddSocketDataProvider(&socket_data); + // The non-alternate protocol job needs to hang in order to guarantee that + // the alternate-protocol job will "win". + AddHangingNonAlternateProtocolSocketData(); + params_.enable_alternative_service_with_different_host = false; CreateSession(); request_.method = "POST"; diff --git a/net/quic/quic_stream_factory.cc b/net/quic/quic_stream_factory.cc index 1620ae2259f964..1a31440464be54 100644 --- a/net/quic/quic_stream_factory.cc +++ b/net/quic/quic_stream_factory.cc @@ -587,6 +587,7 @@ QuicStreamFactory::QuicStreamFactory( int threshold_public_resets_post_handshake, int threshold_timeouts_with_open_streams, int socket_receive_buffer_size, + bool delay_tcp_race, int max_server_configs_stored_in_properties, bool close_sessions_on_ip_change, bool disable_quic_on_timeout_with_open_streams, @@ -635,6 +636,7 @@ QuicStreamFactory::QuicStreamFactory( threshold_public_resets_post_handshake_( threshold_public_resets_post_handshake), socket_receive_buffer_size_(socket_receive_buffer_size), + delay_tcp_race_(delay_tcp_race), yield_after_packets_(kQuicYieldAfterPacketsRead), yield_after_duration_(QuicTime::Delta::FromMilliseconds( kQuicYieldAfterDurationMilliseconds)), @@ -731,7 +733,7 @@ bool QuicStreamFactory::ZeroRTTEnabledFor(const QuicServerId& quic_server_id) { base::TimeDelta QuicStreamFactory::GetTimeDelayForWaitingJob( const QuicServerId& server_id) { - if (require_confirmation_) + if (!delay_tcp_race_ || require_confirmation_) return base::TimeDelta(); int64_t srtt = 1.5 * GetServerNetworkStatsSmoothedRttInMicroseconds(server_id); diff --git a/net/quic/quic_stream_factory.h b/net/quic/quic_stream_factory.h index d1aab0136bda21..542687f5e5f420 100644 --- a/net/quic/quic_stream_factory.h +++ b/net/quic/quic_stream_factory.h @@ -178,6 +178,7 @@ class NET_EXPORT_PRIVATE QuicStreamFactory int threshold_timeouts_with_streams_open, int threshold_public_resets_post_handshake, int socket_receive_buffer_size, + bool delay_tcp_race, int max_server_configs_stored_in_properties, bool close_sessions_on_ip_change, bool disable_quic_on_timeout_with_open_streams, @@ -345,6 +346,8 @@ class NET_EXPORT_PRIVATE QuicStreamFactory int socket_receive_buffer_size() const { return socket_receive_buffer_size_; } + bool delay_tcp_race() const { return delay_tcp_race_; } + private: class Job; friend class test::QuicStreamFactoryPeer; @@ -535,6 +538,9 @@ class NET_EXPORT_PRIVATE QuicStreamFactory // Size of the UDP receive buffer. int socket_receive_buffer_size_; + // Set if we do want to delay TCP connection when it is racing with QUIC. + bool delay_tcp_race_; + // If more than |yield_after_packets_| packets have been read or more than // |yield_after_duration_| time has passed, then // QuicChromiumPacketReader::StartReading() yields by doing a PostTask(). diff --git a/net/quic/quic_stream_factory_test.cc b/net/quic/quic_stream_factory_test.cc index a3e647343aac83..e8f1017eedd87a 100644 --- a/net/quic/quic_stream_factory_test.cc +++ b/net/quic/quic_stream_factory_test.cc @@ -294,6 +294,7 @@ class QuicStreamFactoryTestBase { threshold_timeouts_with_open_streams_(2), threshold_public_resets_post_handshake_(2), receive_buffer_size_(0), + delay_tcp_race_(false), close_sessions_on_ip_change_(false), disable_quic_on_timeout_with_open_streams_(false), idle_connection_timeout_seconds_(kIdleConnectionTimeoutSeconds), @@ -325,7 +326,7 @@ class QuicStreamFactoryTestBase { prefer_aes_, max_number_of_lossy_connections_, packet_loss_threshold_, max_disabled_reasons_, threshold_timeouts_with_open_streams_, threshold_public_resets_post_handshake_, receive_buffer_size_, - /*max_server_configs_stored_in_properties*/ 0, + delay_tcp_race_, /*max_server_configs_stored_in_properties*/ 0, close_sessions_on_ip_change_, disable_quic_on_timeout_with_open_streams_, idle_connection_timeout_seconds_, migrate_sessions_on_network_change_, @@ -514,6 +515,7 @@ class QuicStreamFactoryTestBase { int threshold_timeouts_with_open_streams_; int threshold_public_resets_post_handshake_; int receive_buffer_size_; + bool delay_tcp_race_; bool close_sessions_on_ip_change_; bool disable_quic_on_timeout_with_open_streams_; int idle_connection_timeout_seconds_; @@ -3650,6 +3652,8 @@ TEST_P(QuicStreamFactoryTest, EnableDelayTcpRace) { Initialize(); ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails(); crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details); + bool delay_tcp_race = QuicStreamFactoryPeer::GetDelayTcpRace(factory_.get()); + QuicStreamFactoryPeer::SetDelayTcpRace(factory_.get(), false); MockRead reads[] = {MockRead(SYNCHRONOUS, ERR_IO_PENDING, 0)}; SequencedSocketData socket_data(reads, arraysize(reads), nullptr, 0); socket_factory_.AddSocketDataProvider(&socket_data); @@ -3672,6 +3676,14 @@ TEST_P(QuicStreamFactoryTest, EnableDelayTcpRace) { /*cert_verify_flags=*/0, url_, "POST", net_log_, callback_.callback())); + // If we don't delay TCP connection, then time delay should be 0. + EXPECT_FALSE(factory_->delay_tcp_race()); + EXPECT_EQ(base::TimeDelta(), request.GetTimeDelayForWaitingJob()); + + // Enable |delay_tcp_race_| param and verify delay is one RTT and that + // server supports QUIC. + QuicStreamFactoryPeer::SetDelayTcpRace(factory_.get(), true); + EXPECT_TRUE(factory_->delay_tcp_race()); EXPECT_EQ(base::TimeDelta::FromMicroseconds(15), request.GetTimeDelayForWaitingJob()); @@ -3685,6 +3697,7 @@ TEST_P(QuicStreamFactoryTest, EnableDelayTcpRace) { EXPECT_TRUE(stream.get()); EXPECT_TRUE(socket_data.AllReadDataConsumed()); EXPECT_TRUE(socket_data.AllWriteDataConsumed()); + QuicStreamFactoryPeer::SetDelayTcpRace(factory_.get(), delay_tcp_race); } TEST_P(QuicStreamFactoryTest, MaybeInitialize) { diff --git a/net/quic/test_tools/quic_stream_factory_peer.cc b/net/quic/test_tools/quic_stream_factory_peer.cc index 63ec75daa1b2e3..0d58f20a63b21a 100644 --- a/net/quic/test_tools/quic_stream_factory_peer.cc +++ b/net/quic/test_tools/quic_stream_factory_peer.cc @@ -73,6 +73,15 @@ bool QuicStreamFactoryPeer::IsQuicDisabled(QuicStreamFactory* factory, return factory->IsQuicDisabled(port); } +bool QuicStreamFactoryPeer::GetDelayTcpRace(QuicStreamFactory* factory) { + return factory->delay_tcp_race_; +} + +void QuicStreamFactoryPeer::SetDelayTcpRace(QuicStreamFactory* factory, + bool delay_tcp_race) { + factory->delay_tcp_race_ = delay_tcp_race; +} + void QuicStreamFactoryPeer::SetYieldAfterPackets(QuicStreamFactory* factory, int yield_after_packets) { factory->yield_after_packets_ = yield_after_packets; diff --git a/net/quic/test_tools/quic_stream_factory_peer.h b/net/quic/test_tools/quic_stream_factory_peer.h index a8ab16e11b550b..e0d85a34c64183 100644 --- a/net/quic/test_tools/quic_stream_factory_peer.h +++ b/net/quic/test_tools/quic_stream_factory_peer.h @@ -55,6 +55,10 @@ class QuicStreamFactoryPeer { static bool IsQuicDisabled(QuicStreamFactory* factory, uint16_t port); + static bool GetDelayTcpRace(QuicStreamFactory* factory); + + static void SetDelayTcpRace(QuicStreamFactory* factory, bool delay_tcp_race); + static void SetYieldAfterPackets(QuicStreamFactory* factory, int yield_after_packets); diff --git a/net/url_request/url_request_context_builder.cc b/net/url_request/url_request_context_builder.cc index 0d59128b00dea1..8e7fcbce245286 100644 --- a/net/url_request/url_request_context_builder.cc +++ b/net/url_request/url_request_context_builder.cc @@ -186,6 +186,7 @@ URLRequestContextBuilder::HttpNetworkSessionParams::HttpNetworkSessionParams() enable_alternative_service_with_different_host(false), enable_quic(false), quic_max_server_configs_stored_in_properties(0), + quic_delay_tcp_race(false), quic_max_number_of_lossy_connections(0), quic_prefer_aes(false), quic_packet_loss_threshold(1.0f), @@ -419,6 +420,8 @@ std::unique_ptr URLRequestContextBuilder::Build() { network_session_params.enable_quic = http_network_session_params_.enable_quic; network_session_params.quic_max_server_configs_stored_in_properties = http_network_session_params_.quic_max_server_configs_stored_in_properties; + network_session_params.quic_delay_tcp_race = + http_network_session_params_.quic_delay_tcp_race; network_session_params.quic_max_number_of_lossy_connections = http_network_session_params_.quic_max_number_of_lossy_connections; network_session_params.quic_packet_loss_threshold = diff --git a/net/url_request/url_request_context_builder.h b/net/url_request/url_request_context_builder.h index 20ff0f33171fec..2aa534f5865a68 100644 --- a/net/url_request/url_request_context_builder.h +++ b/net/url_request/url_request_context_builder.h @@ -96,6 +96,7 @@ class NET_EXPORT URLRequestContextBuilder { bool enable_quic; std::string quic_user_agent_id; int quic_max_server_configs_stored_in_properties; + bool quic_delay_tcp_race; int quic_max_number_of_lossy_connections; std::unordered_set quic_host_whitelist; bool quic_prefer_aes; @@ -231,6 +232,10 @@ class NET_EXPORT URLRequestContextBuilder { quic_max_server_configs_stored_in_properties; } + void set_quic_delay_tcp_race(bool quic_delay_tcp_race) { + http_network_session_params_.quic_delay_tcp_race = quic_delay_tcp_race; + } + void set_quic_max_number_of_lossy_connections( int quic_max_number_of_lossy_connections) { http_network_session_params_.quic_max_number_of_lossy_connections =