Remove QuicChromiumClientStream::Delegate in favor of async methods.

net/quic/chromium/bidirectional_stream_quic_impl.cc

Index: net/quic/chromium/bidirectional_stream_quic_impl.cc
diff --git a/net/quic/chromium/bidirectional_stream_quic_impl.cc b/net/quic/chromium/bidirectional_stream_quic_impl.cc
index d581acd3d1966622293c8d9a014d8a6067b884c7..1141617096ec7d0a958979d671607d4a739b8d43 100644
--- a/net/quic/chromium/bidirectional_stream_quic_impl.cc
+++ b/net/quic/chromium/bidirectional_stream_quic_impl.cc
@@ -28,6 +28,7 @@ BidirectionalStreamQuicImpl::BidirectionalStreamQuicImpl(
       delegate_(nullptr),
       response_status_(OK),
       negotiated_protocol_(kProtoUnknown),
+      expect_trailers_(true),
       read_buffer_len_(0),
       headers_bytes_received_(0),
       headers_bytes_sent_(0),
@@ -86,9 +87,9 @@ void BidirectionalStreamQuicImpl::SendRequestHeaders() {
 
 bool BidirectionalStreamQuicImpl::WriteHeaders() {
   DCHECK(!has_sent_headers_);
-  if (!stream_) {
+  if (!stream_->IsOpen()) {
     LOG(ERROR)
-        << "Trying to send request headers after stream has been destroyed.";
+        << "Trying to send request headers after stream has been closed.";
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError,
                               weak_factory_.GetWeakPtr(), ERR_UNEXPECTED));
@@ -103,14 +104,13 @@ bool BidirectionalStreamQuicImpl::WriteHeaders() {
 
   CreateSpdyHeadersFromHttpRequest(
       http_request_info, http_request_info.extra_headers, true, &headers);
-  // Sending the request might result in |this| being deleted.
-  auto guard = weak_factory_.GetWeakPtr();
-  size_t headers_bytes_sent = stream_->WriteHeaders(
-      std::move(headers), request_info_->end_stream_on_headers, nullptr);
-  if (!guard.get())
+  int rv = stream_->WriteHeaders(std::move(headers),
+                                 request_info_->end_stream_on_headers, nullptr);
+  if (rv < 0) {
+    NotifyError(rv);

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

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?

[Ryan Hamilton, 2017/06/01 23:20:29]

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!!)

     return false;
-
-  headers_bytes_sent_ += headers_bytes_sent;
+  }
+  headers_bytes_sent_ += rv;
   has_sent_headers_ = true;
   return true;
 }
@@ -119,10 +119,6 @@ int BidirectionalStreamQuicImpl::ReadData(IOBuffer* buffer, int buffer_len) {
   DCHECK(buffer);
   DCHECK(buffer_len);
 
-  if (!stream_) {
-    // If the stream is already closed, there is no body to read.
-    return response_status_;
-  }
   int rv = stream_->ReadBody(
       buffer, buffer_len,
       base::Bind(&BidirectionalStreamQuicImpl::OnReadDataComplete,
@@ -139,6 +135,7 @@ int BidirectionalStreamQuicImpl::ReadData(IOBuffer* buffer, int buffer_len) {
   if (stream_->IsDoneReading()) {
     // If the write side is closed, OnFinRead() will call
     // BidirectionalStreamQuicImpl::OnClose().
+    expect_trailers_ = false;
     stream_->OnFinRead();
   }
   return rv;
@@ -148,8 +145,8 @@ void BidirectionalStreamQuicImpl::SendData(const scoped_refptr<IOBuffer>& data,
                                            int length,
                                            bool end_stream) {
   DCHECK(length > 0 || (length == 0 && end_stream));
-  if (!stream_) {
-    LOG(ERROR) << "Trying to send data after stream has been destroyed.";
+  if (!stream_->IsOpen()) {
+    LOG(ERROR) << "Trying to send data after stream has been closed.";
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError,
                               weak_factory_.GetWeakPtr(), ERR_UNEXPECTED));
@@ -173,11 +170,10 @@ void BidirectionalStreamQuicImpl::SendData(const scoped_refptr<IOBuffer>& data,
       string_data, end_stream,
       base::Bind(&BidirectionalStreamQuicImpl::OnSendDataComplete,
                  weak_factory_.GetWeakPtr()));
-  DCHECK(rv == OK || rv == ERR_IO_PENDING);
-  if (rv == OK) {
+  if (rv != ERR_IO_PENDING) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::OnSendDataComplete,
-                              weak_factory_.GetWeakPtr(), OK));
+                              weak_factory_.GetWeakPtr(), rv));
   }
 }
 
@@ -187,8 +183,8 @@ void BidirectionalStreamQuicImpl::SendvData(
     bool end_stream) {
   DCHECK_EQ(buffers.size(), lengths.size());
 
-  if (!stream_) {
-    LOG(ERROR) << "Trying to send data after stream has been destroyed.";
+  if (!stream_->IsOpen()) {
+    LOG(ERROR) << "Trying to send data after stream has been closed.";
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError,
                               weak_factory_.GetWeakPtr(), ERR_UNEXPECTED));
@@ -209,11 +205,10 @@ void BidirectionalStreamQuicImpl::SendvData(
       base::Bind(&BidirectionalStreamQuicImpl::OnSendDataComplete,
                  weak_factory_.GetWeakPtr()));
 
-  DCHECK(rv == OK || rv == ERR_IO_PENDING);
-  if (rv == OK) {
+  if (rv != ERR_IO_PENDING) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::OnSendDataComplete,
-                              weak_factory_.GetWeakPtr(), OK));
+                              weak_factory_.GetWeakPtr(), rv));
   }
 }
 
@@ -247,42 +242,15 @@ bool BidirectionalStreamQuicImpl::GetLoadTimingInfo(
   return true;
 }
 
-void BidirectionalStreamQuicImpl::OnClose() {
-  DCHECK(stream_);
-
-  if (stream_->connection_error() != QUIC_NO_ERROR ||
-      stream_->stream_error() != QUIC_STREAM_NO_ERROR) {
-    NotifyError(session_->IsCryptoHandshakeConfirmed()
-                    ? ERR_QUIC_PROTOCOL_ERROR
-                    : ERR_QUIC_HANDSHAKE_FAILED);
-    return;
-  }
-
-  if (!stream_->fin_sent() || !stream_->fin_received()) {
-    // The connection must have been closed by the peer with QUIC_NO_ERROR,
-    // which is improper.
-    NotifyError(ERR_UNEXPECTED);
-    return;
-  }
-
-  // The connection was closed normally so there is no need to notify
-  // the delegate.
-  ResetStream();
-}
-
-void BidirectionalStreamQuicImpl::OnError(int error) {
-  NotifyError(error);
-}
-
 void BidirectionalStreamQuicImpl::OnStreamReady(int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
-  DCHECK(rv == OK || !stream_);
+  DCHECK(!stream_);
   if (rv != OK) {
     NotifyError(rv);
     return;
   }
 
-  stream_ = session_->ReleaseStream(this);
+  stream_ = session_->ReleaseStream();
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::ReadInitialHeaders,
@@ -292,8 +260,8 @@ void BidirectionalStreamQuicImpl::OnStreamReady(int rv) {
 }
 
 void BidirectionalStreamQuicImpl::OnSendDataComplete(int rv) {
-  DCHECK(rv == OK || !stream_);
-  if (rv != 0) {
+  DCHECK_NE(ERR_IO_PENDING, rv);
+  if (rv < OK) {
     NotifyError(rv);
     return;
   }
@@ -342,7 +310,8 @@ void BidirectionalStreamQuicImpl::ReadTrailingHeaders() {
 void BidirectionalStreamQuicImpl::OnReadTrailingHeadersComplete(int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
   if (rv < 0) {
-    NotifyError(rv);
+    if (expect_trailers_)

[xunjieli, 2017/06/01 15:30:32]

Is this needed? The second time NotifyError() is called, it is no-op. 

[Ryan Hamilton, 2017/06/01 23:20:29]

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.

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

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.

[Ryan Hamilton, 2017/06/02 14:06:26]

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_.

+      NotifyError(rv);
     return;
   }
 
@@ -353,7 +322,6 @@ void BidirectionalStreamQuicImpl::OnReadTrailingHeadersComplete(int rv) {
 }
 
 void BidirectionalStreamQuicImpl::OnReadDataComplete(int rv) {
-  DCHECK_GE(rv, 0);
   read_buffer_ = nullptr;
   read_buffer_len_ = 0;
 
@@ -363,7 +331,12 @@ void BidirectionalStreamQuicImpl::OnReadDataComplete(int rv) {
     stream_->OnFinRead();
   }
 
-  if (delegate_)
+  if (!delegate_)
+    return;
+
+  if (rv < 0)
+    NotifyError(rv);
+  else
     delegate_->OnDataRead(rv);
 }
 
@@ -398,8 +371,6 @@ void BidirectionalStreamQuicImpl::ResetStream() {
   closed_stream_received_bytes_ = stream_->stream_bytes_read();
   closed_stream_sent_bytes_ = stream_->stream_bytes_written();
   closed_is_first_stream_ = stream_->IsFirstStream();
-  stream_->ClearDelegate();
-  stream_ = nullptr;
 }
 
 }  // namespace net