Migrates QUIC sessions to a new network when old network is (about to be) disconnected.

net/quic/quic_stream_factory.cc

Index: net/quic/quic_stream_factory.cc
diff --git a/net/quic/quic_stream_factory.cc b/net/quic/quic_stream_factory.cc
index f44675595691c90063e47c5e72a166e20a05bbd2..a81362670d681d78204cec56c6ab317fdc0829d4 100644
--- a/net/quic/quic_stream_factory.cc
+++ b/net/quic/quic_stream_factory.cc
@@ -569,6 +569,7 @@ QuicStreamFactory::QuicStreamFactory(
     bool delay_tcp_race,
     bool store_server_configs_in_properties,
     bool close_sessions_on_ip_change,
+    bool migrate_sessions_on_network_change,
     const QuicTagVector& connection_options)
     : require_confirmation_(true),
       host_resolver_(host_resolver),
@@ -613,6 +614,7 @@ QuicStreamFactory::QuicStreamFactory(
           kQuicYieldAfterDurationMilliseconds)),
       store_server_configs_in_properties_(store_server_configs_in_properties),
       close_sessions_on_ip_change_(close_sessions_on_ip_change),
+      migrate_sessions_on_network_change_(migrate_sessions_on_network_change),
       port_seed_(random_generator_->RandUint64()),
       check_persisted_supports_quic_(true),
       has_initialized_data_(false),
@@ -651,9 +653,22 @@ QuicStreamFactory::QuicStreamFactory(
         new PropertiesBasedQuicServerInfoFactory(http_server_properties_));
   }
 
+#if defined(OS_ANDROID)
+  // Migration only implemented for Android versions >= L.
+  // TODO (jri): Maybe move this to NetworkChangeNotifier, as a
+  // IsSupported (or some such) method.
+  if (migrate_sessions_on_network_change_ &&
+      base::android::BuildInfo::GetInstance()->sdk_int() >=
+          base::android::SDK_VERSION_LOLLIPOP) {
+    NetworkChangeNotifier::AddNetworkObserver(this);
+  } else if (close_sessions_on_ip_change_) {
+    NetworkChangeNotifier::AddIPAddressObserver(this);
+  }
+#else
   if (close_sessions_on_ip_change_) {
     NetworkChangeNotifier::AddIPAddressObserver(this);
   }
+#endif
 }
 
 QuicStreamFactory::~QuicStreamFactory() {
@@ -1131,11 +1146,107 @@ void QuicStreamFactory::ClearCachedStatesInCryptoConfig() {
   crypto_config_.ClearCachedStates();
 }
 
+// We still need OnIPAddressChanged, but not for Android >= L.
+// pauljensen: How do we do this?
 void QuicStreamFactory::OnIPAddressChanged() {
   CloseAllSessions(ERR_NETWORK_CHANGED);
   set_require_confirmation(true);
 }
 
+void QuicStreamFactory::OnNetworkConnected(
+    NetworkChangeNotifier::NetworkHandle network) {
+  // Nothing to do.

[Ryan Hamilton, 2015/11/18 22:23:36]

I'm not sure that this comment adds any value.

+}
+
+void QuicStreamFactory::OnNetworkMadeDefault(
+    NetworkChangeNotifier::NetworkHandle network) {
+  // Nothing to do.
+}
+
+void QuicStreamFactory::OnNetworkDisconnected(
+    NetworkChangeNotifier::NetworkHandle network) {
+  MigrateOrCloseSessions(network);
+}
+
+void QuicStreamFactory::OnNetworkSoonToDisconnect(
+    NetworkChangeNotifier::NetworkHandle network) {
+  MigrateOrCloseSessions(network);
+}
+
+void QuicStreamFactory::MigrateOrCloseSessions(
+    NetworkChangeNotifier::NetworkHandle network) {
+  // TODO(jri): Need a "verified" flag on each network that gets set when
+  // any data is received on the network.
+  set_require_confirmation(true);
+
+  // pauljensen: do we need the following "== network" check?
+  NetworkChangeNotifier::NetworkList network_list;
+  NetworkChangeNotifier::GetConnectedNetworks(&network_list);
+  if (network_list.empty() ||
+      (network_list.size() == 1 && network_list[0] == network)) {
+    CloseAllSessions(ERR_NETWORK_CHANGED);
+    return;
+  }
+
+  // Find a new network that old sockets can be migrated to.
+  // pauljensen: do we need the following "== network" check?
+  NetworkChangeNotifier::NetworkHandle new_network =
+      NetworkChangeNotifier::kInvalidNetworkHandle;
+  for (NetworkChangeNotifier::NetworkHandle n : network_list) {
+    if (n == network) {
+      continue;
+    }
+    new_network = n;
+  }
+
+  DCHECK_NE(NetworkChangeNotifier::kInvalidNetworkHandle, new_network);
+  DCHECK_NE(network, new_network);

[Ryan Hamilton, 2015/11/18 22:23:36]

It looks like these DCHECKs are needed because of the code on 1182-1189. How
about doing:

   // Find a new network that old sockets can be migrated to.
   // pauljensen: do we need the following "== network" check?
   NetworkChangeNotifier::NetworkHandle new_network =
       NetworkChangeNotifier::kInvalidNetworkHandle;
   for (NetworkChangeNotifier::NetworkHandle n : network_list) {
    if (n == network) {
      continue;
     }
     new_network = n;
   }

   if (new_network == NetworkChangeNotifier::kInvalidNetworkHandle) {
     CloseAllSessions(ERR_NETWORK_CHANGED);
     return;
   }

This handles the empty case and the single element that is the current network
case.

[Jana, 2015/11/21 02:21:22]

Good suggestion -- there was definitely redundant code there. Done.

+
+  for (auto& it : all_sessions_) {
+    QuicChromiumClientSession* session = it.first;
+    QuicServerId server_id = it.second;
+
+    if (session->GetNumOpenStreams() == 0) {
+      // If idle session, close session
+      active_sessions_.erase(server_id);
+      // TODO(jri): Add new error for QUIC connection migration.
+      session->CloseSessionOnError(ERR_NETWORK_CHANGED, QUIC_INTERNAL_ERROR);

[Ryan Hamilton, 2015/11/18 22:23:36]

CloseSessionOnError causes the session to be removed from active_sessions_ so I
don't think you need to explicitly erase it here. If you were paranoid about
this, you could do

  DCHECK(!ContainsKey(active_sessions_, server_id));

After calling CloseSessionOnError

[Jana, 2015/11/21 02:21:22]

Ah, ok, I see that now. Removed.

+      //    } else if (session->current_network() == new_network) {
+      // @@@ TODO (jri): store network and record in session when
+      // BindToNetwork() is called.
+      // If session is already using |network|, move on.
+      //      continue;

[Ryan Hamilton, 2015/11/18 22:23:36]

What's up with this code?

[Jana, 2015/11/21 02:21:22]

Cleaned it up some and added a method in session to help... this is waiting for
Paul's CL to land.

+      continue;
+    }
+    // If session has active streams,
+    // (i) make the session GO_AWAY,
+    // (ii) create a new socket (which should be bound to the new interface),
+    // (iii) migrate the session to using the new socket.
+    OnSessionGoingAway(session);

[Ryan Hamilton, 2015/11/18 22:23:36]

I wonder if you want to send a GOAWAY as well?

[Jana, 2015/11/21 02:21:22]

Why? We won't sent any more requests to the server anyways, does the server
learn anything from the client sending it a GOAWAY?

+    QuicConnection* connection = session->connection();
+    scoped_ptr<DatagramClientSocket> socket =
+        CreateDatagramClientSocket(all_sessions_[session], session->net_log());

[Ryan Hamilton, 2015/11/18 22:23:36]

instead of all_sessions_[session] can you call session_-server_id()?

[Jana, 2015/11/21 02:21:22]

Ah! Didn't know that existed. Done.

+    int rv = ConfigureDatagramClientSocket(socket.get(),
+                                           connection->peer_address(), network);

[Ryan Hamilton, 2015/11/18 22:23:36]

It looks like both places this method is called is immediately after
CreateDatagramClientSocket(). How about folding the configuration into Create?

[Jana, 2015/11/21 02:21:22]

I had it together first (some earlier rev) but I needed an "out" param in
addition to the return value for either the scoped socket ptr or for rv. Since
it's unintuitive to have rv as an out param, and chrome style guide explicitly
calls out use of scoped_ptr*, I split this call up. 

+    if (rv != OK) {

[Ryan Hamilton, 2015/11/18 22:23:36]

nit: no need to store rv in a variable if it's not used after this:

if (Configure() != OK) { ...}

[Jana, 2015/11/21 02:21:22]

Done.

+      session->CloseSessionOnError(ERR_NETWORK_CHANGED, QUIC_INTERNAL_ERROR);
+      continue;
+    }
+    scoped_ptr<QuicPacketReader> new_reader(new QuicPacketReader(
+        socket.get(), clock_.get(), session, yield_after_packets_,
+        yield_after_duration_, session->net_log()));
+    DefaultPacketWriterFactory packet_writer_factory(socket.get());
+    scoped_ptr<QuicPacketWriter> new_writer(
+        packet_writer_factory.Create(connection));
+
+    if (!session->MigrateToSocket(socket.Pass(), new_reader.Pass(),
+                                  new_writer.Pass())) {
+      // TODO (jri): Log this as a different ERROR.
+      session->CloseSessionOnError(ERR_NETWORK_CHANGED, QUIC_INTERNAL_ERROR);
+    }
+  }
+}
+
 void QuicStreamFactory::OnSSLConfigChanged() {
   CloseAllSessions(ERR_CERT_DATABASE_CHANGED);
 }
@@ -1169,13 +1280,9 @@ bool QuicStreamFactory::HasActiveJob(const QuicServerId& key) const {
   return ContainsKey(active_jobs_, key);
 }
 
-int QuicStreamFactory::CreateSession(const QuicServerId& server_id,
-                                     int cert_verify_flags,
-                                     scoped_ptr<QuicServerInfo> server_info,
-                                     const AddressList& address_list,
-                                     base::TimeTicks dns_resolution_end_time,
-                                     const BoundNetLog& net_log,
-                                     QuicChromiumClientSession** session) {
+scoped_ptr<DatagramClientSocket> QuicStreamFactory::CreateDatagramClientSocket(
+    const QuicServerId& server_id,
+    const BoundNetLog& net_log) {
   bool enable_port_selection = enable_port_selection_;
   if (enable_port_selection &&
       ContainsKey(gone_away_aliases_, server_id)) {
@@ -1185,20 +1292,33 @@ int QuicStreamFactory::CreateSession(const QuicServerId& server_id,
     enable_port_selection = false;
     gone_away_aliases_.erase(server_id);
   }
-
-  QuicConnectionId connection_id = random_generator_->RandUint64();
-  IPEndPoint addr = *address_list.begin();
   scoped_refptr<PortSuggester> port_suggester =
       new PortSuggester(server_id.host_port_pair(), port_seed_);
   DatagramSocket::BindType bind_type = enable_port_selection ?
       DatagramSocket::RANDOM_BIND :  // Use our callback.
       DatagramSocket::DEFAULT_BIND;  // Use OS to randomize.
+
   scoped_ptr<DatagramClientSocket> socket(
       client_socket_factory_->CreateDatagramClientSocket(
           bind_type,
           base::Bind(&PortSuggester::SuggestPort, port_suggester),
           net_log.net_log(), net_log.source()));
 
+  UMA_HISTOGRAM_COUNTS("Net.QuicEphemeralPortsSuggested",
+                       port_suggester->call_count());
+  if (enable_port_selection) {
+    DCHECK_LE(1u, port_suggester->call_count());
+  } else {
+    DCHECK_EQ(0u, port_suggester->call_count());
+  }
+
+  return socket;
+}
+
+int QuicStreamFactory::ConfigureDatagramClientSocket(
+    DatagramClientSocket* socket,
+    IPEndPoint addr,
+    NetworkChangeNotifier::NetworkHandle network) {
   if (enable_non_blocking_io_ &&
       client_socket_factory_ == ClientSocketFactory::GetDefaultFactory()) {
 #if defined(OS_WIN)
@@ -1206,25 +1326,29 @@ int QuicStreamFactory::CreateSession(const QuicServerId& server_id,
 #endif
   }
 
-  int rv = socket->Connect(addr);
+  // If caller leaves network unspecified, find current default.
+  int rv;
+  if (network == NetworkChangeNotifier::kInvalidNetworkHandle) {
+    rv = socket->BindToDefaultNetwork();
+  } else {
+    rv = socket->BindToNetwork(network);
+  }
+  if (rv != OK) {
+    return rv;
+  }
 
+  rv = socket->Connect(addr);
   if (rv != OK) {
     HistogramCreateSessionFailure(CREATION_ERROR_CONNECTING_SOCKET);
     return rv;
   }
-  UMA_HISTOGRAM_COUNTS("Net.QuicEphemeralPortsSuggested",
-                       port_suggester->call_count());
-  if (enable_port_selection) {
-    DCHECK_LE(1u, port_suggester->call_count());
-  } else {
-    DCHECK_EQ(0u, port_suggester->call_count());
-  }
 
   rv = socket->SetReceiveBufferSize(socket_receive_buffer_size_);
   if (rv != OK) {
     HistogramCreateSessionFailure(CREATION_ERROR_SETTING_RECEIVE_BUFFER);
     return rv;
   }
+
   // Set a buffer large enough to contain the initial CWND's worth of packet
   // to work around the problem with CHLO packets being sent out with the
   // wrong encryption level, when the send buffer is full.
@@ -1244,14 +1368,36 @@ int QuicStreamFactory::CreateSession(const QuicServerId& server_id,
     }
   }
 
-  DefaultPacketWriterFactory packet_writer_factory(socket.get());
+  return OK;
+}
+
+int QuicStreamFactory::CreateSession(const QuicServerId& server_id,
+                                     int cert_verify_flags,
+                                     scoped_ptr<QuicServerInfo> server_info,
+                                     const AddressList& address_list,
+                                     base::TimeTicks dns_resolution_end_time,
+                                     const BoundNetLog& net_log,
+                                     QuicChromiumClientSession** session) {
+  IPEndPoint addr = *address_list.begin();
+
+  scoped_ptr<DatagramClientSocket> socket =
+      CreateDatagramClientSocket(server_id, net_log);
+
+  int rv = ConfigureDatagramClientSocket(
+      socket.get(), addr, NetworkChangeNotifier::kInvalidNetworkHandle);

[Ryan Hamilton, 2015/11/18 22:23:36]

PLease add a comment here about using an "invalid" network handle since that
sounds ... bad :>

[Jana, 2015/11/21 02:21:22]

added a comment here and in the .h declaration of ConfigureDatagramClientSocket.

+
+  if (rv != OK) {
+    return rv;
+  }
 
+  DefaultPacketWriterFactory packet_writer_factory(socket.get());
   if (!helper_.get()) {
     helper_.reset(
         new QuicConnectionHelper(base::ThreadTaskRunnerHandle::Get().get(),
                                  clock_.get(), random_generator_));
   }
 
+  QuicConnectionId connection_id = random_generator_->RandUint64();
   QuicConnection* connection = new QuicConnection(
       connection_id, addr, helper_.get(), packet_writer_factory,
       true /* owns_writer */, Perspective::IS_CLIENT, supported_versions_);