Makes migration on write error asynchronous to avoid reentrancy issues

net/quic/chromium/quic_stream_factory.cc

Index: net/quic/chromium/quic_stream_factory.cc
diff --git a/net/quic/chromium/quic_stream_factory.cc b/net/quic/chromium/quic_stream_factory.cc
index 07065e3941fca3cfe58525a3593c9e11e5822857..49dc1816fa1bde645e6046e95e9a6d2050f3b81f 100644
--- a/net/quic/chromium/quic_stream_factory.cc
+++ b/net/quic/chromium/quic_stream_factory.cc
@@ -1545,15 +1545,13 @@ void QuicStreamFactory::MaybeMigrateOrCloseSessions(
     }
 
     MigrateSessionToNewNetwork(session, new_network,
-                               /*close_session_on_error=*/true, bound_net_log,
-                               nullptr);
+                               /*close_session_on_error=*/true, bound_net_log);
   }
 }
 
-void QuicStreamFactory::MaybeMigrateSingleSession(
+MigrationResult QuicStreamFactory::MaybeMigrateSingleSession(
     QuicChromiumClientSession* session,
-    MigrationCause migration_cause,
-    scoped_refptr<StringIOBuffer> packet) {
+    MigrationCause migration_cause) {
   ScopedConnectionMigrationEventLog scoped_event_log(
       net_log_,
       migration_cause == EARLY_MIGRATION ? "EarlyMigration" : "WriteError");
@@ -1564,7 +1562,7 @@ void QuicStreamFactory::MaybeMigrateSingleSession(
     HistogramAndLogMigrationFailure(
         scoped_event_log.net_log(), MIGRATION_STATUS_DISABLED,
         session->connection_id(), "Migration disabled");
-    return;
+    return MigrationResult::FAILURE;
   }
   NetworkHandle new_network =
       FindAlternateNetwork(session->GetDefaultSocket()->GetBoundNetwork());
@@ -1573,12 +1571,14 @@ void QuicStreamFactory::MaybeMigrateSingleSession(
     HistogramAndLogMigrationFailure(
         scoped_event_log.net_log(), MIGRATION_STATUS_NO_ALTERNATE_NETWORK,
         session->connection_id(), "No alternate network found");
-    return;
+    return MigrationResult::NO_NEW_NETWORK;
   }
   OnSessionGoingAway(session);
-  MigrateSessionToNewNetwork(session, new_network,
-                             migration_cause != WRITE_ERROR,
-                             scoped_event_log.net_log(), packet);
+  return MigrateSessionToNewNetwork(session, new_network,

[Ryan Hamilton, 2016/09/10 16:40:34]

Seems like it might just be cleaner to have this method (and Inner) just return
a MigrationResult.

[Jana, 2016/09/10 23:08:31]

Done.

+                                    migration_cause != WRITE_ERROR,
+                                    scoped_event_log.net_log())
+             ? MigrationResult::SUCCESS
+             : MigrationResult::FAILURE;
 }
 
 void QuicStreamFactory::MigrateSessionToNewPeerAddress(
@@ -1594,27 +1594,25 @@ void QuicStreamFactory::MigrateSessionToNewPeerAddress(
 
   // Specifying kInvalidNetworkHandle for the |network| parameter
   // causes the session to use the default network for the new socket.
-  MigrateSession(session, peer_address,
-                 NetworkChangeNotifier::kInvalidNetworkHandle,
-                 /*close_session_on_error=*/true, bound_net_log, nullptr);
+  MigrateSessionInner(session, peer_address,
+                      NetworkChangeNotifier::kInvalidNetworkHandle,
+                      /*close_session_on_error=*/true, bound_net_log);
 }
 
-void QuicStreamFactory::MigrateSessionToNewNetwork(
+bool QuicStreamFactory::MigrateSessionToNewNetwork(
     QuicChromiumClientSession* session,
     NetworkHandle network,
     bool close_session_on_error,
-    const BoundNetLog& bound_net_log,
-    scoped_refptr<StringIOBuffer> packet) {
-  MigrateSession(session, session->connection()->peer_address(), network,
-                 close_session_on_error, bound_net_log, packet);
-}
-
-void QuicStreamFactory::MigrateSession(QuicChromiumClientSession* session,
-                                       IPEndPoint peer_address,
-                                       NetworkHandle network,
-                                       bool close_session_on_error,
-                                       const BoundNetLog& bound_net_log,
-                                       scoped_refptr<StringIOBuffer> packet) {
+    const BoundNetLog& bound_net_log) {
+  return MigrateSessionInner(session, session->connection()->peer_address(),
+                             network, close_session_on_error, bound_net_log);
+}
+
+bool QuicStreamFactory::MigrateSessionInner(QuicChromiumClientSession* session,
+                                            IPEndPoint peer_address,
+                                            NetworkHandle network,
+                                            bool close_session_on_error,
+                                            const BoundNetLog& bound_net_log) {
   // Use OS-specified port for socket (DEFAULT_BIND) instead of
   // using the PortSuggester since the connection is being migrated
   // and not being newly created.
@@ -1629,7 +1627,7 @@ void QuicStreamFactory::MigrateSession(QuicChromiumClientSession* session,
     if (close_session_on_error) {
       session->CloseSessionOnError(ERR_NETWORK_CHANGED, QUIC_INTERNAL_ERROR);
     }
-    return;
+    return false;
   }
   std::unique_ptr<QuicChromiumPacketReader> new_reader(
       new QuicChromiumPacketReader(socket.get(), clock_.get(), session,
@@ -1640,7 +1638,7 @@ void QuicStreamFactory::MigrateSession(QuicChromiumClientSession* session,
   new_writer->set_delegate(session);
 
   if (!session->MigrateToSocket(std::move(socket), std::move(new_reader),
-                                std::move(new_writer), packet)) {
+                                std::move(new_writer))) {
     // TODO(jokulik): It's not clear how we could end up on this code
     // path.  We would theoretically hit this failure if we've
     // performed too many migrations on this session.  However, the
@@ -1653,13 +1651,14 @@ void QuicStreamFactory::MigrateSession(QuicChromiumClientSession* session,
       session->CloseSessionOnError(ERR_NETWORK_CHANGED,
                                    QUIC_CONNECTION_MIGRATION_TOO_MANY_CHANGES);
     }
-    return;
+    return false;
   }
   HistogramMigrationStatus(MIGRATION_STATUS_SUCCESS);
   bound_net_log.AddEvent(
       NetLog::TYPE_QUIC_CONNECTION_MIGRATION_SUCCESS,
       base::Bind(&NetLogQuicConnectionMigrationSuccessCallback,
                  session->connection_id()));
+  return true;
 }
 
 void QuicStreamFactory::OnSSLConfigChanged() {