Land Recent QUIC Changes.

net/quic/quic_unacked_packet_map.cc

 // Copyright 2014 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/quic_unacked_packet_map.h"
 
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "net/quic/quic_connection_stats.h"
 #include "net/quic/quic_utils_chromium.h"
@@ skipping 73 matching lines @@
   while (it != unacked_packets_.end() && num_to_clear > 0) {
     QuicPacketSequenceNumber sequence_number = it->first;
     // If this is a pending packet, or has retransmittable data, then there is
     // no point in clearing out any further packets, because they would not
     // affect the high water mark.
     if (it->second.pending || it->second.retransmittable_frames != NULL) {
       break;
     }
 
     ++it;
-    NeuterIfPendingOrRemovePacket(sequence_number);
+    RemovePacket(sequence_number);
     --num_to_clear;
   }
 }
 
 bool QuicUnackedPacketMap::HasRetransmittableFrames(
     QuicPacketSequenceNumber sequence_number) const {
   const TransmissionInfo* transmission_info =
       FindOrNull(unacked_packets_, sequence_number);
   if (transmission_info == NULL) {
     return false;
   }
 
   return transmission_info->retransmittable_frames != NULL;
 }
 
 void QuicUnackedPacketMap::NackPacket(QuicPacketSequenceNumber sequence_number,
                                       size_t min_nacks) {
   UnackedPacketMap::iterator it = unacked_packets_.find(sequence_number);
   if (it == unacked_packets_.end()) {
     LOG(DFATAL) << "NackPacket called for packet that is not unacked: "
                 << sequence_number;
     return;
   }
 
   it->second.nack_count = max(min_nacks, it->second.nack_count);
 }
 
-void QuicUnackedPacketMap::NeuterIfPendingOrRemovePacket(
+void QuicUnackedPacketMap::RemovePacket(
     QuicPacketSequenceNumber sequence_number) {
   UnackedPacketMap::iterator it = unacked_packets_.find(sequence_number);
   if (it == unacked_packets_.end()) {
     LOG(DFATAL) << "packet is not unacked: " << sequence_number;
     return;
   }
   TransmissionInfo* transmission_info = &it->second;
+  DCHECK(!transmission_info->pending);
+  MaybeRemoveRetransmittableFrames(transmission_info);
+  transmission_info->all_transmissions->erase(sequence_number);
+  if (transmission_info->all_transmissions->empty()) {
+    delete transmission_info->all_transmissions;

[wtc, 2014/05/13 18:49:23]

IMPORTANT: should we set transmission_info->all_transmissions to NULL to avoid a
dangling pointer?

[Ian Swett, 2014/05/13 18:58:28]

It shouldn't matter, since the element is being erased in two lines later.

+  }
+  unacked_packets_.erase(it);
+}
+
+void QuicUnackedPacketMap::NeuterPacket(
+    QuicPacketSequenceNumber sequence_number) {
+  UnackedPacketMap::iterator it = unacked_packets_.find(sequence_number);
+  if (it == unacked_packets_.end()) {
+    LOG(DFATAL) << "packet is not unacked: " << sequence_number;
+    return;
+  }
+  TransmissionInfo* transmission_info = &it->second;
+  // TODO(ianswett): Ensure packets are pending before neutering them.
+  MaybeRemoveRetransmittableFrames(transmission_info);
+  if (transmission_info->all_transmissions->size() > 1) {

[wtc, 2014/05/13 18:49:23]

Can you explain why we only do the following when
transmission_info->all_transmissions has more than one element?

[Ian Swett, 2014/05/13 18:58:28]

Neuter packet is supposed to remove the retranmittable frames and isolate the
packet from other copies, under the assumption that only the pending packets
would be kept at this point.

I will note that Ryan and I are working on a plan to clean this up
substantially, because it's confusing as possible.

+    transmission_info->all_transmissions->erase(sequence_number);
+    transmission_info->all_transmissions = new SequenceNumberSet();

[wtc, 2014/05/13 18:49:23]

IMPORTANT: do we leak the old value of transmission_info->all_transmissions
here?

[Ian Swett, 2014/05/13 18:58:28]

No, if the map's size > 1, that means someone else already owns the map. 
Whoever takes it from size 1 to 0 is responsible for deleting it.

Also, this would be caught by asan and other tools quite quickly.

+    transmission_info->all_transmissions->insert(sequence_number);
+  }
+}
+
+void QuicUnackedPacketMap::MaybeRemoveRetransmittableFrames(
+    TransmissionInfo* transmission_info) {
   if (transmission_info->retransmittable_frames != NULL) {
     if (transmission_info->retransmittable_frames->HasCryptoHandshake()
             == IS_HANDSHAKE) {
       --pending_crypto_packet_count_;
     }
     delete transmission_info->retransmittable_frames;
     transmission_info->retransmittable_frames = NULL;
   }
-  if (transmission_info->pending) {
-    // Neuter it so it can't be retransmitted.
-    if (transmission_info->all_transmissions->size() > 1) {
-      transmission_info->all_transmissions->erase(sequence_number);
-      transmission_info->all_transmissions = new SequenceNumberSet();
-      transmission_info->all_transmissions->insert(sequence_number);
-    }
-  } else {
-    // Remove it.
-    transmission_info->all_transmissions->erase(sequence_number);
-    if (transmission_info->all_transmissions->empty()) {
-      delete transmission_info->all_transmissions;
-    }
-    unacked_packets_.erase(it);
-  }
 }
 
 // static
 bool QuicUnackedPacketMap::IsSentAndNotPending(
     const TransmissionInfo& transmission_info) {
   return !transmission_info.pending &&
       transmission_info.sent_time != QuicTime::Zero() &&
       transmission_info.bytes_sent == 0;
 }
 
 bool QuicUnackedPacketMap::IsUnacked(
     QuicPacketSequenceNumber sequence_number) const {
   return ContainsKey(unacked_packets_, sequence_number);
 }
 
-bool QuicUnackedPacketMap::IsPending(
-    QuicPacketSequenceNumber sequence_number) const {
-  const TransmissionInfo* transmission_info =
-      FindOrNull(unacked_packets_, sequence_number);
-  return transmission_info != NULL && transmission_info->pending;
-}
-
 void QuicUnackedPacketMap::SetNotPending(
     QuicPacketSequenceNumber sequence_number) {
   UnackedPacketMap::iterator it = unacked_packets_.find(sequence_number);
   if (it == unacked_packets_.end()) {
     LOG(DFATAL) << "SetNotPending called for packet that is not unacked: "
                 << sequence_number;
     return;
   }
   if (it->second.pending) {
     LOG_IF(DFATAL, bytes_in_flight_ < it->second.bytes_sent);
@@ skipping 94 matching lines @@
 QuicPacketSequenceNumber
 QuicUnackedPacketMap::GetLeastUnackedSentPacket() const {
   if (unacked_packets_.empty()) {
     // If there are no unacked packets, return 0.
     return 0;
   }
 
   return unacked_packets_.begin()->first;
 }
 
-SequenceNumberSet QuicUnackedPacketMap::GetUnackedPackets() const {
-  SequenceNumberSet unacked_packets;
-  for (UnackedPacketMap::const_iterator it = unacked_packets_.begin();
-       it != unacked_packets_.end(); ++it) {
-    unacked_packets.insert(it->first);
-  }
-  return unacked_packets;
-}
-
 void QuicUnackedPacketMap::SetSent(QuicPacketSequenceNumber sequence_number,
                                    QuicTime sent_time,
                                    QuicByteCount bytes_sent,
                                    bool set_pending) {
   DCHECK_LT(0u, sequence_number);
   UnackedPacketMap::iterator it = unacked_packets_.find(sequence_number);
   if (it == unacked_packets_.end()) {
     LOG(DFATAL) << "OnPacketSent called for packet that is not unacked: "
                 << sequence_number;
     return;
   }
   DCHECK(!it->second.pending);
 
   largest_sent_packet_ = max(sequence_number, largest_sent_packet_);
   it->second.sent_time = sent_time;
   if (set_pending) {
     bytes_in_flight_ += bytes_sent;
     it->second.bytes_sent = bytes_sent;
     it->second.pending = true;
   }
 }
 
 }  // namespace net