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 b776e1ded53239f172364761f5f472877e9e4409..3198bd077cc4c50ac103bfda43838693f7068004 100644
--- a/net/quic/chromium/bidirectional_stream_quic_impl.cc
+++ b/net/quic/chromium/bidirectional_stream_quic_impl.cc
@@ -44,6 +44,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),
@@ -102,21 +103,16 @@ void BidirectionalStreamQuicImpl::Start(
 
 void BidirectionalStreamQuicImpl::SendRequestHeaders() {
   ScopedBoolSaver saver(&may_invoke_callbacks_, false);
-  // If this fails, a task will have been posted to notify the delegate
-  // asynchronously.
-  WriteHeaders();
-}
-
-bool BidirectionalStreamQuicImpl::WriteHeaders() {
-  DCHECK(!has_sent_headers_);
-  if (!stream_) {
-    LOG(ERROR)
-        << "Trying to send request headers after stream has been destroyed.";
+  int rv = WriteHeaders();
+  if (rv < 0) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError,
-                              weak_factory_.GetWeakPtr(), ERR_UNEXPECTED));
-    return false;
+                              weak_factory_.GetWeakPtr(), rv));
   }
+}
+
+int BidirectionalStreamQuicImpl::WriteHeaders() {
+  DCHECK(!has_sent_headers_);
 
   SpdyHeaderBlock headers;
   HttpRequestInfo http_request_info;
@@ -126,17 +122,13 @@ bool BidirectionalStreamQuicImpl::WriteHeaders() {
 
   CreateSpdyHeadersFromHttpRequest(
       http_request_info, http_request_info.extra_headers, true, &headers);
-  // Sending the request might result in the stream being closed via OnClose
-  // which will post a task to notify the delegate asynchronously.
-  // TODO(rch): Clean up this interface when OnClose and OnError are removed.
-  size_t headers_bytes_sent = stream_->WriteHeaders(
-      std::move(headers), request_info_->end_stream_on_headers, nullptr);
-  if (!stream_)
-    return false;
-
-  headers_bytes_sent_ += headers_bytes_sent;
-  has_sent_headers_ = true;
-  return true;
+  int rv = stream_->WriteHeaders(std::move(headers),
+                                 request_info_->end_stream_on_headers, nullptr);
+  if (rv >= 0) {
+    headers_bytes_sent_ += rv;
+    has_sent_headers_ = true;
+  }
+  return rv;
 }
 
 int BidirectionalStreamQuicImpl::ReadData(IOBuffer* buffer, int buffer_len) {
@@ -144,10 +136,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,
@@ -164,6 +152,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;
@@ -174,8 +163,8 @@ void BidirectionalStreamQuicImpl::SendData(const scoped_refptr<IOBuffer>& data,
                                            bool end_stream) {
   ScopedBoolSaver saver(&may_invoke_callbacks_, false);
   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));
@@ -190,8 +179,13 @@ void BidirectionalStreamQuicImpl::SendData(const scoped_refptr<IOBuffer>& data,
     bundler =
         session_->CreatePacketBundler(QuicConnection::SEND_ACK_IF_PENDING);
     // Sending the request might result in the stream being closed.
-    if (!WriteHeaders())
+    int rv = WriteHeaders();
+    if (rv < 0) {
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError,
+                                weak_factory_.GetWeakPtr(), rv));
       return;
+    }
   }
 
   QuicStringPiece string_data(data->data(), length);
@@ -199,11 +193,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));
   }
 }
 
@@ -214,8 +207,8 @@ void BidirectionalStreamQuicImpl::SendvData(
   ScopedBoolSaver saver(&may_invoke_callbacks_, false);
   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));
@@ -226,9 +219,13 @@ void BidirectionalStreamQuicImpl::SendvData(
       session_->CreatePacketBundler(QuicConnection::SEND_ACK_IF_PENDING));
   if (!has_sent_headers_) {
     DCHECK(!send_request_headers_automatically_);
-    // Sending the request might result in the stream being closed.
-    if (!WriteHeaders())
+    int rv = WriteHeaders();
+    if (rv < 0) {
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError,
+                                weak_factory_.GetWeakPtr(), rv));

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

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?

       return;
+    }
   }
 
   int rv = stream_->WritevStreamData(
@@ -236,11 +233,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));
   }
 }
 
@@ -274,42 +270,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) {
-    OnError(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.
-    OnError(ERR_UNEXPECTED);
-    return;
-  }
-
-  // The connection was closed normally so there is no need to notify
-  // the delegate.
-  ResetStream();
-}
-
-void BidirectionalStreamQuicImpl::OnError(int error) {
-  // Avoid reentrancy by notifying the delegate asynchronously.
-  NotifyErrorImpl(error, /*notify_delegate_later*/ true);
-}
-
 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,
@@ -320,8 +289,8 @@ void BidirectionalStreamQuicImpl::OnStreamReady(int rv) {
 
 void BidirectionalStreamQuicImpl::OnSendDataComplete(int rv) {
   CHECK(may_invoke_callbacks_);
-  DCHECK(rv == OK || !stream_);
-  if (rv != 0) {
+  DCHECK_NE(ERR_IO_PENDING, rv);
+  if (rv < 0) {
     NotifyError(rv);
     return;
   }
@@ -372,7 +341,8 @@ void BidirectionalStreamQuicImpl::OnReadTrailingHeadersComplete(int rv) {
   CHECK(may_invoke_callbacks_);
   DCHECK_NE(ERR_IO_PENDING, rv);
   if (rv < 0) {
-    NotifyError(rv);
+    if (expect_trailers_)
+      NotifyError(rv);
     return;
   }
 
@@ -384,7 +354,7 @@ void BidirectionalStreamQuicImpl::OnReadTrailingHeadersComplete(int rv) {
 
 void BidirectionalStreamQuicImpl::OnReadDataComplete(int rv) {
   CHECK(may_invoke_callbacks_);
-  DCHECK_GE(rv, 0);
+
   read_buffer_ = nullptr;
   read_buffer_len_ = 0;
 
@@ -394,7 +364,12 @@ void BidirectionalStreamQuicImpl::OnReadDataComplete(int rv) {
     stream_->OnFinRead();
   }
 
-  if (delegate_)
+  if (!delegate_)
+    return;
+
+  if (rv < 0)
+    NotifyError(rv);
+  else
     delegate_->OnDataRead(rv);
 }
 
@@ -435,9 +410,15 @@ void BidirectionalStreamQuicImpl::NotifyFailure(
 
 void BidirectionalStreamQuicImpl::NotifyStreamReady() {
   CHECK(may_invoke_callbacks_);
-  // Sending the request might result in the stream being closed.
-  if (send_request_headers_automatically_ && !WriteHeaders())
-    return;
+  if (send_request_headers_automatically_) {
+    int rv = WriteHeaders();
+    if (rv < 0) {
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError,
+                                weak_factory_.GetWeakPtr(), rv));
+      return;
+    }
+  }
 
   if (delegate_)
     delegate_->OnStreamReady(has_sent_headers_);
@@ -449,8 +430,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