Remove QuicChromiumClientStream::Delegate in favor of async methods.

Remove QuicChromiumClientStream::Delegate in favor of async methods.

This removes OnClose and OnError from QuicHttpStream and
BidirectionalStreamQuicImpl.

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

[Ryan Hamilton, 2017-05-30 19:15:14]

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

[xunjieli, 2017-05-31 00:25:22]

Overall this change makes sense to me. This is my first pass (mostly a couple of
questions to make sure I understand).

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

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.cc:168:
weak_factory_.GetWeakPtr(), OK));
Need to pass |rv| instead of hardcoding OK here.

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.cc:201:
weak_factory_.GetWeakPtr(), OK));
Need to pass |rv| instead of hardcoding OK here.

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/bidir...
File net/quic/chromium/bidirectional_stream_quic_impl.h (right):

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.h:90: bool expect_trailers_;
Can you add a comment on what this bool does? It's not immediately obvious why
it is needed.

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

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:122:
base::ThreadTaskRunnerHandle::Get()->PostTask(
Is this PostTask necessary? 
The callback shouldn't call into Handle again if it's being called with an
error, right? If so, we should be able to get rid of the PostTask and just
invoke the callback here.

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:202: return
HandleIOComplete(stream_->WriteHeaders(std::move(header_block), fin,
Doesn't stream_->WriteHeaders() return a size_t ? Should we check that before
invoking HandleIOComplete() which accepts a net error code?

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:244:
read_headers_callback_.Reset();
Should we also reset the read trailers callback and read body callback here?

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:362:
CHECK(!may_invoke_callbacks_);
Should we add a TODO to convert this CHECK to DCHECK once the API is stable?

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:373: if (rv < 0 || stream_)
Why do we need the |stream_| check? Could you add a comment?
It's not immediately obvious because |stream_| is not being deferenced in this
function.

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

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.h:73: int
WriteHeaders(SpdyHeaderBlock header_block,
Could you add a comment here mentioning that the return value is a net error
code?

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream_test.cc (left):

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream_test.cc:1473: TEST_P(QuicHttpStreamTest,
CheckPriorityWithNoDelegate) {
Why is this test deleted?

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

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream_test.cc:1406:
ProcessPacket(ConstructResponseHeadersPacket(2, !kFin, &response_size));
Why is kFin changed to !kFin?

[Ryan Hamilton, 2017-05-31 02:49:56]

Thanks for the comments! As usual, you've keyed in on some thorny issues.

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

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.cc:168:
weak_factory_.GetWeakPtr(), OK));
On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> Need to pass |rv| instead of hardcoding OK here. 

I could do that but OnSendDataComplete has:

  DCHECK(rv == OK || !stream_);

Would you prefer I change == OK to >= 0?

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.cc:201:
weak_factory_.GetWeakPtr(), OK));
On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> Need to pass |rv| instead of hardcoding OK here. 

Ditto. Happy to do whatever you prefer.

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/bidir...
File net/quic/chromium/bidirectional_stream_quic_impl.h (right):

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.h:90: bool expect_trailers_;
On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> Can you add a comment on what this bool does? It's not immediately obvious why
> it is needed.

Done.

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

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:122:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> Is this PostTask necessary? 
> The callback shouldn't call into Handle again if it's being called with an
> error, right? If so, we should be able to get rid of the PostTask and just
> invoke the callback here.

The problem is not that the caller might call into the handle again, but that
the handle might call into the caller when the caller is still on the stack. I
did not have a PostTask here until I started working on a CL to bundle POST
headers together with body by use of a scoped packet bundler. In this instance,
we can do:

DoPost() {
// do stuff
{
 ScopedPacketBundler;
 handle_->WriteHeaders() => OK
 handle_->WriteData(callback) => ERR_IO_PENDING
}
// do more stuff
}

In this case, when the bundler goes out of scope, it can cause the write to
flush and the connection to close because of a write error. This write error
then triggers the callbacks to be executed. But this violates the reentrancy
guarantee that the callback will not be executed while under the callstack of
the caller. 

This is super annoying. Buck and I spent quite a while talking through this and
this was the best we came up with. Happy to do something better though!

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:202: return
HandleIOComplete(stream_->WriteHeaders(std::move(header_block), fin,
On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> Doesn't stream_->WriteHeaders() return a size_t ? Should we check that before
> invoking HandleIOComplete() which accepts a net error code?

What should we check?

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:244:
read_headers_callback_.Reset();
On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> Should we also reset the read trailers callback and read body callback here?

There is no read trailers callback, only read_headers_callback_ which is set
when ReadInitialHeaders() is called and when ReadTrailingHeaders() is called.
(You can't read trailers until you've read initial headers, of course)

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:362:
CHECK(!may_invoke_callbacks_);
On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> Should we add a TODO to convert this CHECK to DCHECK once the API is stable?

Done.

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:373: if (rv < 0 || stream_)
On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> Why do we need the |stream_| check? Could you add a comment?
> It's not immediately obvious because |stream_| is not being deferenced in this
> function.

If |stream_| is still valid the stream has not been closed. if the stream has
not been closed, then we don't need to return the close error code. Does that
make sense?

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

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.h:73: int
WriteHeaders(SpdyHeaderBlock header_block,
On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> Could you add a comment here mentioning that the return value is a net error
> code?

Done.

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream_test.cc (left):

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream_test.cc:1473: TEST_P(QuicHttpStreamTest,
CheckPriorityWithNoDelegate) {
On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> Why is this test deleted?

Ah, because this test is all about what happens when a stream has no delegate.
But there is no such thing as a delegate any longer, so this test has become
invalid.

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

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream_test.cc:1406:
ProcessPacket(ConstructResponseHeadersPacket(2, !kFin, &response_size));
On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> Why is kFin changed to !kFin?

Ah! Good question :) So with the new code, if kFin is set, then when the HEADERS
are received, the stream becomes closed the "normal" way (fin sent and
received). As a result, the write on line 1383 does not happen (because the
stream is not reset). By using !kFin, the stream does not close normally and
instead it's the callback (of QuicHttpStreamTest::CloseStream) which actually
closes the stream.

I believe the old code passes because of buggy close handling in QuicHttpStream
which results in the stream being reset even though it is already closed. I can
make this change in a stand-alone CL if you'd prefer?

[Ryan Hamilton, 2017-05-31 02:51:40]

Patchset #6 (id:100001) has been deleted

[Ryan Hamilton, 2017-05-31 14:18:26]

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

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.cc:168:
weak_factory_.GetWeakPtr(), OK));
On 2017/05/31 02:49:55, Ryan Hamilton wrote:
> On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> > Need to pass |rv| instead of hardcoding OK here. 
> 
> I could do that but OnSendDataComplete has:
> 
>   DCHECK(rv == OK || !stream_);
> 
> Would you prefer I change == OK to >= 0?

Oh. I confused myself. Fixed to pass |rv| and cleaned up OnSendDataComplete()

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/bidir...
net/quic/chromium/bidirectional_stream_quic_impl.cc:201:
weak_factory_.GetWeakPtr(), OK));
On 2017/05/31 02:49:55, Ryan Hamilton wrote:
> On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> > Need to pass |rv| instead of hardcoding OK here. 
> 
> Ditto. Happy to do whatever you prefer.

Done.

[xunjieli, 2017-05-31 18:47:48]

I am going to hold off this for a bit. Cronet is having seg faults when
accessing the handle object. b/62212721
Ryan, any thought? If we figure out a fix, we need to merge it back to M60.

[xunjieli, 2017-06-01 15:30:33]

Thanks for bearing with me. I think I am ready to sign off after one more pass.

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

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:122:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2017/05/31 02:49:56, Ryan Hamilton wrote:
> On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> > Is this PostTask necessary? 
> > The callback shouldn't call into Handle again if it's being called with an
> > error, right? If so, we should be able to get rid of the PostTask and just
> > invoke the callback here.
> 
> The problem is not that the caller might call into the handle again, but that
> the handle might call into the caller when the caller is still on the stack. I
> did not have a PostTask here until I started working on a CL to bundle POST
> headers together with body by use of a scoped packet bundler. In this
instance,
> we can do:
> 
> DoPost() {
> // do stuff
> {
>  ScopedPacketBundler;
>  handle_->WriteHeaders() => OK
>  handle_->WriteData(callback) => ERR_IO_PENDING
> }
> // do more stuff
> }
> 
> In this case, when the bundler goes out of scope, it can cause the write to
> flush and the connection to close because of a write error. This write error
> then triggers the callbacks to be executed. But this violates the reentrancy
> guarantee that the callback will not be executed while under the callstack of
> the caller. 
> 
> This is super annoying. Buck and I spent quite a while talking through this
and
> this was the best we came up with. Happy to do something better though!

Acknowledged. You are totally right. I missed the part where Handle::OnClose()
can be called re-entrantly.

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_stream.cc:244:
read_headers_callback_.Reset();
On 2017/05/31 02:49:56, Ryan Hamilton wrote:
> On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> > Should we also reset the read trailers callback and read body callback here?
> 
> There is no read trailers callback, only read_headers_callback_ which is set
> when ReadInitialHeaders() is called and when ReadTrailingHeaders() is called.
> (You can't read trailers until you've read initial headers, of course)

Got it. Thanks for the details!

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_http_stream_test.cc (left):

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream_test.cc:1473: TEST_P(QuicHttpStreamTest,
CheckPriorityWithNoDelegate) {
On 2017/05/31 02:49:56, Ryan Hamilton wrote:
> On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> > Why is this test deleted?
> 
> Ah, because this test is all about what happens when a stream has no delegate.
> But there is no such thing as a delegate any longer, so this test has become
> invalid.

Acknowledged.

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

https://codereview.chromium.org/2908243002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_http_stream_test.cc:1406:
ProcessPacket(ConstructResponseHeadersPacket(2, !kFin, &response_size));
On 2017/05/31 02:49:56, Ryan Hamilton wrote:
> On 2017/05/31 00:25:22, xunjieli (OOO.until.6-5) wrote:
> > Why is kFin changed to !kFin?
> 
> Ah! Good question :) So with the new code, if kFin is set, then when the
HEADERS
> are received, the stream becomes closed the "normal" way (fin sent and
> received). As a result, the write on line 1383 does not happen (because the
> stream is not reset). By using !kFin, the stream does not close normally and
> instead it's the callback (of QuicHttpStreamTest::CloseStream) which actually
> closes the stream.
> 
> I believe the old code passes because of buggy close handling in
QuicHttpStream
> which results in the stream being reset even though it is already closed. I
can
> make this change in a stand-alone CL if you'd prefer?

Acknowledged.

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.cc:110: NotifyError(rv);
When WriteHeaders() is called synchronously in SendData()/SendvData(), we should
use a PostTask to NotifyError(). 

How about we change signature of WriteHeaders to return a net_error code? so the
caller of WriteHeaders() can choose whether to handle error asynchronously or
synchronously?

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.cc:313: if (expect_trailers_)
Is this needed? The second time NotifyError() is called, it is no-op.

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

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:113: void
QuicChromiumClientStream::Handle::OnError(int error) {
Why is this named "OnError"? It's called when the Stream is closed without an
error, right?
Should we inline this method, given that it is used only in one place?

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:128: void
QuicChromiumClientStream::Handle::InvokeCallbacksOnError(int error) {
nit: maybe s/InvokeCallbacksOnError/InvokeCallbacksOnClose
This method can be called when the Stream is closed with no error.

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:135: if (*callback)
should this be if (!callback->is_null())?

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:202: return
HandleIOComplete(stream_->WriteHeaders(std::move(header_block), fin,
Sorry, I didn't make myself clear last time. I think there is a bug here.
HandleIOComplete accepts a net error, whereas stream_->WriteHeaders returns a
size_t. Are we auto casting a size_t to an int?

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:360: CompletionCallback
new_callback,
const CompletionCallback& ?

[Ryan Hamilton, 2017-06-01 23:20:30]

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.cc:110: NotifyError(rv);
On 2017/06/01 15:30:33, xunjieli (Slow.until.6-5) wrote:
> When WriteHeaders() is called synchronously in SendData()/SendvData(), we
should
> use a PostTask to NotifyError(). 
> 
> How about we change signature of WriteHeaders to return a net_error code? so
the
> caller of WriteHeaders() can choose whether to handle error asynchronously or
> synchronously?

Done. (I didn't do that in the previous patch set because at that time I
understood reentrant callbacks to be ok. Glad we got this sorted out!!)

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.cc:313: if (expect_trailers_)
On 2017/06/01 15:30:32, xunjieli (Slow.until.6-5) wrote:
> Is this needed? The second time NotifyError() is called, it is no-op. 

Yes, I think it is. The usecase is that when a server sends initial headers, and
body with a fin. The BidiQuic stream will have an aync ReadTrailers call
outstanding. When the fin arrives, the quic stream is closed which results in
the ReadTrailers callback being executed with an error (ERR_CONNECTION_CLOSED, I
think). That ends up calling NotifyError on the delegate which makes the
delegate think the request failed whereas it actually succeeded.

Now, that being said, maybe this is no longer needed now that OnClose/OnError
happen asynchronously. I'll take a look.

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

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:113: void
QuicChromiumClientStream::Handle::OnError(int error) {
On 2017/06/01 15:30:33, xunjieli (Slow.until.6-5) wrote:
> Why is this named "OnError"? It's called when the Stream is closed without an
> error, right?
> Should we inline this method, given that it is used only in one place?

As a public method, it's called from QuicChromiumClientStream::OnError when the
stream is shut down by the session with a net_error (for example
ERR_IP_ADDRESS_CHANGED, and friends). But to simplify things, I also call it
from OnClose (which is called in for non-error closes as well as QUIC error
closes).  I could factor the body out into OnCloseImpl() which would be called
by both OnError and OnClose if that would be more clear?

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:128: void
QuicChromiumClientStream::Handle::InvokeCallbacksOnError(int error) {
On 2017/06/01 15:30:33, xunjieli (Slow.until.6-5) wrote:
> nit: maybe s/InvokeCallbacksOnError/InvokeCallbacksOnClose
> This method can be called when the Stream is closed with no error.

Done.

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:135: if (*callback)
On 2017/06/01 15:30:33, xunjieli (Slow.until.6-5) wrote:
> should this be if (!callback->is_null())?

Callback has a bool operator() which is the same as is_null:

  // Returns true if Callback is null (doesn't refer to anything).
  bool is_null() const { return !bind_state_; }
  explicit operator bool() const { return !is_null(); }

If you'd prefer "->is_null" I can do that, though.

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:202: return
HandleIOComplete(stream_->WriteHeaders(std::move(header_block), fin,
On 2017/06/01 15:30:33, xunjieli (Slow.until.6-5) wrote:
> Sorry, I didn't make myself clear last time. I think there is a bug here.
> HandleIOComplete accepts a net error, whereas stream_->WriteHeaders returns a
> size_t. Are we auto casting a size_t to an int?

well, that's only a bug if the size_t is larger than max int which is only true
if the size_t is > 2^31 which would mean we'd have to write a 2GB header but
that's way larger than our limits. SO I don't think there's a bug. If we were
going the other direction, from int -> size_t, then we'd potentially have a
problem.

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:360: CompletionCallback
new_callback,
On 2017/06/01 15:30:33, xunjieli (Slow.until.6-5) wrote:
> const CompletionCallback& ?

Good point! Done.

https://codereview.chromium.org/2908243002/diff/240001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2908243002/diff/240001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.cc:226:
weak_factory_.GetWeakPtr(), rv));
Since we do this same thing every time we call WriteHeaders (check rv, and
PostTask if it failed). Should I go back to returning a bool, and do the
PostTask inside of WriteHeaders?

[xunjieli, 2017-06-02 12:43:41]

LGTM. One question below about whether |expect_trailers_| is needed.

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.cc:313: if (expect_trailers_)
On 2017/06/01 23:20:29, Ryan Hamilton wrote:
> On 2017/06/01 15:30:32, xunjieli (Slow.until.6-5) wrote:
> > Is this needed? The second time NotifyError() is called, it is no-op. 
> 
> Yes, I think it is. The usecase is that when a server sends initial headers,
and
> body with a fin. The BidiQuic stream will have an aync ReadTrailers call
> outstanding. When the fin arrives, the quic stream is closed which results in
> the ReadTrailers callback being executed with an error (ERR_CONNECTION_CLOSED,
I
> think). That ends up calling NotifyError on the delegate which makes the
> delegate think the request failed whereas it actually succeeded.
> 
> Now, that being said, maybe this is no longer needed now that OnClose/OnError
> happen asynchronously. I'll take a look.

Thanks! If body is received with a Fin, OnFinRead() should reset the read
trailers callback. So I think it's okay to not have |expect_trailers_|, but I
might be missing something. Let me know if it works.

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

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:135: if (*callback)
On 2017/06/01 23:20:29, Ryan Hamilton wrote:
> On 2017/06/01 15:30:33, xunjieli (Slow.until.6-5) wrote:
> > should this be if (!callback->is_null())?
> 
> Callback has a bool operator() which is the same as is_null:
> 
>   // Returns true if Callback is null (doesn't refer to anything).
>   bool is_null() const { return !bind_state_; }
>   explicit operator bool() const { return !is_null(); }
> 
> If you'd prefer "->is_null" I can do that, though.

Acknowledged.

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/quic...
net/quic/chromium/quic_chromium_client_stream.cc:202: return
HandleIOComplete(stream_->WriteHeaders(std::move(header_block), fin,
On 2017/06/01 23:20:29, Ryan Hamilton wrote:
> On 2017/06/01 15:30:33, xunjieli (Slow.until.6-5) wrote:
> > Sorry, I didn't make myself clear last time. I think there is a bug here.
> > HandleIOComplete accepts a net error, whereas stream_->WriteHeaders returns
a
> > size_t. Are we auto casting a size_t to an int?
> 
> well, that's only a bug if the size_t is larger than max int which is only
true
> if the size_t is > 2^31 which would mean we'd have to write a 2GB header but
> that's way larger than our limits. SO I don't think there's a bug. If we were
> going the other direction, from int -> size_t, then we'd potentially have a
> problem.

Acknowledged.

[Ryan Hamilton, 2017-06-02 14:06:27]

Thanks!!!

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/bidi...
File net/quic/chromium/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/2908243002/diff/200001/net/quic/chromium/bidi...
net/quic/chromium/bidirectional_stream_quic_impl.cc:313: if (expect_trailers_)
On 2017/06/02 12:43:41, xunjieli (Slow.until.6-5) wrote:
> On 2017/06/01 23:20:29, Ryan Hamilton wrote:
> > On 2017/06/01 15:30:32, xunjieli (Slow.until.6-5) wrote:
> > > Is this needed? The second time NotifyError() is called, it is no-op. 
> > 
> > Yes, I think it is. The usecase is that when a server sends initial headers,
> and
> > body with a fin. The BidiQuic stream will have an aync ReadTrailers call
> > outstanding. When the fin arrives, the quic stream is closed which results
in
> > the ReadTrailers callback being executed with an error
(ERR_CONNECTION_CLOSED,
> I
> > think). That ends up calling NotifyError on the delegate which makes the
> > delegate think the request failed whereas it actually succeeded.
> > 
> > Now, that being said, maybe this is no longer needed now that
OnClose/OnError
> > happen asynchronously. I'll take a look.
> 
> Thanks! If body is received with a Fin, OnFinRead() should reset the read
> trailers callback. So I think it's okay to not have |expect_trailers_|, but I
> might be missing something. Let me know if it works.

Oh, right! I forgot that I did that :) (I was struggling with how to solve this
problem and ended up implementing two different solutions and apparently forgot
to pick one!)

Removed expect_trailers_.

[xunjieli, 2017-06-02 14:13:46]

lgtm

[Ryan Hamilton, 2017-06-02 17:40:23]

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

[commit-bot: I haz the power, 2017-06-02 17:40:40]

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-06-02 19:58:57]

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

[commit-bot: I haz the power, 2017-06-02 19:58:58]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Ryan Hamilton, 2017-06-02 20:00:05]

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

[commit-bot: I haz the power, 2017-06-02 20:00:34]

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-06-02 20:59:45]

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

[commit-bot: I haz the power, 2017-06-02 20:59:46]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2017-06-03 00:20:23]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2017-06-03 00:21:18]

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-06-03 00:26:32]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1496449223022150,
"parent_rev": "b24e869c0085f7cb80020f073f731c43ae97eb17", "commit_rev":
"1bcfddf2df553b180d0253f26527f94d38710b0e"}

[commit-bot: I haz the power, 2017-06-03 00:26:46]

Description was changed from

==========
Remove QuicChromiumClientStream::Delegate in favor of async methods.

This removes OnClose and OnError from QuicHttpStream and
BidirectionalStreamQuicImpl.

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

to

==========
Remove QuicChromiumClientStream::Delegate in favor of async methods.

This removes OnClose and OnError from QuicHttpStream and
BidirectionalStreamQuicImpl.

BUG=716563

Review-Url: https://codereview.chromium.org/2908243002
Cr-Commit-Position: refs/heads/master@{#476842}
Committed:
https://chromium.googlesource.com/chromium/src/+/1bcfddf2df553b180d0253f26527...
==========

[commit-bot: I haz the power, 2017-06-03 00:26:47]

Committed patchset #15 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/1bcfddf2df553b180d0253f26527...