Repair two DCHECKS

Repair two DCHECKS

In one case, the packet was seemingly buffered, and then discarded, but
asynchronously sent (needlessly). We can just return, and not
track the packet since it definately won't have its contents
retransmitted.

In the other case it appears that the packet was sent, containing a
retransmission of previously sent frames.  Unfortunately that resend
was write-blocked, and while waiting, the original packet was acked,
which lead to removing the list of retransmittable frames from that
latter (write-blocked) packet.  An assertion was then failing when
the packet was sent, as it was expected to be retransmittable, but
had no retransmittable frames.

R=ian,rch
BUG=332631
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244129

[Ryan Hamilton, 2014-01-10 05:22:45]

https://chromiumcodereview.appspot.com/133243002/diff/1/net/quic/quic_sent_pa...
File net/quic/quic_sent_packet_manager.cc (left):

https://chromiumcodereview.appspot.com/133243002/diff/1/net/quic/quic_sent_pa...
net/quic/quic_sent_packet_manager.cc:556:
DCHECK(unacked_packets_[sequence_number].retransmittable_frames);
Is this DCHECK invalid?  If so, why?

https://chromiumcodereview.appspot.com/133243002/diff/1/net/quic/quic_sent_pa...
File net/quic/quic_sent_packet_manager.cc (right):

https://chromiumcodereview.appspot.com/133243002/diff/1/net/quic/quic_sent_pa...
net/quic/quic_sent_packet_manager.cc:560: // retransmission.
Very clear.  Awesome.

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

lgtm, but we really need a regression test for this.

https://chromiumcodereview.appspot.com/133243002/diff/1/net/quic/quic_sent_pa...
File net/quic/quic_sent_packet_manager.cc (left):

https://chromiumcodereview.appspot.com/133243002/diff/1/net/quic/quic_sent_pa...
net/quic/quic_sent_packet_manager.cc:556:
DCHECK(unacked_packets_[sequence_number].retransmittable_frames);
On 2014/01/10 05:22:46, Ryan Hamilton wrote:
> Is this DCHECK invalid?  If so, why?

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:42]

The try bots seem terribly horked this evening... but I'll hit the CQ and see if
I get lucky :-/.

https://chromiumcodereview.appspot.com/133243002/diff/1/net/quic/quic_sent_pa...
File net/quic/quic_sent_packet_manager.cc (left):

https://chromiumcodereview.appspot.com/133243002/diff/1/net/quic/quic_sent_pa...
net/quic/quic_sent_packet_manager.cc:556:
DCHECK(unacked_packets_[sequence_number].retransmittable_frames);
On 2014/01/10 05:24:25, Ryan Hamilton wrote:
> On 2014/01/10 05:22:46, Ryan Hamilton wrote:
> > Is this DCHECK invalid?  If so, why?
> 
> 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 :>

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.

[commit-bot: I haz the power, 2014-01-10 05:37:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jar@chromium.org/133243002/1

[commit-bot: I haz the power, 2014-01-10 08:38:27]

Change committed as 244129