Actually close QUIC sessions when client IP address changes

BUG=544156

Review URL: https://codereview.chromium.org/1413533002

Cr-Commit-Position: refs/heads/master@{#355929}

8 files changed, 60 insertions, 4 deletions

diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc
index fcd5890..14f405c 100644
--- a/chrome/browser/io_thread.cc
+++ b/chrome/browser/io_thread.cc
@@ -1081,6 +1081,8 @@ void IOThread::InitializeNetworkSessionParamsFromGlobals(
   globals.quic_supported_versions.CopyToIfSet(
       &params->quic_supported_versions);
   params->quic_connection_options = globals.quic_connection_options;
+  globals.quic_close_sessions_on_ip_change.CopyToIfSet(
+      &params->quic_close_sessions_on_ip_change);
 
   globals.origin_to_force_quic_on.CopyToIfSet(
       &params->origin_to_force_quic_on);
@@ -1214,6 +1216,8 @@ void IOThread::ConfigureQuicGlobals(
         ShouldEnableQuicPortSelection(command_line));
     globals->quic_connection_options =
         GetQuicConnectionOptions(command_line, quic_trial_params);
+    globals->quic_close_sessions_on_ip_change.set(
+        ShouldQuicCloseSessionsOnIpChange(quic_trial_params));
   }
 
   size_t max_packet_length = GetQuicMaxPacketLength(command_line,
@@ -1448,6 +1452,13 @@ bool IOThread::ShouldQuicDelayTcpRace(
       GetVariationParam(quic_trial_params, "delay_tcp_race"), "true");
 }
 
+bool IOThread::ShouldQuicCloseSessionsOnIpChange(
+    const VariationParameters& quic_trial_params) {
+  return base::LowerCaseEqualsASCII(
+      GetVariationParam(quic_trial_params, "close_sessions_on_ip_change"),
+      "true");
+}
+
 size_t IOThread::GetQuicMaxPacketLength(
     const base::CommandLine& command_line,
     const VariationParameters& quic_trial_params) {
diff --git a/chrome/browser/io_thread.h b/chrome/browser/io_thread.h
index 6e747ac..33291d6 100644
--- a/chrome/browser/io_thread.h
+++ b/chrome/browser/io_thread.h
@@ -207,6 +207,7 @@ class IOThread : public content::BrowserThreadDelegate {
     Optional<std::string> quic_user_agent_id;
     Optional<net::QuicVersionVector> quic_supported_versions;
     Optional<net::HostPortPair> origin_to_force_quic_on;
+    Optional<bool> quic_close_sessions_on_ip_change;
     bool enable_user_alternate_protocol_ports;
     // NetErrorTabHelper uses |dns_probe_service| to send DNS probes when a
     // main frame load fails with a DNS error in order to provide more useful
@@ -408,6 +409,11 @@ class IOThread : public content::BrowserThreadDelegate {
   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(
+      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 6251d04..4430306 100644
--- a/chrome/browser/io_thread_unittest.cc
+++ b/chrome/browser/io_thread_unittest.cc
@@ -171,6 +171,7 @@ TEST_F(IOThreadTest, EnableQuicFromFieldTrialGroup) {
   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_FALSE(IOThread::ShouldEnableQuicForDataReductionProxy());
 }
 
@@ -258,6 +259,15 @@ TEST_F(IOThreadTest, PacketLengthFromCommandLine) {
   EXPECT_EQ(1450u, params.quic_max_packet_length);
 }
 
+TEST_F(IOThreadTest, QuicCloseSessionsOnIpChangeFromFieldTrialParams) {
+  field_trial_group_ = "Enabled";
+  field_trial_params_["close_sessions_on_ip_change"] = "true";
+  ConfigureQuicGlobals();
+  net::HttpNetworkSession::Params params;
+  InitializeNetworkSessionParams(&params);
+  EXPECT_TRUE(params.quic_close_sessions_on_ip_change);
+}
+
 TEST_F(IOThreadTest, PacketLengthFromFieldTrialParams) {
   field_trial_group_ = "Enabled";
   field_trial_params_["max_packet_length"] = "1450";
diff --git a/net/http/http_network_session.cc b/net/http/http_network_session.cc
index 687fc56..736d5a9 100644
--- a/net/http/http_network_session.cc
+++ b/net/http/http_network_session.cc
@@ -116,6 +116,7 @@ HttpNetworkSession::Params::Params()
       quic_max_recent_disabled_reasons(kQuicMaxRecentDisabledReasons),
       quic_threshold_public_resets_post_handshake(0),
       quic_threshold_timeouts_streams_open(0),
+      quic_close_sessions_on_ip_change(false),
       proxy_delegate(NULL) {
   quic_supported_versions.push_back(QUIC_VERSION_27);
 }
@@ -168,6 +169,7 @@ HttpNetworkSession::HttpNetworkSession(const Params& params)
           params.quic_socket_receive_buffer_size,
           params.quic_delay_tcp_race,
           params.quic_store_server_configs_in_properties,
+          params.quic_close_sessions_on_ip_change,
           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 c11afc3..0075154 100644
--- a/net/http/http_network_session.h
+++ b/net/http/http_network_session.h
@@ -166,8 +166,10 @@ class NET_EXPORT HttpNetworkSession
     int quic_max_recent_disabled_reasons;
     int quic_threshold_public_resets_post_handshake;
     int quic_threshold_timeouts_streams_open;
-    // Set of QUIC tags to send in the handshakes connection options.
+    // Set of QUIC tags to send in the handshake's connection options.
     QuicTagVector quic_connection_options;
+    // If true, all QUIC sessions are closed when any local IP address changes.
+    bool quic_close_sessions_on_ip_change;
     ProxyDelegate* proxy_delegate;
   };
 
diff --git a/net/quic/quic_stream_factory.cc b/net/quic/quic_stream_factory.cc
index 51fb16c..7563019 100644
--- a/net/quic/quic_stream_factory.cc
+++ b/net/quic/quic_stream_factory.cc
@@ -568,6 +568,7 @@ QuicStreamFactory::QuicStreamFactory(
     int socket_receive_buffer_size,
     bool delay_tcp_race,
     bool store_server_configs_in_properties,
+    bool close_sessions_on_ip_change,
     const QuicTagVector& connection_options)
     : require_confirmation_(true),
       host_resolver_(host_resolver),
@@ -608,6 +609,7 @@ QuicStreamFactory::QuicStreamFactory(
       yield_after_duration_(QuicTime::Delta::FromMilliseconds(
           kQuicYieldAfterDurationMilliseconds)),
       store_server_configs_in_properties_(store_server_configs_in_properties),
+      close_sessions_on_ip_change_(close_sessions_on_ip_change),
       port_seed_(random_generator_->RandUint64()),
       check_persisted_supports_quic_(true),
       has_initialized_data_(false),
@@ -647,6 +649,10 @@ QuicStreamFactory::QuicStreamFactory(
     quic_server_info_factory_.reset(
         new PropertiesBasedQuicServerInfoFactory(http_server_properties_));
   }
+
+  if (close_sessions_on_ip_change_) {
+    NetworkChangeNotifier::AddIPAddressObserver(this);
+  }
 }
 
 QuicStreamFactory::~QuicStreamFactory() {
@@ -660,6 +666,9 @@ QuicStreamFactory::~QuicStreamFactory() {
     STLDeleteElements(&(active_jobs_[server_id]));
     active_jobs_.erase(server_id);
   }
+  if (close_sessions_on_ip_change_) {
+    NetworkChangeNotifier::RemoveIPAddressObserver(this);
+  }
 }
 
 void QuicStreamFactory::set_require_confirmation(bool require_confirmation) {
diff --git a/net/quic/quic_stream_factory.h b/net/quic/quic_stream_factory.h
index 42d688a..7100126 100644
--- a/net/quic/quic_stream_factory.h
+++ b/net/quic/quic_stream_factory.h
@@ -137,6 +137,7 @@ class NET_EXPORT_PRIVATE QuicStreamFactory
       int socket_receive_buffer_size,
       bool delay_tcp_race,
       bool store_server_configs_in_properties,
+      bool close_sessions_on_ip_change,
       const QuicTagVector& connection_options);
   ~QuicStreamFactory() override;
 
@@ -437,6 +438,9 @@ class NET_EXPORT_PRIVATE QuicStreamFactory
   // Set if server configs are to be stored in HttpServerProperties.
   bool store_server_configs_in_properties_;
 
+  // Set if all sessions should be closed when any local IP address changes.
+  const bool close_sessions_on_ip_change_;
+
   // 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
diff --git a/net/quic/quic_stream_factory_test.cc b/net/quic/quic_stream_factory_test.cc
index e3a6f45..3c11cceb 100644
--- a/net/quic/quic_stream_factory_test.cc
+++ b/net/quic/quic_stream_factory_test.cc
@@ -247,7 +247,8 @@ class QuicStreamFactoryTest : public ::testing::TestWithParam<TestParams> {
         threshold_public_resets_post_handshake_(2),
         receive_buffer_size_(0),
         delay_tcp_race_(false),
-        store_server_configs_in_properties_(false) {
+        store_server_configs_in_properties_(false),
+        close_sessions_on_ip_change_(false) {
     clock_->AdvanceTime(QuicTime::Delta::FromSeconds(1));
   }
 
@@ -266,7 +267,8 @@ class QuicStreamFactoryTest : public ::testing::TestWithParam<TestParams> {
         max_number_of_lossy_connections_, packet_loss_threshold_,
         max_disabled_reasons_, threshold_timeouts_with_open_streams_,
         threshold_public_resets_post_handshake_, receive_buffer_size_,
-        delay_tcp_race_, store_server_configs_in_properties_, QuicTagVector()));
+        delay_tcp_race_, store_server_configs_in_properties_,
+        close_sessions_on_ip_change_, QuicTagVector()));
     factory_->set_require_confirmation(false);
     factory_->set_quic_server_info_factory(new MockQuicServerInfoFactory());
   }
@@ -364,6 +366,13 @@ class QuicStreamFactoryTest : public ::testing::TestWithParam<TestParams> {
     return verify_details;
   }
 
+  void NotifyIPAddressChanged() {
+    NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests();
+    // For thread safety, the NCN queues tasks to do the actual notifications,
+    // so we need to spin the message loop so the notification is delivered.
+    base::MessageLoop::current()->RunUntilIdle();
+  }
+
   MockHostResolver host_resolver_;
   DeterministicMockClientSocketFactory socket_factory_;
   MockCryptoClientStreamFactory crypto_client_stream_factory_;
@@ -398,6 +407,7 @@ class QuicStreamFactoryTest : public ::testing::TestWithParam<TestParams> {
   int receive_buffer_size_;
   bool delay_tcp_race_;
   bool store_server_configs_in_properties_;
+  bool close_sessions_on_ip_change_;
 };
 
 INSTANTIATE_TEST_CASE_P(Version,
@@ -1307,7 +1317,9 @@ TEST_P(QuicStreamFactoryTest, CloseAllSessions) {
 }
 
 TEST_P(QuicStreamFactoryTest, OnIPAddressChanged) {
+  close_sessions_on_ip_change_ = true;
   Initialize();
+
   MockRead reads[] = {
     MockRead(ASYNC, 0, 0)  // EOF
   };
@@ -1341,7 +1353,7 @@ TEST_P(QuicStreamFactoryTest, OnIPAddressChanged) {
                                          net_log_, CompletionCallback()));
 
   // Change the IP address and verify that stream saw the error.
-  factory_->OnIPAddressChanged();
+  NotifyIPAddressChanged();
   EXPECT_EQ(ERR_NETWORK_CHANGED,
             stream->ReadResponseHeaders(callback_.callback()));
   EXPECT_TRUE(factory_->require_confirmation());