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

net/quic/chromium/quic_chromium_client_session.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_chromium_client_session.h"
 
 #include <openssl/ssl.h>
 
 #include <utility>
 
@@ skipping 39 matching lines @@
 // connection migration. A new Reader is created every time this endpoint's
 // IP address changes.
 const size_t kMaxReadersPerQuicSession = 5;
 
 // Size of the MRU cache of Token Binding signatures. Since the material being
 // signed is constant and there aren't many keys being used to sign, a fairly
 // small number was chosen, somewhat arbitrarily, and to match
 // 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
 enum Location {
   DESTRUCTOR = 0,
   ADD_OBSERVER = 1,
   TRY_CREATE_STREAM = 2,
   CREATE_OUTGOING_RELIABLE_STREAM = 3,
   NOTIFY_FACTORY_OF_SESSION_CLOSED_LATER = 4,
   NOTIFY_FACTORY_OF_SESSION_CLOSED = 5,
@@ skipping 165 matching lines @@
                                        connection_description,
                                        std::move(socket_performance_watcher),
                                        net_log_)),
       going_away_(false),
       port_migration_detected_(false),
       disabled_reason_(QUIC_DISABLED_NOT),
       token_binding_signatures_(kTokenBindingSignatureMapSize),
       streams_pushed_count_(0),
       streams_pushed_and_claimed_count_(0),
       migration_pending_(false),
+      migration_alarm_pending_(false),
       weak_factory_(this) {
   sockets_.push_back(std::move(socket));
   packet_readers_.push_back(base::MakeUnique<QuicChromiumPacketReader>(
       sockets_.back().get(), clock, this, yield_after_packets,
       yield_after_duration, net_log_));
   crypto_stream_.reset(
       crypto_client_stream_factory->CreateQuicCryptoClientStream(
           server_id, this, base::MakeUnique<ProofVerifyContextChromium>(
                                cert_verify_flags, net_log_),
           crypto_config));
@@ skipping 737 matching lines @@
   // Cause the packet writer to return ERR_IO_PENDING and block so
   // that the actual migration happens from the message loop instead
   // of under the call stack of QuicConnection::WritePacket.
   return ERR_IO_PENDING;
 }
 
 void QuicChromiumClientSession::MigrateSessionOnWriteError() {
   // If migration_pending_ is false, an earlier task completed migration.
   if (!migration_pending_)
     return;
 

[Ryan Hamilton, 2016/09/13 02:57:31]

What happened to all the vertical white space? I found it easier to read in the
old version.

[Jana, 2016/09/13 03:12:14]

I wasn't sure if it was too much white space, so I had removed it. Added space.

-  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.
   connection()->CloseConnection(QUIC_PACKET_WRITE_ERROR,
                                 "Write and subsequent migration failed",
                                 ConnectionCloseBehavior::SILENT_CLOSE);
 }
 
+void QuicChromiumClientSession::OnNoNewNetwork() {
+  if (migration_alarm_pending_)
+    return;
+  migration_alarm_pending_ = true;
+  // When called from a network notification, migration_pending_ must
+  // be set to true here.

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

I don't understand this comment. Perhaps you can explain why this needs to be
set?

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

Comment changed PTAL. 

+  migration_pending_ = true;
+
+  // Block the packet writer to avoid any writes while migration is in progress.
+  static_cast<QuicChromiumPacketWriter*>(connection()->writer())
+      ->set_write_blocked(true);
+
+  // 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() {
   // Prevent any pending migration from executing.
   migration_pending_ = false;
 
   static_cast<QuicChromiumPacketWriter*>(connection()->writer())
       ->set_write_blocked(false);
   DCHECK(!connection()->writer()->IsWriteBlocked());
 
   if (packet_ == nullptr) {
     // Unblock the connection before sending a PING packet, since it
@@ skipping 17 matching lines @@
           ->WritePacketToSocket(packet);
 
   if (result.error_code == ERR_IO_PENDING)
     return;
   // All write errors should be mapped into ERR_IO_PENDING by
   // HandleWriteError.
   DCHECK_LT(0, result.error_code);
   connection()->OnCanWrite();
 }
 
+void QuicChromiumClientSession::OnMigrationTimeout() {
+  DLOG(INFO) << "In OnMigrationTimeout: migration_pending_: " << migration_pending_;

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

nit: remove this?

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

Done.

+  if (!migration_pending_)

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

As discussed, if two migrations start before the first timeout, then the
connection will close prematurely. the sockets_.size() trick will fix this. Just
recording this for posterity since you're going to do it in a followup.

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

Changed to using sockets_.size().

+    return;
+  migration_alarm_pending_ = false;
+  migration_pending_ = false;

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

Since the connection is about to be deleted, I suspect you don't need these two
lines, right?

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

Yup, removed.

+  /*
+  UMA_HISTOGRAM_ENUMERATION(
+      "Net.QuicSession.ConnectionMigration",
+      static_cast<int>(MigrationStatus::NO_ALTERNATE_NETWORK),
+      static_cast<int>(MigrationStatus::MAX));
+  */

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

What's up with this?

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

Fixed.

+  CloseSessionOnError(ERR_NETWORK_CHANGED,
+                      QUIC_CONNECTION_MIGRATION_NO_NEW_NETWORK);
+}
+
+void QuicChromiumClientSession::OnNetworkConnected(
+    NetworkChangeNotifier::NetworkHandle network,
+    const BoundNetLog& bound_net_log) {
+  // Cancel migration alarm if one is pending.
+  migration_alarm_pending_ = false;
+  // If migration_pending_ is false, there was no migration pending or
+  // an earlier task completed migration.
+  if (!migration_pending_)
+    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);
   connection()->OnWriteError(error_code);
 }
 
 void QuicChromiumClientSession::OnWriteUnblocked() {
   connection()->OnCanWrite();
 }
 
@@ skipping 268 matching lines @@
 }
 
 void QuicChromiumClientSession::DeletePromised(
     QuicClientPromisedInfo* promised) {
   if (IsOpenStream(promised->id()))
     streams_pushed_and_claimed_count_++;
   QuicClientSessionBase::DeletePromised(promised);
 }
 
 }  // namespace net