Introduces ability for session to wait on a migration trigger for a new

net/quic/chromium/quic_chromium_client_session.cc

Index: net/quic/chromium/quic_chromium_client_session.cc
diff --git a/net/quic/chromium/quic_chromium_client_session.cc b/net/quic/chromium/quic_chromium_client_session.cc
index 11019aa75af5c583e7e0a532c8d5c17f54b5c4ca..42a0de6c613971795983b7332d7c818d0bca563d 100644
--- a/net/quic/chromium/quic_chromium_client_session.cc
+++ b/net/quic/chromium/quic_chromium_client_session.cc
@@ -53,6 +53,10 @@ const size_t kMaxReadersPerQuicSession = 5;
 // SSLClientSocketImpl.
 const size_t kTokenBindingSignatureMapSize = 10;
 
+// Time to wait (in seconds) when no networks are available and
+// migrating sessions need to wait for a new network to connect.
+const size_t kWaitTimeForNewNetworkSecs = 10;
+
 // Histograms for tracking down the crashes from http://crbug.com/354669
 // Note: these values must be kept in sync with the corresponding values in:
 // tools/metrics/histograms/histograms.xml
@@ -239,6 +243,7 @@ QuicChromiumClientSession::QuicChromiumClientSession(
       streams_pushed_and_claimed_count_(0),
       migration_pending_(false),
       write_pending_(false),
+      migration_alarm_pending_(false),
       weak_factory_(this) {
   sockets_.push_back(std::move(socket));
   packet_readers_.push_back(base::WrapUnique(new QuicChromiumPacketReader(
@@ -957,6 +962,7 @@ int QuicChromiumClientSession::HandleWriteError(
   DCHECK(!migration_pending_);
   DCHECK(packet_ == nullptr);
 
+  DLOG(INFO) << "Write error encountered.";
   // Post a task to migrate the session onto a new network.
   task_runner_->PostTask(
       FROM_HERE,
@@ -981,10 +987,18 @@ void QuicChromiumClientSession::MigrateSessionOnWriteError() {
   if (!migration_pending_)
     return;
 
-  if (stream_factory_ != nullptr &&
-      stream_factory_->MaybeMigrateSingleSession(this, WRITE_ERROR) ==
-          MigrationResult::SUCCESS)
+  MigrationResult result = MigrationResult::FAILURE;
+  if (stream_factory_ != nullptr) {
+    result = stream_factory_->MaybeMigrateSingleSession(this, WRITE_ERROR);
+  }
+
+  if (result == MigrationResult::SUCCESS)
+    return;
+
+  if (result == MigrationResult::NO_NEW_NETWORK) {
+    OnNoNewNetwork();
     return;
+  }
 
   // Close the connection if migration failed. Do not cause a
   // connection close packet to be sent since socket may be borked.
@@ -993,6 +1007,22 @@ void QuicChromiumClientSession::MigrateSessionOnWriteError() {
                                 ConnectionCloseBehavior::SILENT_CLOSE);
 }
 
+void QuicChromiumClientSession::OnNoNewNetwork() {

[Ryan Hamilton, 2016/09/12 03:42:01]

So this is called, obviously, from the write error code path. But it's also
called from any of the various NCN events, right?

[Jana, 2016/09/12 23:11:01]

Yes. It's called only for the DISCONNECTED event, since the other events are
CONNECTED and MADE_DEFAULT, which come with a valid network specified.

+  if (migration_alarm_pending_)
+    return;
+  migration_alarm_pending_ = true;
+
+  // Block the packet writer to avoid any writes while migration is in progress.
+  static_cast<QuicChromiumPacketWriter*>(connection()->writer())
+      ->set_write_blocked(true);

[Ryan Hamilton, 2016/09/12 03:42:01]

I don't understand the motivation for this. What happens if we don't do this? it
seems like this will be handled via the HandleWriteError code path.

[Jana, 2016/09/12 23:11:01]

This causes an NCN notification to block the socket to avoid any subsequent
writes to this socket. If we don't do this, there may be later write events to a
disconnected socket causing write error to happen.

[Ryan Hamilton, 2016/09/13 00:33:18]

What makes the socket "disconnected" in this case?

[Jana, 2016/09/13 01:12:18]

Something in Android? So this is the case where we aren't trying to write but
receive a notification from NCN that network is disconnected. Presumably, the
socket is borked (or in some indeterminate state), but we may try to write to it
later and go down the HandleWriteError path. It seems to make sense to disallow
any writes while we expect the socket to not be working.

+
+  // Post a task to maybe close the session if the alarm fires.
+  task_runner_->PostDelayedTask(
+      FROM_HERE, base::Bind(&QuicChromiumClientSession::OnMigrationTimeout,
+                            weak_factory_.GetWeakPtr()),
+      base::TimeDelta::FromSeconds(kWaitTimeForNewNetworkSecs));
+}
+
 void QuicChromiumClientSession::WriteToNewSocket() {
   // If write_pending_ is false, an earlier task wrote to the new socket.
   if (!write_pending_)
@@ -1037,6 +1067,38 @@ void QuicChromiumClientSession::WriteToNewSocket() {
   connection()->OnCanWrite();
 }
 
+void QuicChromiumClientSession::OnMigrationTimeout() {
+  if (!migration_alarm_pending_)
+    return;

[Ryan Hamilton, 2016/09/12 03:42:01]

So does this mean that if multiple timeouts are queued, only the oldest one
fires? I'd think we actually want the most recent to fire.

[Jana, 2016/09/12 23:11:01]

I don't think it matters much, since it's either DISCONNECTED or write error
that sets this alarm, and they should be in close proximity (basically a race in
Android between the events.) The way the code is now, it's also easier to
arrange for the first one to continue.

+  migration_alarm_pending_ = false;
+  migration_pending_ = false;
+  write_pending_ = false;
+  /*
+  UMA_HISTOGRAM_ENUMERATION(
+      "Net.QuicSession.ConnectionMigration",
+      static_cast<int>(MigrationStatus::NO_ALTERNATE_NETWORK),
+      static_cast<int>(MigrationStatus::MAX));
+  */
+  CloseSessionOnError(ERR_NETWORK_CHANGED,
+                      QUIC_CONNECTION_MIGRATION_NO_NEW_NETWORK);
+}
+
+void QuicChromiumClientSession::OnNetworkConnected(
+    NetworkChangeNotifier::NetworkHandle network,
+    const BoundNetLog& bound_net_log) {
+  // Cancel migration alarm.
+  migration_alarm_pending_ = false;
+  // If migration_pending_ is false, an earlier task completed migration.
+  if (!migration_pending_)

[Ryan Hamilton, 2016/09/12 03:42:01]

With this CL, migration_pending_ is now true both for the write_error_ case, or
this new delayed case, right? And in both cases, this is guarding the call to
MigrateToNewNetwork, right? (Just confirming).

(I think the sockets_.size() trick would also work here, but I just wanted to
make sure I understood what's going on)

Hm, that's not quite right. The write error case calls
MaybeMigrateSingleSession. But that ends up calling MigrateToNewNetwork?

[Jana, 2016/09/13 01:12:18]

That's correct.
That's right.

+    return;
+  // TODO(jri): Ensure that OnSessionGoingAway is called consistently,
+  // and that it's always called at the same time in the whole
+  // migration process. Allows tests to be more uniform.
+  stream_factory_->OnSessionGoingAway(this);
+  stream_factory_->MigrateSessionToNewNetwork(
+      this, network, /*close_session_on_error=*/true, net_log_);
+}
+
 void QuicChromiumClientSession::OnWriteError(int error_code) {
   DCHECK_NE(ERR_IO_PENDING, error_code);
   DCHECK_GT(0, error_code);