Default enables option to close QUIC sessions on an IP address change,

when QUIC connection migration is not enabled.

Bug: 544156
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: I6440c4833e798b61d2d83dcef2f1bf24869719be
Reviewed-on: https://chromium-review.googlesource.com/655767Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Jana Iyengar <jri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503577}

components/cronet/url_request_context_config.cc

@@ -48,7 +48,6 @@ const char kQuicMaxServerConfigsStoredInProperties[] =
     "max_server_configs_stored_in_properties";
 const char kQuicIdleConnectionTimeoutSeconds[] =
     "idle_connection_timeout_seconds";
-const char kQuicCloseSessionsOnIpChange[] = "close_sessions_on_ip_change";
 const char kQuicMigrateSessionsOnNetworkChange[] =
     "migrate_sessions_on_network_change";
 const char kQuicUserAgentId[] = "user_agent_id";
@@ -247,13 +246,6 @@ void URLRequestContextConfig::ParseAndSetExperimentalOptions(
             quic_idle_connection_timeout_seconds;
       }
 
-      bool quic_close_sessions_on_ip_change = false;
-      if (quic_args->GetBoolean(kQuicCloseSessionsOnIpChange,
-                                &quic_close_sessions_on_ip_change)) {
-        session_params->quic_close_sessions_on_ip_change =
-            quic_close_sessions_on_ip_change;
-      }
-
       bool quic_migrate_sessions_on_network_change = false;
       if (quic_args->GetBoolean(kQuicMigrateSessionsOnNetworkChange,
                                 &quic_migrate_sessions_on_network_change)) {

components/cronet/url_request_context_config_unittest.cc

@@ -47,7 +47,6 @@ TEST(URLRequestContextConfigTest, TestExperimentalOptionParsing) {
       "{\"QUIC\":{\"max_server_configs_stored_in_properties\":2,"
       "\"user_agent_id\":\"Custom QUIC UAID\","
       "\"idle_connection_timeout_seconds\":300,"
-      "\"close_sessions_on_ip_change\":true,"
       "\"race_cert_verification\":true,"
       "\"connection_options\":\"TIME,TBBR,REJ\"},"
       "\"AsyncDNS\":{\"enable\":true},"
@@ -92,7 +91,6 @@ TEST(URLRequestContextConfigTest, TestExperimentalOptionParsing) {
   // Check idle_connection_timeout_seconds.
   EXPECT_EQ(300, params->quic_idle_connection_timeout_seconds);
 
-  EXPECT_TRUE(params->quic_close_sessions_on_ip_change);
   EXPECT_FALSE(params->quic_migrate_sessions_on_network_change);
 
   // Check race_cert_verification.
@@ -157,7 +155,6 @@ TEST(URLRequestContextConfigTest, SetQuicConnectionMigrationOptions) {
   const net::HttpNetworkSession::Params* params =
       context->GetNetworkSessionParams();
 
-  EXPECT_FALSE(params->quic_close_sessions_on_ip_change);
   EXPECT_TRUE(params->quic_migrate_sessions_on_network_change);
   EXPECT_TRUE(params->quic_migrate_sessions_early);
 }

components/network_session_configurator/browser/network_session_configurator.cc

@@ -167,13 +167,6 @@ net::QuicTagVector GetQuicClientConnectionOptions(
   return net::ParseQuicConnectionOptions(it->second);
 }
 
-bool ShouldQuicCloseSessionsOnIpChange(
-    const VariationParameters& quic_trial_params) {
-  return base::LowerCaseEqualsASCII(
-      GetVariationParam(quic_trial_params, "close_sessions_on_ip_change"),
-      "true");
-}
-
 int GetQuicIdleConnectionTimeoutSeconds(
     const VariationParameters& quic_trial_params) {
   int value;
@@ -273,8 +266,6 @@ void ConfigureQuicParams(base::StringPiece quic_trial_group,
         GetQuicConnectionOptions(quic_trial_params);
     params->quic_client_connection_options =
         GetQuicClientConnectionOptions(quic_trial_params);
-    params->quic_close_sessions_on_ip_change =
-        ShouldQuicCloseSessionsOnIpChange(quic_trial_params);
     int idle_connection_timeout_seconds =
         GetQuicIdleConnectionTimeoutSeconds(quic_trial_params);
     if (idle_connection_timeout_seconds != 0) {

components/network_session_configurator/browser/network_session_configurator_unittest.cc

@@ -91,7 +91,6 @@ TEST_F(NetworkSessionConfiguratorTest, EnableQuicFromFieldTrialGroup) {
   EXPECT_EQ(net::QuicTagVector(), params_.quic_connection_options);
   EXPECT_EQ(net::QuicTagVector(), params_.quic_client_connection_options);
   EXPECT_FALSE(params_.enable_server_push_cancellation);
-  EXPECT_FALSE(params_.quic_close_sessions_on_ip_change);
   EXPECT_EQ(net::kIdleConnectionTimeoutSeconds,
             params_.quic_idle_connection_timeout_seconds);
   EXPECT_EQ(net::kPingTimeoutSecs, params_.quic_reduced_ping_timeout_seconds);
@@ -150,18 +149,6 @@ TEST_F(NetworkSessionConfiguratorTest, RetryWithoutAltSvcOnQuicErrors) {
   EXPECT_TRUE(params_.retry_without_alt_svc_on_quic_errors);
 }
 
-TEST_F(NetworkSessionConfiguratorTest,
-       QuicCloseSessionsOnIpChangeFromFieldTrialParams) {
-  std::map<std::string, std::string> field_trial_params;
-  field_trial_params["close_sessions_on_ip_change"] = "true";
-  variations::AssociateVariationParams("QUIC", "Enabled", field_trial_params);
-  base::FieldTrialList::CreateFieldTrial("QUIC", "Enabled");
-
-  ParseFieldTrials();
-
-  EXPECT_TRUE(params_.quic_close_sessions_on_ip_change);
-}
-
 TEST_F(NetworkSessionConfiguratorTest,
        QuicIdleConnectionTimeoutSecondsFieldTrialParams) {
   std::map<std::string, std::string> field_trial_params;

net/http/http_network_session.cc

@@ -113,7 +113,6 @@ HttpNetworkSession::Params::Params()
       quic_max_server_configs_stored_in_properties(0u),
       mark_quic_broken_when_network_blackholes(false),
       retry_without_alt_svc_on_quic_errors(false),
-      quic_close_sessions_on_ip_change(false),
       quic_idle_connection_timeout_seconds(kIdleConnectionTimeoutSeconds),
       quic_reduced_ping_timeout_seconds(kPingTimeoutSecs),
       quic_migrate_sessions_on_network_change(false),
@@ -187,7 +186,6 @@ HttpNetworkSession::HttpNetworkSession(const Params& params,
           params.quic_max_packet_length,
           params.quic_user_agent_id,
           params.quic_max_server_configs_stored_in_properties > 0,
-          params.quic_close_sessions_on_ip_change,
           params.mark_quic_broken_when_network_blackholes,
           params.quic_idle_connection_timeout_seconds,
           params.quic_reduced_ping_timeout_seconds,
@@ -335,8 +333,6 @@ std::unique_ptr<base::Value> HttpNetworkSession::QuicInfoToValue() const {
                    params_.quic_race_cert_verification);
   dict->SetBoolean("disable_bidirectional_streams",
                    params_.quic_disable_bidirectional_streams);
-  dict->SetBoolean("close_sessions_on_ip_change",
-                   params_.quic_close_sessions_on_ip_change);
   dict->SetBoolean("migrate_sessions_on_network_change",
                    params_.quic_migrate_sessions_on_network_change);
   dict->SetBoolean("migrate_sessions_early",

net/http/http_network_session.h

@@ -144,8 +144,6 @@ class NET_EXPORT HttpNetworkSession : public base::MemoryCoordinatorClient {
     // Retry requests which fail with QUIC_PROTOCOL_ERROR, and mark QUIC
     // broken if the retry succeeds.
     bool retry_without_alt_svc_on_quic_errors;
-    // If true, all QUIC sessions are closed when any local IP address changes.
-    bool quic_close_sessions_on_ip_change;
     // Specifies QUIC idle connection state lifetime.
     int quic_idle_connection_timeout_seconds;
     // Specifies the reduced ping timeout subsequent connections should use when

net/quic/chromium/quic_stream_factory.cc

@@ -682,7 +682,6 @@ QuicStreamFactory::QuicStreamFactory(
     size_t max_packet_length,
     const std::string& user_agent_id,
     bool store_server_configs_in_properties,
-    bool close_sessions_on_ip_change,
     bool mark_quic_broken_when_network_blackholes,
     int idle_connection_timeout_seconds,
     int reduced_ping_timeout_seconds,
@@ -725,10 +724,7 @@ QuicStreamFactory::QuicStreamFactory(
       yield_after_packets_(kQuicYieldAfterPacketsRead),
       yield_after_duration_(QuicTime::Delta::FromMilliseconds(
           kQuicYieldAfterDurationMilliseconds)),
-      close_sessions_on_ip_change_(close_sessions_on_ip_change),
-      migrate_sessions_on_network_change_(
-          migrate_sessions_on_network_change &&
-          NetworkChangeNotifier::AreNetworkHandlesSupported()),
+      migrate_sessions_on_network_change_(migrate_sessions_on_network_change),
       migrate_sessions_early_(migrate_sessions_early &&
                               migrate_sessions_on_network_change_),
       allow_server_migration_(allow_server_migration),
@@ -767,15 +763,10 @@ QuicStreamFactory::QuicStreamFactory(
   // migrate_sessions_on_network_change is set to true.
   if (migrate_sessions_early)
     DCHECK(migrate_sessions_on_network_change);
-  // close_sessions_on_ip_change and migrate_sessions_on_network_change should
-  // never be simultaneously set to true.
-  DCHECK(!(close_sessions_on_ip_change && migrate_sessions_on_network_change));
 
-  if (migrate_sessions_on_network_change_) {
+  NetworkChangeNotifier::AddIPAddressObserver(this);
+  if (NetworkChangeNotifier::AreNetworkHandlesSupported())
     NetworkChangeNotifier::AddNetworkObserver(this);
-  } else if (close_sessions_on_ip_change_) {
-    NetworkChangeNotifier::AddIPAddressObserver(this);
-  }
 }
 
 QuicStreamFactory::~QuicStreamFactory() {
@@ -789,11 +780,9 @@ QuicStreamFactory::~QuicStreamFactory() {
     active_cert_verifier_jobs_.erase(active_cert_verifier_jobs_.begin());
   if (ssl_config_service_.get())
     ssl_config_service_->RemoveObserver(this);
-  if (migrate_sessions_on_network_change_) {
+  NetworkChangeNotifier::RemoveIPAddressObserver(this);
+  if (NetworkChangeNotifier::AreNetworkHandlesSupported())
     NetworkChangeNotifier::RemoveNetworkObserver(this);
-  } else if (close_sessions_on_ip_change_) {
-    NetworkChangeNotifier::RemoveIPAddressObserver(this);
-  }
 }
 
 void QuicStreamFactory::set_require_confirmation(bool require_confirmation) {
@@ -1167,11 +1156,16 @@ void QuicStreamFactory::ClearCachedStatesInCryptoConfig(
 }
 
 void QuicStreamFactory::OnIPAddressChanged() {
+  // Do nothing if connection migration is in use.
+  if (migrate_sessions_on_network_change_)
+    return;
   CloseAllSessions(ERR_NETWORK_CHANGED, QUIC_IP_ADDRESS_CHANGED);
   set_require_confirmation(true);
 }
 
 void QuicStreamFactory::OnNetworkConnected(NetworkHandle network) {
+  if (!migrate_sessions_on_network_change_)
+    return;
   ScopedConnectionMigrationEventLog scoped_event_log(net_log_,
                                                      "OnNetworkConnected");
   QuicStreamFactory::SessionIdMap::iterator it = all_sessions_.begin();
@@ -1184,15 +1178,19 @@ void QuicStreamFactory::OnNetworkConnected(NetworkHandle network) {
 }
 
 void QuicStreamFactory::OnNetworkMadeDefault(NetworkHandle network) {
+  if (!migrate_sessions_on_network_change_)
+    return;
+  DCHECK_NE(NetworkChangeNotifier::kInvalidNetworkHandle, network);
   ScopedConnectionMigrationEventLog scoped_event_log(net_log_,
                                                      "OnNetworkMadeDefault");
-  DCHECK_NE(NetworkChangeNotifier::kInvalidNetworkHandle, network);
   MaybeMigrateOrCloseSessions(network, /*close_if_cannot_migrate=*/false,
                               scoped_event_log.net_log());
   set_require_confirmation(true);
 }
 
 void QuicStreamFactory::OnNetworkDisconnected(NetworkHandle network) {
+  if (!migrate_sessions_on_network_change_)
+    return;
   ScopedConnectionMigrationEventLog scoped_event_log(net_log_,
                                                      "OnNetworkDisconnected");
   NetworkHandle new_network = FindAlternateNetwork(network);

net/quic/chromium/quic_stream_factory.h

@@ -207,7 +207,6 @@ class NET_EXPORT_PRIVATE QuicStreamFactory
       size_t max_packet_length,
       const std::string& user_agent_id,
       bool store_server_configs_in_properties,
-      bool close_sessions_on_ip_change,
       bool mark_quic_broken_when_network_blackholes,
       int idle_connection_timeout_seconds,
       int reduced_ping_timeout_seconds,
@@ -329,8 +328,8 @@ class NET_EXPORT_PRIVATE QuicStreamFactory
 
   // NetworkChangeNotifier::IPAddressObserver methods:
 
-  // Until the servers support roaming, close all connections when the local
-  // IP address changes.
+  // Called when local IP address changes. Must not be called if
+  // |migrate_sessions_on_network_change_| is true.
   void OnIPAddressChanged() override;
 
   // NetworkChangeNotifier::NetworkObserver methods:
@@ -531,9 +530,6 @@ class NET_EXPORT_PRIVATE QuicStreamFactory
   int yield_after_packets_;
   QuicTime::Delta yield_after_duration_;
 
-  // Set if all sessions should be closed when any local IP address changes.
-  const bool close_sessions_on_ip_change_;
-
   // Set if migration should be attempted on active sessions when primary
   // interface changes.
   const bool migrate_sessions_on_network_change_;

net/quic/chromium/quic_stream_factory_test.cc

@@ -194,7 +194,6 @@ class QuicStreamFactoryTestBase {
         url4_(kServer4Url),
         privacy_mode_(PRIVACY_MODE_DISABLED),
         store_server_configs_in_properties_(false),
-        close_sessions_on_ip_change_(false),
         idle_connection_timeout_seconds_(kIdleConnectionTimeoutSeconds),
         reduced_ping_timeout_seconds_(kPingTimeoutSecs),
         migrate_sessions_on_network_change_(false),
@@ -215,7 +214,6 @@ class QuicStreamFactoryTestBase {
         /*SocketPerformanceWatcherFactory*/ nullptr,
         &crypto_client_stream_factory_, &random_generator_, &clock_,
         kDefaultMaxPacketSize, string(), store_server_configs_in_properties_,
-        close_sessions_on_ip_change_,
         /*mark_quic_broken_when_network_blackholes*/ false,
         idle_connection_timeout_seconds_, reduced_ping_timeout_seconds_,
         migrate_sessions_on_network_change_, migrate_sessions_early_,
@@ -314,17 +312,12 @@ class QuicStreamFactoryTestBase {
     return client_maker_.MakeConnectionClosePacket(num);
   }
 
-  std::unique_ptr<QuicEncryptedPacket> ConstructClientRstPacket() {
-    QuicStreamId stream_id = GetNthClientInitiatedStreamId(0);
-    return client_maker_.MakeRstPacket(1, true, stream_id,
-                                       QUIC_RST_ACKNOWLEDGEMENT);
-  }
-
   std::unique_ptr<QuicEncryptedPacket> ConstructClientRstPacket(
-      QuicPacketNumber packet_number) {
+      QuicPacketNumber packet_number,
+      QuicRstStreamErrorCode error_code) {
     QuicStreamId stream_id = GetNthClientInitiatedStreamId(0);
     return client_maker_.MakeRstPacket(packet_number, true, stream_id,
-                                       QUIC_RST_ACKNOWLEDGEMENT);
+                                       error_code);
   }
 
   static ProofVerifyDetailsChromium DefaultProofVerifyDetails() {
@@ -726,7 +719,6 @@ class QuicStreamFactoryTestBase {
 
   // Variables to configure QuicStreamFactory.
   bool store_server_configs_in_properties_;
-  bool close_sessions_on_ip_change_;
   int idle_connection_timeout_seconds_;
   int reduced_ping_timeout_seconds_;
   bool migrate_sessions_on_network_change_;
@@ -1597,7 +1589,7 @@ TEST_P(QuicStreamFactoryTest, CloseAllSessions) {
   MockQuicData socket_data;
   socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
   socket_data.AddWrite(ConstructInitialSettingsPacket());
-  socket_data.AddWrite(ConstructClientRstPacket(2));
+  socket_data.AddWrite(ConstructClientRstPacket(2, QUIC_RST_ACKNOWLEDGEMENT));
   socket_data.AddSocketDataToFactory(&socket_factory_);
 
   MockQuicData socket_data2;
@@ -1766,7 +1758,6 @@ TEST_P(QuicStreamFactoryTest, WriteErrorInCryptoConnectWithSyncHostResolution) {
 }
 
 TEST_P(QuicStreamFactoryTest, OnIPAddressChanged) {
-  close_sessions_on_ip_change_ = true;
   Initialize();
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
@@ -1775,7 +1766,7 @@ TEST_P(QuicStreamFactoryTest, OnIPAddressChanged) {
   MockQuicData socket_data;
   socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
   socket_data.AddWrite(ConstructInitialSettingsPacket());
-  socket_data.AddWrite(ConstructClientRstPacket(2));
+  socket_data.AddWrite(ConstructClientRstPacket(2, QUIC_RST_ACKNOWLEDGEMENT));
   socket_data.AddSocketDataToFactory(&socket_factory_);
 
   MockQuicData socket_data2;
@@ -1823,6 +1814,51 @@ TEST_P(QuicStreamFactoryTest, OnIPAddressChanged) {
   EXPECT_TRUE(socket_data2.AllWriteDataConsumed());
 }
 
+TEST_P(QuicStreamFactoryTest, OnIPAddressChangedWithConnectionMigration) {
+  InitializeConnectionMigrationTest(
+      {kDefaultNetworkForTests, kNewNetworkForTests});
+  ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
+  crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
+  crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
+
+  MockQuicData socket_data;
+  socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
+  socket_data.AddWrite(ConstructInitialSettingsPacket());
+  socket_data.AddWrite(ConstructClientRstPacket(2, QUIC_STREAM_CANCELLED));
+  socket_data.AddSocketDataToFactory(&socket_factory_);
+
+  QuicStreamRequest request(factory_.get());
+  EXPECT_EQ(ERR_IO_PENDING,
+            request.Request(host_port_pair_, version_, privacy_mode_,
+                            /*cert_verify_flags=*/0, url_, "GET", net_log_,
+                            &net_error_details_, callback_.callback()));
+
+  EXPECT_THAT(callback_.WaitForResult(), IsOk());
+  std::unique_ptr<HttpStream> stream = request.CreateStream();
+  HttpRequestInfo request_info;
+  EXPECT_EQ(OK, stream->InitializeStream(&request_info, DEFAULT_PRIORITY,
+                                         net_log_, CompletionCallback()));
+
+  IPAddress last_address;
+  EXPECT_TRUE(http_server_properties_.GetSupportsQuic(&last_address));
+
+  // Change the IP address and verify that the connection is unaffected.
+  NotifyIPAddressChanged();
+  EXPECT_FALSE(factory_->require_confirmation());
+  EXPECT_TRUE(http_server_properties_.GetSupportsQuic(&last_address));
+
+  // Attempting a new request to the same origin uses the same connection.
+  QuicStreamRequest request2(factory_.get());
+  EXPECT_EQ(OK, request2.Request(host_port_pair_, version_, privacy_mode_,
+                                 /*cert_verify_flags=*/0, url_, "GET", net_log_,
+                                 &net_error_details_, callback_.callback()));
+  stream = request2.CreateStream();
+
+  stream.reset();
+  EXPECT_TRUE(socket_data.AllReadDataConsumed());
+  EXPECT_TRUE(socket_data.AllWriteDataConsumed());
+}
+
 TEST_P(QuicStreamFactoryTest, OnNetworkMadeDefaultWithSynchronousWriteBefore) {
   OnNetworkMadeDefault(/*async_write_before=*/false);
 }
@@ -4189,7 +4225,7 @@ TEST_P(QuicStreamFactoryTest, OnSSLConfigChanged) {
   MockQuicData socket_data;
   socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
   socket_data.AddWrite(ConstructInitialSettingsPacket());
-  socket_data.AddWrite(ConstructClientRstPacket(2));
+  socket_data.AddWrite(ConstructClientRstPacket(2, QUIC_RST_ACKNOWLEDGEMENT));
   socket_data.AddSocketDataToFactory(&socket_factory_);
 
   MockQuicData socket_data2;
@@ -4242,7 +4278,7 @@ TEST_P(QuicStreamFactoryTest, OnCertDBChanged) {
   MockQuicData socket_data;
   socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
   socket_data.AddWrite(ConstructInitialSettingsPacket());
-  socket_data.AddWrite(ConstructClientRstPacket(2));
+  socket_data.AddWrite(ConstructClientRstPacket(2, QUIC_RST_ACKNOWLEDGEMENT));
   socket_data.AddSocketDataToFactory(&socket_factory_);
 
   MockQuicData socket_data2;