Land Recent QUIC Changes.

Land Recent QUIC Changes.

Fix a flaky EndToEndTest where packet loss is possible with a large
buffer that may overrun.

Merge internal change: 66821654
https://codereview.chromium.org/280383003/


QUIC change to invoke SetNotPending for retransmissions in
MarkForRetransmission and a minor cleanup in UnackedPacketMap.

Merge internal change: 66729074
https://codereview.chromium.org/278823005/


Remove unused IsHandshake argument to QuicConnection::OnCanWrite.

Merge internal change: 66652078
https://codereview.chromium.org/286563003/


Revert of Fix a bug where if a crypto packet is spuriously retransmitted
and thenneutered, it remains in missing_packets until the end of the
connection.

Reverted chromium cl: https://codereview.chromium.org/270213002/

Rollback of Merge internal change: 66262890.

*** Reason for rollback ***

 This appears to have caused a major QUIC loadtest regression on
 internal server.

Additionally, it's wrong in some other subtle ways, so rolling it back
and trying a new approach.

*** Original change description ***

 Fix a bug where if a crypto packet is spuriously retransmitted and then
 neutered, it remains in missing_packets until the end of the
 connection.

***

Merge internal change: 66615668
https://codereview.chromium.org/287583002/


Revert of Minor QUIC cleanup to combine QuicUnackedPacketMap's
RemovePacket and NeuterPacket into RemoveOrNeuterPacket.

Rollback of merge internal change: 66306887
Rollback of Chromium's CL https://codereview.chromium.org/271443008/

*** Reason for rollback ***

 After further issues with the QUIC handshake packets, rolling back this
 change in favor of a different approach.

*** Original change description ***

 Minor QUIC cleanup to combine QuicUnackedPacketMap's RemovePacket and
 NeuterPacket into RemoveOrNeuterPacket.

 This prevents potential bugs where a packet can be set not pending, then
 neuter'd, which cuases the packet to never be removed from the unacked
 packet map.

***

Merge internal change: 66573334
https://codereview.chromium.org/280753002/


Merging end_to_end_test.cc with the internal source tree.

+ Fixed comments (MB for megabytes and 256KB per sec instead of 1Mbit).
+ Made LargePostWithPacketLossAndBlockedSocket and
  LargePostNoPacketLossWithDelayAndReordering to be in the same
  order as the internal source tree.

Merge internal change: 66556306
https://codereview.chromium.org/283683002/


Allows QUIC connections to remain established even though the peer's
port has changed. Should make things a bit better for clients
experiencing NAT rebinding while talking QUIC to internal server.

QUIC connection port migration. Protected behind
FLAGS_quic_allow_port_migration

Merge internal change: 66523001
https://codereview.chromium.org/279453004/


Remove unnecessary methods from QuicUnackedPacketMap and a minor
QuicSentPacketManagerTest fix as a result.

Merge internal change: 66427453
https://codereview.chromium.org/287443003/

R=rch@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270046

[Ryan Hamilton, 2014-05-13 04:26:44]

lgtm

[ramant (doing other things), 2014-05-13 06:18:33]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-05-13 06:18:46]

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

[commit-bot: I haz the power, 2014-05-13 07:44:27]

Change committed as 270046

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

Patch set 1 LGTM.

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_sent_packet_ma...
File net/quic/quic_sent_packet_manager.cc (right):

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_sent_packet_ma...
net/quic/quic_sent_packet_manager.cc:255: while (unacked_it !=
unacked_packets_.end()) {

Nit: in the new code, this loop can be a for loop because it updates
|unacked_it| in only one way. (The old code updated |unacked_it| in two ways, so
it had to be a while loop.)

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_sent_packet_ma...
net/quic/quic_sent_packet_manager.cc:259: // Since once you're forward secure,
no unencrypted packets will be sent,

Nit: "Since" and "once" sound a little redundant. It seems that "once" is enough
(as in the original comment).

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_unacked_packet...
File net/quic/quic_unacked_packet_map.cc (right):

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_unacked_packet...
net/quic/quic_unacked_packet_map.cc:134: delete
transmission_info->all_transmissions;

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

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_unacked_packet...
net/quic/quic_unacked_packet_map.cc:149: if
(transmission_info->all_transmissions->size() > 1) {

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

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_unacked_packet...
net/quic/quic_unacked_packet_map.cc:151: transmission_info->all_transmissions =
new SequenceNumberSet();

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

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_unacked_packet...
File net/quic/quic_unacked_packet_map.h (right):

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_unacked_packet...
net/quic/quic_unacked_packet_map.h:122: // frames, and sets all_transmissions to
only include itself.

1. Unless we expect readers of this code to know all_transmissions is a member
of the TransmissionInfo struct, it would be nice to clarify that.

2. It's not clear what "itself" refers to. The specified packet or
all_transmissions?

3. Nit: If I only read the comments here, I don't really know the difference
between RemovePacket and NeuterPacket and hence don't know when I should use
which method. Unfortunately I can't suggest a way to clarify this.

https://codereview.chromium.org/283693002/diff/1/net/tools/quic/end_to_end_te...
File net/tools/quic/end_to_end_test.cc (right):

https://codereview.chromium.org/283693002/diff/1/net/tools/quic/end_to_end_te...
net/tools/quic/end_to_end_test.cc:663: // 256KB per second with a 256k buffer
from server to client.  Wireless

Nit: 256k => 256KB ?

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

Comments added WanTeh.  The current code is quite confusing.  This CL is a
rollback of a previous effort to clean it up some, but it ended up being more
complex in the end.

The hope is to improve it and make the contracts clearer in the near future.

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_unacked_packet...
File net/quic/quic_unacked_packet_map.cc (right):

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_unacked_packet...
net/quic/quic_unacked_packet_map.cc:134: delete
transmission_info->all_transmissions;
On 2014/05/13 18:49:23, wtc wrote:
> 
> IMPORTANT: should we set transmission_info->all_transmissions to NULL to avoid
a
> dangling pointer?

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

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_unacked_packet...
net/quic/quic_unacked_packet_map.cc:149: if
(transmission_info->all_transmissions->size() > 1) {
On 2014/05/13 18:49:23, wtc wrote:
> 
> Can you explain why we only do the following when
> transmission_info->all_transmissions has more than one element?

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.

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_unacked_packet...
net/quic/quic_unacked_packet_map.cc:151: transmission_info->all_transmissions =
new SequenceNumberSet();
On 2014/05/13 18:49:23, wtc wrote:
> 
> IMPORTANT: do we leak the old value of transmission_info->all_transmissions
> here?

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.

[ramant (doing other things), 2014-05-14 05:33:29]

Made the changes in https://codereview.chromium.org/286693003/

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_sent_packet_ma...
File net/quic/quic_sent_packet_manager.cc (right):

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_sent_packet_ma...
net/quic/quic_sent_packet_manager.cc:255: while (unacked_it !=
unacked_packets_.end()) {
On 2014/05/13 18:49:23, wtc wrote:
> 
> Nit: in the new code, this loop can be a for loop because it updates
> |unacked_it| in only one way. (The old code updated |unacked_it| in two ways,
so
> it had to be a while loop.)

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

Done.

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_sent_packet_ma...
net/quic/quic_sent_packet_manager.cc:259: // Since once you're forward secure,
no unencrypted packets will be sent,
On 2014/05/13 18:49:23, wtc wrote:
> 
> Nit: "Since" and "once" sound a little redundant. It seems that "once" is
enough
> (as in the original comment).

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

Done.

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_unacked_packet...
File net/quic/quic_unacked_packet_map.h (right):

https://codereview.chromium.org/283693002/diff/1/net/quic/quic_unacked_packet...
net/quic/quic_unacked_packet_map.h:122: // frames, and sets all_transmissions to
only include itself.
On 2014/05/13 18:49:23, wtc wrote:
> 
> 1. Unless we expect readers of this code to know all_transmissions is a member
> of the TransmissionInfo struct, it would be nice to clarify that.
> 
> 2. It's not clear what "itself" refers to. The specified packet or
> all_transmissions?
> 
> 3. Nit: If I only read the comments here, I don't really know the difference
> between RemovePacket and NeuterPacket and hence don't know when I should use
> which method. Unfortunately I can't suggest a way to clarify this.

ianswett@: Would like to work with you to make this change in internal source
tree and then merge into chromium.

https://codereview.chromium.org/283693002/diff/1/net/tools/quic/end_to_end_te...
File net/tools/quic/end_to_end_test.cc (right):

https://codereview.chromium.org/283693002/diff/1/net/tools/quic/end_to_end_te...
net/tools/quic/end_to_end_test.cc:663: // 256KB per second with a 256k buffer
from server to client.  Wireless
On 2014/05/13 18:49:23, wtc wrote:
> 
> Nit: 256k => 256KB ?

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

Done.