Add an async ReadTrailers method to QuicChromiumClientStream::Handle

Add an async ReadTrailers method to QuicChromiumClientStream::Handle

This does not change OnClose or OnError and the callback is not yet invoked
on errors, but that can happen in the next CL since all upcalls have now
been replaced by async methods.

BUG=716563
Review-Url: https://codereview.chromium.org/2900533002
Cr-Commit-Position: refs/heads/master@{#475517}
Committed: https://chromium.googlesource.com/chromium/src/+/ce2464110221db485c26fd738c097c95c2269c13

[Ryan Hamilton, 2017-05-20 22:16:39]

rch@chromium.org changed reviewers:
+ xunjieli@chromium.org

[xunjieli, 2017-05-26 15:53:06]

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

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.cc:306:
weak_factory_.GetWeakPtr()));
Should we post this task before calling into the |delegate_| ?
If |delegate_| destroys |this| during the callback, then we will have a
segmentation fault when dereferencing |weak_factory_|, right?

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

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream_test.cc:504:
base::RunLoop().RunUntilIdle();
Is "base::RunLoop().RunUntilIdle()" needed here?

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

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:468: if (!stream_)
Is the null check needed? I thought the handle is never null-ed.

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:476: OnReadTrailingHeadersComplete(rv);
Should we test (rv != ERR_IO_PENDING) before calling
OnReadTrailingHeadersComplete(rv) here?

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream.h (right):

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.h:188: bool trailing_headers_received_;
nit: we need to default initialize this boolean.

[Ryan Hamilton, 2017-05-26 22:35:50]

Thanks!

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

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.cc:306:
weak_factory_.GetWeakPtr()));
On 2017/05/26 15:53:05, xunjieli wrote:
> Should we post this task before calling into the |delegate_| ?
> If |delegate_| destroys |this| during the callback, then we will have a
> segmentation fault when dereferencing |weak_factory_|, right?

Indeed! I noticed this in a followup, but forgot to upload the new patch set. :(

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

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream_test.cc:504:
base::RunLoop().RunUntilIdle();
On 2017/05/26 15:53:05, xunjieli wrote:
> Is "base::RunLoop().RunUntilIdle()" needed here?

Done.

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

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:468: if (!stream_)
On 2017/05/26 15:53:06, xunjieli wrote:
> Is the null check needed? I thought the handle is never null-ed.

It's still null out in ResetStream, as of this CL. But in the follow up I remove
OnClose and OnError and no longer null it out. Stay tuned! :)

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:476: OnReadTrailingHeadersComplete(rv);
On 2017/05/26 15:53:05, xunjieli wrote:
> Should we test (rv != ERR_IO_PENDING) before calling
> OnReadTrailingHeadersComplete(rv) here?

Good point. Amusingly, OnReadTrailingHeadersComplete is a no-op if rv ==
ERR_IO_PENDING, so there's no "bug" currently, but yeah, it's totally wrong!

Done.

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream.h (right):

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.h:188: bool trailing_headers_received_;
On 2017/05/26 15:53:06, xunjieli wrote:
> nit: we need to default initialize this boolean.

Done.

[xunjieli, 2017-05-29 14:06:08]

LGTM!

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

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream.cc:468: if (!stream_)
On 2017/05/26 22:35:50, Ryan Hamilton wrote:
> On 2017/05/26 15:53:06, xunjieli wrote:
> > Is the null check needed? I thought the handle is never null-ed.
> 
> It's still null out in ResetStream, as of this CL. But in the follow up I
remove
> OnClose and OnError and no longer null it out. Stay tuned! :)

Acknowledged.

https://codereview.chromium.org/2900533002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream.h (right):

https://codereview.chromium.org/2900533002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.h:299: size_t
trailing_headers_frame_len_;
nit: need to default initialize this size_t in QuicChromiumClientStream
constructor.

https://codereview.chromium.org/2900533002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream_test.cc (right):

https://codereview.chromium.org/2900533002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream_test.cc:504:
base::RunLoop().RunUntilIdle();
nit: I think we can remove "base::RunLoop().RunUntilIdle()" here. Or is it
needed for some other reason?

[Ryan Hamilton, 2017-05-29 16:27:23]

Thanks!

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

https://codereview.chromium.org/2900533002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream_test.cc:504:
base::RunLoop().RunUntilIdle();
On 2017/05/26 22:35:50, Ryan Hamilton wrote:
> On 2017/05/26 15:53:05, xunjieli wrote:
> > Is "base::RunLoop().RunUntilIdle()" needed here?
> 
> Done.

Oh, whoops. Actually, this is needed. It's needed because the
ReadTraileringHeaders posts a task for the OnDataAvailable notification and we
need to wait for it to be delivered. I've added this line back.

Alternatively, we would expect ERR_IO_PENDING from ReadBody below. Do you have a
preference?

https://codereview.chromium.org/2900533002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream.h (right):

https://codereview.chromium.org/2900533002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.h:299: size_t
trailing_headers_frame_len_;
On 2017/05/29 14:06:08, xunjieli wrote:
> nit: need to default initialize this size_t in QuicChromiumClientStream
> constructor.

Done.

https://codereview.chromium.org/2900533002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_stream_test.cc (right):

https://codereview.chromium.org/2900533002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream_test.cc:504:
base::RunLoop().RunUntilIdle();
On 2017/05/29 14:06:08, xunjieli wrote:
> nit: I think we can remove "base::RunLoop().RunUntilIdle()" here. Or is it
> needed for some other reason?

Whoops, meant to reply over the weekend, but forgot to hit send.

[Ryan Hamilton, 2017-05-29 17:04:28]

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

[commit-bot: I haz the power, 2017-05-29 17:04:35]

Dry run: 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, 2017-05-29 18:31:02]

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

[commit-bot: I haz the power, 2017-05-29 18:31:03]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[xunjieli, 2017-05-29 19:39:02]

You should be able to remove "base::RunLoop().RunUntilIdle()" once
MarkTrailersConsumed() is moved to DeliverTrailingHeaders(). Let me know if that
doesn't fix it :)

https://codereview.chromium.org/2900533002/diff/100001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_stream.cc (right):

https://codereview.chromium.org/2900533002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:556: MarkTrailersConsumed();
Sorry I haven't realized earlier. Can we move "MarkTrailersConsumed()" to
DeliverTrailingHeaders() where DeliverTrailingHeaders() returned true? 
|trailers_consumed_| should only be set when received_trailers() have been read
and copied.

https://codereview.chromium.org/2900533002/diff/100001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_stream_test.cc (right):

https://codereview.chromium.org/2900533002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream_test.cc:504:
base::RunLoop().RunUntilIdle();
You should be able to remove "base::RunLoop().RunUntilIdle()" once
MarkTrailersConsumed() is moved to DeliverTrailingHeaders(). Let me know if that
doesn't fix it.

https://codereview.chromium.org/2900533002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream_test.cc:601:
handle_->ReadTrailingHeaders(&delegate_.trailers_, callback.callback()));
Sorry for not seeing this earlier. 
Can we use a different TestCompletionCallback here for ReadTrailingHeaders()?
callback.WaitForResult() on the next line is checking the read callback.

[Ryan Hamilton, 2017-05-29 20:29:57]

https://codereview.chromium.org/2900533002/diff/100001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_stream.cc (right):

https://codereview.chromium.org/2900533002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:556: MarkTrailersConsumed();
On 2017/05/29 19:39:01, xunjieli wrote:
> Sorry I haven't realized earlier. Can we move "MarkTrailersConsumed()" to
> DeliverTrailingHeaders() where DeliverTrailingHeaders() returned true? 
> |trailers_consumed_| should only be set when received_trailers() have been
read
> and copied.

Ah, good point. Done.

https://codereview.chromium.org/2900533002/diff/100001/net/quic/chromium/quic...
File net/quic/chromium/quic_chromium_client_stream_test.cc (right):

https://codereview.chromium.org/2900533002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream_test.cc:504:
base::RunLoop().RunUntilIdle();
On 2017/05/29 19:39:01, xunjieli wrote:
> You should be able to remove "base::RunLoop().RunUntilIdle()" once
> MarkTrailersConsumed() is moved to DeliverTrailingHeaders(). Let me know if
that
> doesn't fix it.

Nice! Done.

https://codereview.chromium.org/2900533002/diff/100001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream_test.cc:601:
handle_->ReadTrailingHeaders(&delegate_.trailers_, callback.callback()));
On 2017/05/29 19:39:01, xunjieli wrote:
> Sorry for not seeing this earlier. 
> Can we use a different TestCompletionCallback here for ReadTrailingHeaders()?
> callback.WaitForResult() on the next line is checking the read callback. 

Done.

[Ryan Hamilton, 2017-05-29 20:30:44]

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

[commit-bot: I haz the power, 2017-05-29 20:30:56]

Dry run: 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, 2017-05-29 21:51:33]

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

[commit-bot: I haz the power, 2017-05-29 21:51:34]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2017-05-30 13:19:53]

lgtm!

[Ryan Hamilton, 2017-05-30 13:49:10]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2017-05-30 13:49:19]

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, 2017-05-30 13:51:53]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496152150037150,
"parent_rev": "18addc1ee2e63344ce882005e48c2e509a7402b5", "commit_rev":
"ce2464110221db485c26fd738c097c95c2269c13"}

[commit-bot: I haz the power, 2017-05-30 13:52:02]

Description was changed from

==========
Add an async ReadTrailers method to QuicChromiumClientStream::Handle

This does not change OnClose or OnError and the callback is not yet invoked
on errors, but that can happen in the next CL since all upcalls have now
been replaced by async methods.

BUG=716563
==========

to

==========
Add an async ReadTrailers method to QuicChromiumClientStream::Handle

This does not change OnClose or OnError and the callback is not yet invoked
on errors, but that can happen in the next CL since all upcalls have now
been replaced by async methods.

BUG=716563

Review-Url: https://codereview.chromium.org/2900533002
Cr-Commit-Position: refs/heads/master@{#475517}
Committed:
https://chromium.googlesource.com/chromium/src/+/ce2464110221db485c26fd738c09...
==========

[commit-bot: I haz the power, 2017-05-30 13:52:03]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ce2464110221db485c26fd738c09...