QuicChromiumClientStream handle Read() after trailers are received but not yet delivered

QuicChromiumClientStream handle Read() after trailers are received but not yet delivered

This CL changes QuicChromiumClientStream::Read() to return ERR_IO_PENDNG if
trailers are received but not yet delivered. This CL additionally posts a
OnDataAvailable task when trailers are delivered so as to notify delegate of the
FIN flag.

BUG=652935
Committed: https://crrev.com/ec0ed02ab197a731d3dcccdd7379c71a057353a1
Cr-Commit-Position: refs/heads/master@{#424183}

[xunjieli, 2016-10-07 17:10:29]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-10-07 17:11:24]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 17:11:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2016-10-07 17:15:34]

xunjieli@chromium.org changed reviewers:
+ mef@chromium.org, mpw@chromium.org, zhongyi@chromium.org

[xunjieli, 2016-10-07 17:15:35]

mpw, zhongyi, mef: PTAL.

https://codereview.chromium.org/2405523002/diff/20001/net/quic/chromium/bidir...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (left):

https://codereview.chromium.org/2405523002/diff/20001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.cc:244: if
(stream_->IsDoneReading()) {
This block is removed because now we always have OnDataAvailable() after
OnHeadersAvailable(trailers), and we can do OnFinRead() in OnDataAvailable().

We can't do OnFinRead() here because it will invoke OnClose() which will reset
QuicChromiumClientStream::|delegate_| so we won't get OnDataAvailable().

https://codereview.chromium.org/2405523002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream.cc (left):

https://codereview.chromium.org/2405523002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:252: if
(sequencer()->IsClosed())
This is changed to IsDoneReading() because before returning 0 to caller, we also
want to check if trailers are consumed.

[mef, 2016-10-07 17:28:31]

https://codereview.chromium.org/2405523002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream.cc (right):

https://codereview.chromium.org/2405523002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:264: if (bytes_read == 0 &&
!FinishedReadingTrailers())
What if there are no trailers?

[xunjieli, 2016-10-07 17:39:00]

https://codereview.chromium.org/2405523002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream.cc (right):

https://codereview.chromium.org/2405523002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:264: if (bytes_read == 0 &&
!FinishedReadingTrailers())
On 2016/10/07 17:28:31, mef wrote:
> What if there are no trailers?

FinishedReadingTrailers() will return true if there are no trailers.

[mpw, 2016-10-07 17:45:01]

lgtm

https://codereview.chromium.org/2405523002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream.cc (right):

https://codereview.chromium.org/2405523002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:264: if (bytes_read == 0 &&
!FinishedReadingTrailers())
On 2016/10/07 17:28:31, mef wrote:
> What if there are no trailers?

If there are no trailers, FinishedReadingTrailers() will return true, since
fin_received && !trailers_decompressed_.  Perhaps the method should be named
something like FinishedReadingTrailersIfAny(), though I'm not sure that's much
clearer.  At the very least, the doc comment for it should be updated--I'll do
that in a separate CL.

[commit-bot: I haz the power, 2016-10-07 18:23:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 18:23:12]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-10-10 15:21:49]

zhongyi@: a friendly ping?

[Zhongyi Shi, 2016-10-10 17:00:04]

lgtm!

[xunjieli, 2016-10-10 17:12:35]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2016-10-10 17:12:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 18:05:31]

Committed patchset #1 (id:20001)

[commit-bot: I haz the power, 2016-10-10 18:08:01]

Description was changed from

==========
QuicChromiumClientStream handle Read() after trailers are received but not yet
delivered

This CL changes QuicChromiumClientStream::Read() to return ERR_IO_PENDNG if
trailers are received but not yet delivered. This CL additionally posts a
OnDataAvailable task when trailers are delivered so as to notify delegate of the
FIN flag.

BUG=652935
==========

to

==========
QuicChromiumClientStream handle Read() after trailers are received but not yet
delivered

This CL changes QuicChromiumClientStream::Read() to return ERR_IO_PENDNG if
trailers are received but not yet delivered. This CL additionally posts a
OnDataAvailable task when trailers are delivered so as to notify delegate of the
FIN flag.

BUG=652935

Committed: https://crrev.com/ec0ed02ab197a731d3dcccdd7379c71a057353a1
Cr-Commit-Position: refs/heads/master@{#424183}
==========

[commit-bot: I haz the power, 2016-10-10 18:08:02]

Patchset 1 (id:??) landed as
https://crrev.com/ec0ed02ab197a731d3dcccdd7379c71a057353a1
Cr-Commit-Position: refs/heads/master@{#424183}