Makes migration on write error asynchronous to avoid reentrancy issues

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 1527 matching lines @@
           session->connection_id(), "Non-migratable stream");
       if (close_if_cannot_migrate) {
         session->CloseSessionOnError(
             ERR_NETWORK_CHANGED,
             QUIC_CONNECTION_MIGRATION_NON_MIGRATABLE_STREAM);
       }
       continue;
     }
 
     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");
   if (!migrate_sessions_on_network_change_ ||
       (migration_cause == EARLY_MIGRATION && !migrate_sessions_early_) ||
       session->HasNonMigratableStreams() ||
       session->config()->DisableConnectionMigration()) {
     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());
   if (new_network == NetworkChangeNotifier::kInvalidNetworkHandle) {
     // No alternate network found.
     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(
     QuicChromiumClientSession* session,
     IPEndPoint peer_address,
     const BoundNetLog& bound_net_log) {
   if (!allow_server_migration_)
     return;
 
   IPEndPoint old_address;
   session->GetDefaultSocket()->GetPeerAddress(&old_address);
   DCHECK_EQ(old_address.GetFamily(), peer_address.GetFamily());
 
   // 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);
+    const BoundNetLog& bound_net_log) {
+  return MigrateSessionInner(session, session->connection()->peer_address(),
+                             network, close_session_on_error, bound_net_log);
 }
 
-void QuicStreamFactory::MigrateSession(QuicChromiumClientSession* session,
-                                       IPEndPoint peer_address,
-                                       NetworkHandle network,
-                                       bool close_session_on_error,
-                                       const BoundNetLog& bound_net_log,
-                                       scoped_refptr<StringIOBuffer> packet) {
+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.
   std::unique_ptr<DatagramClientSocket> socket(
       client_socket_factory_->CreateDatagramClientSocket(
           DatagramSocket::DEFAULT_BIND, RandIntCallback(),
           session->net_log().net_log(), session->net_log().source()));
   if (ConfigureSocket(socket.get(), peer_address, network) != OK) {
     HistogramAndLogMigrationFailure(
         bound_net_log, MIGRATION_STATUS_INTERNAL_ERROR,
         session->connection_id(), "Socket configuration failed");
     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,
                                    yield_after_packets_, yield_after_duration_,
                                    session->net_log()));
   std::unique_ptr<QuicChromiumPacketWriter> new_writer(
       new QuicChromiumPacketWriter(socket.get()));
   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
     // session will be marked as going away after a previous
     // migration, making subsequent migration impossible.

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

This is now reachable because of migration on write error, right?

Also, if a session is going away, during a network change it seems like we
should still migrate it. What do you think? (not necessarily in this CL, but
something to think about)

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

Yes -- comment removed. I had thought at first that going away prevented a
session from migrating, but I don't thin it's the case.
 
Yes, I think so too, and it should already happen. Marking a session as going
away does not keep it from migrating, AFAICT.

     HistogramAndLogMigrationFailure(
         bound_net_log, MIGRATION_STATUS_TOO_MANY_CHANGES,
         session->connection_id(), "Too many migrations");
     if (close_session_on_error) {
       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() {
   CloseAllSessions(ERR_CERT_DATABASE_CHANGED, QUIC_CONNECTION_CANCELLED);
 }
 
 void QuicStreamFactory::OnCertAdded(const X509Certificate* cert) {
   CloseAllSessions(ERR_CERT_DATABASE_CHANGED, QUIC_CONNECTION_CANCELLED);
 }
 
@@ skipping 380 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