Repair two DCHECKS

net/quic/quic_sent_packet_manager.cc

Index: net/quic/quic_sent_packet_manager.cc
diff --git a/net/quic/quic_sent_packet_manager.cc b/net/quic/quic_sent_packet_manager.cc
index a89c02e9cd56d5bcef1dddd832b23232b0353b52..af9a507a59c00a3ffd3328f7253be710173ecd7f 100644
--- a/net/quic/quic_sent_packet_manager.cc
+++ b/net/quic/quic_sent_packet_manager.cc
@@ -550,11 +550,17 @@ bool QuicSentPacketManager::OnPacketSent(
     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.