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 <utility>
 
 #include "base/callback_helpers.h"
 #include "base/location.h"
@@ skipping 35 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 166 matching lines @@
                                        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),
       write_pending_(false),
+      migration_alarm_pending_(false),
       weak_factory_(this) {
   sockets_.push_back(std::move(socket));
   packet_readers_.push_back(base::WrapUnique(new 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::WrapUnique(new ProofVerifyContextChromium(
                                cert_verify_flags, net_log_)),
           crypto_config));
@@ skipping 698 matching lines @@
 
 int QuicChromiumClientSession::HandleWriteError(
     int error_code,
     scoped_refptr<StringIOBuffer> packet) {
   DCHECK(packet != nullptr);
   DCHECK_NE(ERR_IO_PENDING, error_code);
   DCHECK_GT(0, error_code);
   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,
       base::Bind(&QuicChromiumClientSession::MigrateSessionOnWriteError,
                  weak_factory_.GetWeakPtr()));
 
   // Store packet in the session since the actual migration and packet rewrite
   // can happen via this posted task or via an async network notification.
   packet_ = packet;
   migration_pending_ = true;
   static_cast<QuicChromiumPacketWriter*>(connection()->writer())
       ->set_write_blocked(true);
 
   // 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;
 
-  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() {

[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_)
     return;
   // Prevent any pending migration or write tasks from executing.
   write_pending_ = false;
   migration_pending_ = false;
 
   static_cast<QuicChromiumPacketWriter*>(connection()->writer())
       ->set_write_blocked(false);
@@ skipping 24 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(result.error_code >= 0);
   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);
   connection()->OnWriteError(error_code);
 }
 
 void QuicChromiumClientSession::OnWriteUnblocked() {
   connection()->OnCanWrite();
 }
 
@@ skipping 271 matching lines @@
 }
 
 void QuicChromiumClientSession::DeletePromised(
     QuicClientPromisedInfo* promised) {
   if (IsOpenStream(promised->id()))
     streams_pushed_and_claimed_count_++;
   QuicClientSessionBase::DeletePromised(promised);
 }
 
 }  // namespace net