Changes connection migration code to migrate on NetworkMadeDefault

net/quic/chromium/quic_stream_factory.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/chromium/quic_stream_factory.h"
 
 #include <openssl/aead.h>
 
 #include <algorithm>
 #include <tuple>
@@ skipping 1436 matching lines @@
   status_ = OPEN;
   CloseAllSessions(ERR_NETWORK_CHANGED, QUIC_IP_ADDRESS_CHANGED);
   set_require_confirmation(true);
 }
 
 void QuicStreamFactory::OnNetworkConnected(NetworkHandle network) {
   num_timeouts_with_open_streams_ = 0;
   status_ = OPEN;
 }
 
-void QuicStreamFactory::OnNetworkMadeDefault(NetworkHandle network) {}
+void QuicStreamFactory::OnNetworkMadeDefault(NetworkHandle 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) {
   ScopedConnectionMigrationEventLog scoped_event_log(net_log_,
                                                      "OnNetworkDisconnected");
-  MaybeMigrateOrCloseSessions(network, /*force_close=*/true,
+  NetworkHandle new_network = FindAlternateNetwork(network);
+  MaybeMigrateOrCloseSessions(new_network, /*close_if_cannot_migrate=*/true,
                               scoped_event_log.net_log());
-  set_require_confirmation(true);
 }
 
 // This method is expected to only be called when migrating from Cellular to
-// WiFi on Android.
-void QuicStreamFactory::OnNetworkSoonToDisconnect(NetworkHandle network) {
-  ScopedConnectionMigrationEventLog scoped_event_log(
-      net_log_, "OnNetworkSoonToDisconnect");
-  MaybeMigrateOrCloseSessions(network, /*force_close=*/false,
-                              scoped_event_log.net_log());
-}
+// WiFi on Android, and should always be preceded by OnNetworkMadeDefault().
+void QuicStreamFactory::OnNetworkSoonToDisconnect(NetworkHandle network) {}
 
 NetworkHandle QuicStreamFactory::FindAlternateNetwork(
     NetworkHandle old_network) {
   // Find a new network that sessions bound to |old_network| can be migrated to.
   NetworkChangeNotifier::NetworkList network_list;
   NetworkChangeNotifier::GetConnectedNetworks(&network_list);
   for (NetworkHandle new_network : network_list) {
     if (new_network != old_network)
       return new_network;
   }
   return NetworkChangeNotifier::kInvalidNetworkHandle;
 }
 
 void QuicStreamFactory::MaybeMigrateOrCloseSessions(
-    NetworkHandle network,
-    bool force_close,
+    NetworkHandle new_network,
+    bool close_if_cannot_migrate,
     const BoundNetLog& bound_net_log) {
-  DCHECK_NE(NetworkChangeNotifier::kInvalidNetworkHandle, network);
-  NetworkHandle new_network = FindAlternateNetwork(network);
+  QuicStreamFactory::SessionIdMap::iterator it = all_sessions_.begin();
 
-  QuicStreamFactory::SessionIdMap::iterator it = all_sessions_.begin();
+  if (new_network == NetworkChangeNotifier::kInvalidNetworkHandle) {
+    // Migration attempted, but no new network was found. Close all sessions.
+    while (it != all_sessions_.end()) {

[Ryan Hamilton, 2016/08/19 22:03:53]

nit: instead of having this if(...) clause outside the while loop (which
duplicates the while loop conditions before, can you move this if and body (w/o
the while) down to line 1514... inside the main while?

[Jana, 2016/08/20 01:48:32]

It used to be inside, but I thought it might be clearer this way ... clearly not
:-)  Done.

+      QuicChromiumClientSession* session = it->first;
+      ++it;
+      HistogramAndLogMigrationFailure(
+          bound_net_log, MIGRATION_STATUS_NO_ALTERNATE_NETWORK,
+          session->connection_id(), "No alternate network found");
+      session->CloseSessionOnError(ERR_NETWORK_CHANGED,
+                                   QUIC_CONNECTION_MIGRATION_NO_NEW_NETWORK);
+    }
+    return;
+  }
+
   while (it != all_sessions_.end()) {
     QuicChromiumClientSession* session = it->first;
     ++it;
 
-    if (session->GetDefaultSocket()->GetBoundNetwork() != network) {
-      // If session is not bound to |network|, move on.
+    // If session is already bound to |new_network|, move on.
+    if (session->GetDefaultSocket()->GetBoundNetwork() == new_network) {
       HistogramAndLogMigrationFailure(
           bound_net_log, MIGRATION_STATUS_ALREADY_MIGRATED,
-          session->connection_id(), "Not bound to network");
+          session->connection_id(), "Already bound to new network");
       continue;
     }
+
+    // Close idle sessions.
     if (session->GetNumActiveStreams() == 0) {
-      // Close idle sessions.
       HistogramAndLogMigrationFailure(
           bound_net_log, MIGRATION_STATUS_NO_MIGRATABLE_STREAMS,
           session->connection_id(), "No active sessions");
       session->CloseSessionOnError(
           ERR_NETWORK_CHANGED, QUIC_CONNECTION_MIGRATION_NO_MIGRATABLE_STREAMS);
       continue;
     }
+
     // If session has active streams, mark it as going away.
     OnSessionGoingAway(session);
 
-    if (new_network == NetworkChangeNotifier::kInvalidNetworkHandle) {
-      // No new network was found.
-      // TODO (jri): Add histogram for this failure case.
-      bound_net_log.AddEvent(
-          NetLog::TYPE_QUIC_CONNECTION_MIGRATION_FAILURE,
-          base::Bind(&NetLogQuicConnectionMigrationFailureCallback,
-                     session->connection_id(), "No new network"));
-      if (force_close) {
-        session->CloseSessionOnError(ERR_NETWORK_CHANGED,
-                                     QUIC_CONNECTION_MIGRATION_NO_NEW_NETWORK);
-      }
-      continue;
-    }
+    // Do not migrate sessions where connection migration is disabled.
     if (session->config()->DisableConnectionMigration()) {
-      // Do not migrate sessions where connection migration is disabled by
-      // config.
       HistogramAndLogMigrationFailure(bound_net_log, MIGRATION_STATUS_DISABLED,
                                       session->connection_id(),
                                       "Migration disabled");
-      if (force_close) {
+      if (close_if_cannot_migrate) {
         // Close sessions where connection migration is disabled.

[Ryan Hamilton, 2016/08/19 22:03:53]

nit: up to you, but I'd drop this comment and the one below on 1554

[Jana, 2016/08/20 01:48:32]

Done.

         session->CloseSessionOnError(ERR_NETWORK_CHANGED,
                                      QUIC_IP_ADDRESS_CHANGED);
       }
       continue;
     }
+
+    // Do not migrate sessions with non-migratable streams.
     if (session->HasNonMigratableStreams()) {
-      // Do not migrate sessions with non-migratable streams.
       HistogramAndLogMigrationFailure(
           bound_net_log, MIGRATION_STATUS_NON_MIGRATABLE_STREAM,
           session->connection_id(), "Non-migratable stream");
-      if (force_close) {
+      if (close_if_cannot_migrate) {
         // Close sessions with non-migratable streams.
         session->CloseSessionOnError(
             ERR_NETWORK_CHANGED,
             QUIC_CONNECTION_MIGRATION_NON_MIGRATABLE_STREAM);
       }
       continue;
     }
 
     MigrateSessionToNewNetwork(session, new_network, bound_net_log, nullptr);
   }
@@ skipping 477 matching lines @@
   // Since the session was active, there's no longer an
   // HttpStreamFactoryImpl::Job running which can mark it broken, unless the TCP
   // job also fails. So to avoid not using QUIC when we otherwise could, we mark
   // it as recently broken, which means that 0-RTT will be disabled but we'll
   // still race.
   http_server_properties_->MarkAlternativeServiceRecentlyBroken(
       alternative_service);
 }
 
 }  // namespace net