Repair two DCHECKS

net/quic/quic_sent_packet_manager.cc

 // Copyright 2013 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_sent_packet_manager.h"
 
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "net/quic/congestion_control/pacing_sender.h"
 #include "net/quic/quic_ack_notifier_manager.h"
@@ skipping 532 matching lines @@
   return unacked_packets;
 }
 
 bool QuicSentPacketManager::OnPacketSent(
     QuicPacketSequenceNumber sequence_number,
     QuicTime sent_time,
     QuicByteCount bytes,
     TransmissionType transmission_type,
     HasRetransmittableData has_retransmittable_data) {
   DCHECK_LT(0u, sequence_number);
-  DCHECK(ContainsKey(unacked_packets_, sequence_number));
+  // In some edge cases, on some platforms (such as Windows), it is possible
+  // that we were write-blocked when we tried to send a packet, and then decided
+  // not to send the packet (such as when the encryption key changes, and we
+  // "discard" the unsent packet).  In that rare case, we may indeed
+  // asynchronously (later) send the packet, calling this method, but the
+  // sequence number may already be erased from unacked_packets_ map. In that
+  // case, we can just return false since the packet will not be tracked for
+  // retransmission.

[Ryan Hamilton, 2014/01/10 05:22:46]

Very clear.  Awesome.

+  if (!ContainsKey(unacked_packets_, sequence_number))
+    return false;
   DCHECK(!unacked_packets_[sequence_number].pending);
-  if (has_retransmittable_data == HAS_RETRANSMITTABLE_DATA) {
-    DCHECK(unacked_packets_[sequence_number].retransmittable_frames);

[Ryan Hamilton, 2014/01/10 05:22:46]

Is this DCHECK invalid?  If so, why?

[Ryan Hamilton, 2014/01/10 05:24:25]

Oh, it's in the CL description :>

Might be worth adding that comment here so that someone else doesn't think that
such a DCHECK is a good idea, only to be confused when it fires :>

[jar (doing other things), 2014/01/10 05:36:43]

Originally, I put a big comment in the code next to the commented out DCHECK...
and then decided that the code was just wrong... shouldn't be there... and hence
shouldn't have comments about why it isn't there. <imagine how many other places
we could put such commentary :-/  >


I do think it is a good idea to try to set up a regression test that would fire
these DCHECKs, and that exercises this rather strange set of events (assuming I
have 'em right).  ...but it sure seem worth getting this into the canary, so
that other developers don't trip on the DCHECKs.

I can try doing this... and I'll file a bug against myself... but I have a
growing bunch of yaks being shaved at the moment that *seem* more crucial.

-  }
   UnackedPacketMap::iterator it = unacked_packets_.find(sequence_number);
 
   // Only track packets the send algorithm wants us to track.
   if (!send_algorithm_->OnPacketSent(sent_time, sequence_number, bytes,
                                      transmission_type,
                                      has_retransmittable_data)) {
     DCHECK(it->second.retransmittable_frames == NULL);
     unacked_packets_.erase(it);
     // Do not reset the retransmission timer, since the packet isn't tracked.
     return false;
@@ skipping 423 matching lines @@
   }
   previous_transmissions->erase(sequence_number);
   if (previous_transmissions->size() == 1) {
     QuicPacketSequenceNumber current = *previous_transmissions->begin();
     unacked_packets_[current].previous_transmissions = NULL;
     delete previous_transmissions;
   }
 }
 
 }  // namespace net