Fix QuicSession to unbuffer data when the QuicTransportChannel reconnects

Fix QuicSession to unbuffer data when the QuicTransportChannel reconnects

The QuicWriteBlockedList needs to register outgoing QUIC
streams so that when the QuicTransportChannel becomes
unwritable and QUIC streams have buffered data, they can
send data once the QuicTransportChannel becomes writable.

Otherwise the QUIC streams will remain write blocked
after the QuicTransportChannel is writable.

BUG=

Committed: https://crrev.com/70035cae4d4cd2906a244e420830bb9f7b6e3391
Cr-Commit-Position: refs/heads/master@{#12573}

[mikescarlett, 2016-04-14 23:09:10]

Description was changed from

==========
Fix QuicSession to add outgoing QUIC streams to the QuicWriteBlockedList

The QuicWriteBlockedList needs to register outgoing QUIC streams so that
when the QuicTransportChannel becomes unwritable, they can send data
once the QuicTransportChannel is writable.

Otherwise the QUIC streams will remain write blocked even after the
QuicTransportChannel is writable.

BUG=
==========

to

==========
Fix QuicSession to unbuffer data when the QuicTransportChannel reconnects

The QuicWriteBlockedList needs to register outgoing QUIC
streams so that when the QuicTransportChannel becomes
unwritable and QUIC streams have buffered data, they can
send data once the QuicTransportChannel becomes writable.

Otherwise the QUIC streams will remain write blocked
after the QuicTransportChannel is writable.

BUG=
==========

[mikescarlett, 2016-04-14 23:09:11]

mikescarlett@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, honghaiz@webrtc.org, pthatcher@webrtc.org

[honghaiz3, 2016-04-18 16:30:22]

On 2016/04/14 23:09:11, mikescarlett wrote:
> mailto:mikescarlett@webrtc.org changed reviewers:
> + mailto:deadbeef@webrtc.org, mailto:honghaiz@webrtc.org,
mailto:pthatcher@webrtc.org
It would be cleaner if you remove all old patches that were not needed for
reviewing (unless it contains some important information).

[mikescarlett, 2016-04-18 17:14:42]

Patchset #1 (id:1) has been deleted

[mikescarlett, 2016-04-18 17:14:51]

Patchset #1 (id:20001) has been deleted

[mikescarlett, 2016-04-18 17:14:58]

Patchset #1 (id:40001) has been deleted

[mikescarlett, 2016-04-18 17:15:04]

Patchset #1 (id:60001) has been deleted

[mikescarlett, 2016-04-18 17:15:09]

Patchset #1 (id:80001) has been deleted

[mikescarlett, 2016-04-18 17:15:16]

Patchset #1 (id:100001) has been deleted

[mikescarlett, 2016-04-18 17:15:22]

Patchset #1 (id:120001) has been deleted

[mikescarlett, 2016-04-18 17:23:29]

On 2016/04/18 16:30:22, honghaiz3 wrote:
> On 2016/04/14 23:09:11, mikescarlett wrote:
> > mailto:mikescarlett@webrtc.org changed reviewers:
> > + mailto:deadbeef@webrtc.org, mailto:honghaiz@webrtc.org,
> mailto:pthatcher@webrtc.org
> It would be cleaner if you remove all old patches that were not needed for
> reviewing (unless it contains some important information).

Fixed.

[honghaiz3, 2016-04-18 17:37:03]

lgtm

[pthatcher1, 2016-04-29 20:15:47]

lgtm

Just nits.  Go ahead and commit.

https://codereview.webrtc.org/1888903002/diff/140001/webrtc/p2p/quic/quicsess...
File webrtc/p2p/quic/quicsession.cc (right):

https://codereview.webrtc.org/1888903002/diff/140001/webrtc/p2p/quic/quicsess...
webrtc/p2p/quic/quicsession.cc:71: if (!IsIncomingStream(stream_id)) {
What happens if we call write_blocked_streams()->UnregisterStream(stream_id);
without this check and it's not in the write_blocked_streams()?  Does it blow
up?  Or does it just ignore it?  If it just ignores it, it might not be worth
doing the check.

https://codereview.webrtc.org/1888903002/diff/140001/webrtc/p2p/quic/quictran...
File webrtc/p2p/quic/quictransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1888903002/diff/140001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransportchannel_unittest.cc:209: return
num_incoming_quic_streams_;
I'd prefer incoming_stream_count() and incoming_stream_count_.

https://codereview.webrtc.org/1888903002/diff/140001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransportchannel_unittest.cc:536: ASSERT_NE(nullptr,
stream4);
Why do we need 4 streams for this?  Isn't 1 or 2 enough?

[mikescarlett, 2016-04-29 23:25:59]

The CQ bit was checked by mikescarlett@webrtc.org

[mikescarlett, 2016-04-29 23:26:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, honghaiz@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1888903002/#ps160001 (title:
"Respond to pthatcher comments")

[commit-bot: I haz the power, 2016-04-29 23:26:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1888903002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1888903002/160001

[mikescarlett, 2016-04-29 23:29:50]

https://codereview.webrtc.org/1888903002/diff/140001/webrtc/p2p/quic/quicsess...
File webrtc/p2p/quic/quicsession.cc (right):

https://codereview.webrtc.org/1888903002/diff/140001/webrtc/p2p/quic/quicsess...
webrtc/p2p/quic/quicsession.cc:71: if (!IsIncomingStream(stream_id)) {
On 2016/04/29 20:15:46, pthatcher1 wrote:
> What happens if we call write_blocked_streams()->UnregisterStream(stream_id);
> without this check and it's not in the write_blocked_streams()?  Does it blow
> up?  Or does it just ignore it?  If it just ignores it, it might not be worth
> doing the check.

write_blocked_streams()->UnregisterStream(stream_id) fails if the stream is not
in write_blocked_streams().

I had to change this anyway because incoming streams have to be registered also,
apparently, or else QUIC will crash.

https://codereview.webrtc.org/1888903002/diff/140001/webrtc/p2p/quic/quictran...
File webrtc/p2p/quic/quictransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1888903002/diff/140001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransportchannel_unittest.cc:209: return
num_incoming_quic_streams_;
On 2016/04/29 20:15:46, pthatcher1 wrote:
> I'd prefer incoming_stream_count() and incoming_stream_count_.

Done.

https://codereview.webrtc.org/1888903002/diff/140001/webrtc/p2p/quic/quictran...
webrtc/p2p/quic/quictransportchannel_unittest.cc:536: ASSERT_NE(nullptr,
stream4);
On 2016/04/29 20:15:47, pthatcher1 wrote:
> Why do we need 4 streams for this?  Isn't 1 or 2 enough?

2 is fine. I agree 4 is redundant but wanted to test that >1 stream can be
queued.

[commit-bot: I haz the power, 2016-04-30 01:14:41]

Description was changed from

==========
Fix QuicSession to unbuffer data when the QuicTransportChannel reconnects

The QuicWriteBlockedList needs to register outgoing QUIC
streams so that when the QuicTransportChannel becomes
unwritable and QUIC streams have buffered data, they can
send data once the QuicTransportChannel becomes writable.

Otherwise the QUIC streams will remain write blocked
after the QuicTransportChannel is writable.

BUG=
==========

to

==========
Fix QuicSession to unbuffer data when the QuicTransportChannel reconnects

The QuicWriteBlockedList needs to register outgoing QUIC
streams so that when the QuicTransportChannel becomes
unwritable and QUIC streams have buffered data, they can
send data once the QuicTransportChannel becomes writable.

Otherwise the QUIC streams will remain write blocked
after the QuicTransportChannel is writable.

BUG=
==========

[commit-bot: I haz the power, 2016-04-30 01:14:42]

Committed patchset #2 (id:160001)

[commit-bot: I haz the power, 2016-05-01 22:02:15]

Description was changed from

==========
Fix QuicSession to unbuffer data when the QuicTransportChannel reconnects

The QuicWriteBlockedList needs to register outgoing QUIC
streams so that when the QuicTransportChannel becomes
unwritable and QUIC streams have buffered data, they can
send data once the QuicTransportChannel becomes writable.

Otherwise the QUIC streams will remain write blocked
after the QuicTransportChannel is writable.

BUG=
==========

to

==========
Fix QuicSession to unbuffer data when the QuicTransportChannel reconnects

The QuicWriteBlockedList needs to register outgoing QUIC
streams so that when the QuicTransportChannel becomes
unwritable and QUIC streams have buffered data, they can
send data once the QuicTransportChannel becomes writable.

Otherwise the QUIC streams will remain write blocked
after the QuicTransportChannel is writable.

BUG=

Committed: https://crrev.com/70035cae4d4cd2906a244e420830bb9f7b6e3391
Cr-Commit-Position: refs/heads/master@{#12573}
==========

[commit-bot: I haz the power, 2016-05-01 22:02:16]

Patchset 2 (id:??) landed as
https://crrev.com/70035cae4d4cd2906a244e420830bb9f7b6e3391
Cr-Commit-Position: refs/heads/master@{#12573}