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 62fd664968e90222bff02d26f3fd20676184ffc9..cab6455116ae3dfe63bfc7a0c088154814e9b3da 100644
--- a/net/quic/quic_stream_factory.cc
+++ b/net/quic/quic_stream_factory.cc
@@ -56,6 +56,7 @@
 #endif
 
 using std::min;
+using NetworkChangeNotifier::NetworkHandle;
 
 namespace net {
 
@@ -78,6 +79,9 @@ const int32 kQuicStreamMaxRecvWindowSize = 6 * 1024 * 1024;    // 6 MB
 // Set the maximum number of undecryptable packets the connection will store.
 const int32 kMaxUndecryptablePackets = 100;
 
+// A default NetworkHandle.
+const NetworkHandle kDefaultNetworkHandle = -2;

[pauljensen, 2015/10/14 11:51:34]

This is a little sketchy for my liking.  I'd suggest either (listed in order of
increasing sketchyness):
1. add "bool bind_to_default_network" argument to
CreateAndConfigureDatagramSocket
2. overload kInvalidNetworkHandle to mean bind-to-default-network when passed to
CreateAndConfigureDatagramSocket
3. define this to kInvalidNetworkHandle-1

[Jana, 2015/10/16 23:11:00]

I think this is all sketchy :-) But, yeah, it seems reasonable to overload
kInvalid instead of creating a new value, so using option 2.

+
 void HistogramCreateSessionFailure(enum CreateSessionFailure error) {
   UMA_HISTOGRAM_ENUMERATION("Net.QuicSession.CreationError", error,
                             CREATION_ERROR_MAX);
@@ -1137,11 +1141,108 @@ void QuicStreamFactory::ClearCachedStatesInCryptoConfig() {
   crypto_config_.ClearCachedStates();
 }
 
+// We still need OnIPAddressChanged, but not for Android >= L.
+// pauljensen: How do we do this?

[pauljensen, 2015/10/14 11:51:34]

Where do you register for events? (i.e. call
NetworkChangeNotifier::AddIPAddressObserver)?

I'd add a flag controlling all this new behavior and in the constructor I'd add
some:
   migrate_connections(some_quic_connection_migration_flag_thingy
#if defined(OS_ANDROID)
      && base::android::BuildInfo::GetInstance()->sdk_int() >=
         base::android::SDK_VERSION_LOLLIPOP
#endif

Where ever you call AddIPAddressObserver I'd change the code to:
  if (migrate_connections) {
    NetworkChangeNotifier::AddNetworkObserver(this);
  } else {
    NetworkChangeNotifier::AddIPAddressObserver(this);
  }

[Jana, 2015/10/16 23:11:00]

*Facepalm*. So I discovered that we do not actually register for events
currently. Fixing now.
Also, added code here to add NetworkObserver.

 void QuicStreamFactory::OnIPAddressChanged() {
   CloseAllSessions(ERR_NETWORK_CHANGED);
   set_require_confirmation(true);
 }
 
+void QuicStreamFactory::OnNetworkConnected(NetworkHandle network) {
+  // Nothing to do.
+}
+
+void QuicStreamFactory::OnNetworkMadeDefault(NetworkHandle network) {
+  // Nothing to do.
+}
+
+void QuicStreamFactory::OnNetworkDisconnected(NetworkHandle network) {
+  MigrateOrCloseSessions(network);
+}
+
+void QuicStreamFactory::OnNetworkSoonToDisconnect(NetworkHandle network) {
+  MigrateOrCloseSessions(network);
+}
+
+void QuicStreamFactory::MigrateOrCloseSessions(NetworkHandle network) {
+  // pauljensen: do we need the following "== network" check?

[pauljensen, 2015/10/14 11:51:34]

seems like a good idea

[Jana, 2015/10/16 23:11:00]

Acknowledged.

+  NetworkList network_list;
+  GetConnectedNetworks(&network_list);
+  if (network_list.isEmpty() || 
+      (network_list.size() == 1 && network_list[0] == network)) {
+    CloseAllSessions(ERR_NETWORK_CHANGED);
+    set_require_confirmation(true);
+    return;
+  }
+
+  // Find a new network that old sockets can be migrated to.
+  // pauljensen: do we need the following "== network" check?

[pauljensen, 2015/10/14 11:51:34]

seems like a good idea

[Jana, 2015/10/16 23:11:00]

Acknowledged.

+  NetworkHandle new_network = kInvalidNetworkHandle;
+  for (NetworkHandle n : network_list) {
+    if (n == network) {
+      continue;
+    }
+    new_network = n;
+  }
+
+  DCHECK_NE(kInvalidNetworkHandle, new_network);
+  DCHECK_NE(network, new_network);
+
+  for (QuicStreamFactory::SessionIdMap::iterator it = all_sessions_.begin();
+       it != all_sessions_.end(); ++it) {
+    QuicChromiumClientSession* session = it->first;
+    QuicServerId server_id = it->second;
+
+    if (session->GetNumOpenStreams() == 0) {
+      // If idle session, close session
+      active_sessions_.erase(server_id);
+      session->CloseSessionOnError(ERR_NETWORK_CHANGED, QUIC_INTERNAL_ERROR);
+    } 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;
+    } else {
+      // If session is active, 
+      // (i) make the session GO_AWAY,
+      // (ii) create a new socket (which should be bound to the new interface), 
+      // (iii) add a new PacketReader to the session to read from new socket, 
+      // (iv) replace the connection's PacketWriter with a new PacketWriter 
+      // that uses the new socket.
+      set_require_confirmation(true);
+      OnSessionGoingAway(session);
+      scoped_ptr<DatagramClientSocket> socket;
+      int rv = CreateAndConfigureDatagramSocket(&socket,
+                                                all_sessions[*session],
+                                                connection->peer_address(),
+                                                session->net_log(),
+						new_network);
+      if (rv != OK) {
+	session->CloseSessionOnError(ERR_NETWORK_CHANGED, QUIC_INTERNAL_ERROR);
+        continue;
+      }
+
+      if (!session->AddPacketReader(new QuicPacketReader(socket.get(), 
+                                                         clock_.get(),
+                                                         session,
+                                                         yield_after_packets_,
+                                                         yield_after_duration_,
+                                                         session->net_log()))) {
+	session->CloseSessionOnError(ERR_NETWORK_CHANGED, QUIC_INTERNAL_ERROR);
+        continue;
+      }
+
+      session->StartReading();
+      DefaultPacketWriterFactory packet_writer_factory(socket.get());
+      QuicConnection* connection = session->connection();
+      connection->SetQuicPacketWriter(packet_writer_factory.Create(connection),
+                                      /*owns_writer=*/true);
+
+      session->AddSocket(socket.Pass());
+    }
+  }
+}
+
 void QuicStreamFactory::OnSSLConfigChanged() {
   CloseAllSessions(ERR_CERT_DATABASE_CHANGED);
 }
@@ -1172,13 +1273,12 @@ 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) {
+int QuicStreamFactory::CreateAndConfigureDatagramSocket(
+    scoped_ptr<DatagramClientSocket>* new_socket,

[pauljensen, 2015/10/14 11:51:34]

http://dev.chromium.org/developers/smart-pointer-guidelines
says:
"You should especially never see scoped_ptr<>*"

[Jana, 2015/10/16 23:11:00]

Done.

+    const QuicServerId& server_id,
+    IPEndPoint addr,
+    const BoundNetLog& net_log,
+    NetworkHandle network) {
   bool enable_port_selection = enable_port_selection_;
   if (enable_port_selection &&
       ContainsKey(gone_away_aliases_, server_id)) {
@@ -1189,8 +1289,6 @@ int QuicStreamFactory::CreateSession(const QuicServerId& server_id,
     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 ?
@@ -1209,8 +1307,22 @@ int QuicStreamFactory::CreateSession(const QuicServerId& server_id,
 #endif
   }
 
-  int rv = socket->Connect(addr);
+  // If caller leaves network unspecified, find current default.
+  if (network == kDefaultNetworkHandle) {
+    network = NetworkChangeNotifier::GetDefaultNetwork();

[pauljensen, 2015/10/14 11:51:34]

We may want to loop here in cases where the default changes quickly.  I thought
we were going to add DatagramClientSocket::BindToDefaultNetwork() that included
the looping?  So here we'd have something like:
if (migrate_connections) {
  int rv;
  if (network == kDefaultNetworkHandle) {
    rv = socket->BindToDefaultNetwork();
  } else {
    rv = socket->BindToNetwork(network);
  }
  if (rv != OK)
    return rv;
}

[Jana, 2015/10/16 23:11:00]

I've added a BindToDefaultNetwork method to DatagramClientSocket. I don't think
I understand how you see the looping work -- what if there's no default network
for a long time? Does it make sense to consider using a timer to wait "a short
while" and then give up?

+    if (network == kDefaultNetworkHandle) {
+      return ERR_INTERNET_DISCONNECTED;
+    }
+  }
+
+  new_socket->Reset(socket.get());
 
+  int rv = socket->BindToNetwork(network);
+  if (rv != OK) {
+    return rv;
+  }
+
+  rv = socket->Connect(addr);
   if (rv != OK) {
     HistogramCreateSessionFailure(CREATION_ERROR_CONNECTING_SOCKET);
     return rv;
@@ -1246,15 +1358,34 @@ int QuicStreamFactory::CreateSession(const QuicServerId& server_id,
       require_confirmation_ = false;
     }
   }
+  return OK;
+}
 
-  DefaultPacketWriterFactory packet_writer_factory(socket.get());
+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> socket;
+  int rv = CreateAndConfigureDatagramSocket(&socket,
+                                            server_id, 
+                                            *address_list.begin(), 
+                                            net_log,
+                                            kDefaultNetworkHandle);
+  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, server_id.is_https(),