Allows QUIC connections to remain established even though the peer's

net/quic/quic_connection.cc

Index: net/quic/quic_connection.cc
diff --git a/net/quic/quic_connection.cc b/net/quic/quic_connection.cc
index 6838a1ec30a8c07484d194943f1b5793ff0c9466..a5985e28d3db66fccbf6fab308aa4da279045ccc 100644
--- a/net/quic/quic_connection.cc
+++ b/net/quic/quic_connection.cc
@@ -204,6 +204,7 @@ QuicConnection::QuicConnection(QuicConnectionId connection_id,
       random_generator_(helper->GetRandomGenerator()),
       connection_id_(connection_id),
       peer_address_(address),
+      migrating_peer_port_(0),
       last_packet_revived_(false),
       last_size_(0),
       last_decrypted_packet_level_(ENCRYPTION_NONE),
@@ -234,7 +235,10 @@ QuicConnection::QuicConnection(QuicConnectionId connection_id,
       version_negotiation_state_(START_NEGOTIATION),
       is_server_(is_server),
       connected_(true),
-      address_migrating_(false),
+      peer_ip_changed_(false),
+      peer_port_changed_(false),
+      self_ip_changed_(false),
+      self_port_changed_(false),
       max_flow_control_receive_window_bytes_(
           max_flow_control_receive_window_bytes) {
   if (max_flow_control_receive_window_bytes_ < kDefaultFlowControlSendWindow) {
@@ -1105,18 +1109,7 @@ void QuicConnection::ProcessUdpPacket(const IPEndPoint& self_address,
   last_packet_revived_ = false;
   last_size_ = packet.length();
 
-  address_migrating_ = false;
-
-  if (peer_address_.address().empty()) {
-    peer_address_ = peer_address;
-  }
-  if (self_address_.address().empty()) {
-    self_address_ = self_address;
-  }
-
-  if (!(peer_address == peer_address_ && self_address == self_address_)) {
-    address_migrating_ = true;
-  }
+  CheckForAddressMigration(self_address, peer_address);
 
   stats_.bytes_received += packet.length();
   ++stats_.packets_received;
@@ -1141,6 +1134,34 @@ void QuicConnection::ProcessUdpPacket(const IPEndPoint& self_address,
   SetPingAlarm();
 }
 
+void QuicConnection::CheckForAddressMigration(
+    const IPEndPoint& self_address, const IPEndPoint& peer_address) {
+  peer_ip_changed_ = false;
+  peer_port_changed_ = false;
+  self_ip_changed_ = false;
+  self_port_changed_ = false;
+
+  if (peer_address_.address().empty()) {
+    peer_address_ = peer_address;
+  }
+  if (self_address_.address().empty()) {
+    self_address_ = self_address;
+  }
+
+  if (!peer_address.address().empty() && !peer_address_.address().empty()) {
+    peer_ip_changed_ = (peer_address.address() != peer_address_.address());
+    peer_port_changed_ = (peer_address.port() != peer_address_.port());
+
+    // Store in case we want to migrate connection in ProcessValidatedPacket.
+    migrating_peer_port_ = peer_address.port();
+  }
+
+  if (!self_address.address().empty() && !self_address_.address().empty()) {
+    self_ip_changed_ = (self_address.address() != self_address_.address());
+    self_port_changed_ = (self_address.port() != self_address_.port());
+  }
+}
+
 void QuicConnection::OnCanWrite() {
   DCHECK(!writer_->IsWriteBlocked());
 
@@ -1185,12 +1206,23 @@ void QuicConnection::WriteIfNotBlocked() {
 }
 
 bool QuicConnection::ProcessValidatedPacket() {
-  if (address_migrating_) {
+  if ((!FLAGS_quic_allow_port_migration && peer_port_changed_) ||
+      peer_ip_changed_ || self_ip_changed_ || self_port_changed_) {
     SendConnectionCloseWithDetails(
         QUIC_ERROR_MIGRATING_ADDRESS,
-        "Address migration is not yet a supported feature");
+        "IP or port migration is not yet a supported feature");

[wtc, 2014/05/13 16:37:15]

I think this error message is wrong. At least, it contradicts the comment on
line 1217 that port migration is supported.

One possible solution is to use different error messages:

  if (!FLAGS_quic_allow_port_migration && peer_port_changed_) {
    SendConnectionCloseWithDetails(
        QUIC_ERROR_MIGRATING_ADDRESS, "Port migration is not allowed");
    return false;
  }
  if (peer_ip_changed_ || self_ip_changed_ || self_port_changed_) {
    SendConnectionCloseWithDetails(
        QUIC_ERROR_MIGRATING_ADDRESS,
        "IP migration is not yet a supported feature");
    return false;
  }

The second error message is still not correct for the self_port_changed_ case
though.

[Robbie Shade, 2014/05/13 21:31:21]

How about:

"Neither IP address migration, nor server port migration are supported."

[wtc, 2014/05/13 23:20:41]

This sounds good. Did you mean "server port" or "self port"?

Also, do you think we should use a different error message for the
!FLAGS_quic_allow_port_migration && peer_port_changed_) case?

[Robbie Shade, 2014/05/13 23:30:11]

Yep "self port" is correct, although in practice "server port" is also right as
the only expected use case for this is a client behind a NAT.
Not worth changing I don't think - the flag will be removed in a few weeks once
it's been verified as safe.

[ramant (doing other things), 2014/05/14 05:30:23]

Done.
Acknowledged.

[ramant (doing other things), 2014/05/14 05:30:23]

Made this change in
https://codereview.chromium.org/286693003/diff/1/net/quic/quic_connection.cc

Done.

     return false;
   }
+
+  // Port migration is supported, do it now if port has changed.
+  if (FLAGS_quic_allow_port_migration &&
+      peer_port_changed_) {
+    DVLOG(1) << ENDPOINT << "Peer's port changed from "
+             << peer_address_.port() << " to " << migrating_peer_port_
+             << ", migrating connection.";
+    peer_address_ = IPEndPoint(peer_address_.address(), migrating_peer_port_);
+  }
+
   time_of_last_received_packet_ = clock_->Now();
   DVLOG(1) << ENDPOINT << "time of last received packet: "
            << time_of_last_received_packet_.ToDebuggingValue();