Fix potential crash bug with BidirectionalStreamQuicImpl::SendRequestHeaders

resulting in the connection being closed due to a synchronous write error.

BUG=728434

Review-Url: https://codereview.chromium.org/2915973002
Cr-Commit-Position: refs/heads/master@{#476491}

net/quic/chromium/bidirectional_stream_quic_impl.cc

@@ -81,6 +81,10 @@ void BidirectionalStreamQuicImpl::Start(
 }
 
 void BidirectionalStreamQuicImpl::SendRequestHeaders() {
+  WriteHeaders();
+}
+
+bool BidirectionalStreamQuicImpl::WriteHeaders() {
   DCHECK(!has_sent_headers_);
   if (!stream_) {
     LOG(ERROR)
@@ -88,7 +92,7 @@ void BidirectionalStreamQuicImpl::SendRequestHeaders() {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError,
                               weak_factory_.GetWeakPtr(), ERR_UNEXPECTED));
-    return;
+    return false;
   }
 
   SpdyHeaderBlock headers;
@@ -99,14 +103,17 @@ void BidirectionalStreamQuicImpl::SendRequestHeaders() {
 
   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();
+  // 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 (!guard.get())
-    return;
+  if (!stream_)
+    return false;
+
   headers_bytes_sent_ += headers_bytes_sent;
   has_sent_headers_ = true;
+  return true;
 }
 
 int BidirectionalStreamQuicImpl::ReadData(IOBuffer* buffer, int buffer_len) {
@@ -157,7 +164,9 @@ void BidirectionalStreamQuicImpl::SendData(const scoped_refptr<IOBuffer>& data,
     // single data buffer.
     bundler =
         session_->CreatePacketBundler(QuicConnection::SEND_ACK_IF_PENDING);
-    SendRequestHeaders();
+    // Sending the request might result in the stream being closed.
+    if (!WriteHeaders())
+      return;
   }
 
   QuicStringPiece string_data(data->data(), length);
@@ -191,7 +200,9 @@ void BidirectionalStreamQuicImpl::SendvData(
       session_->CreatePacketBundler(QuicConnection::SEND_ACK_IF_PENDING));
   if (!has_sent_headers_) {
     DCHECK(!send_request_headers_automatically_);
-    SendRequestHeaders();
+    // Sending the request might result in the stream being closed.
+    if (!WriteHeaders())
+      return;
   }
 
   int rv = stream_->WritevStreamData(
@@ -242,16 +253,15 @@ void BidirectionalStreamQuicImpl::OnClose() {
 
   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);
+    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.
-    NotifyError(ERR_UNEXPECTED);
+    OnError(ERR_UNEXPECTED);
     return;
   }
 
@@ -261,7 +271,8 @@ void BidirectionalStreamQuicImpl::OnClose() {
 }
 
 void BidirectionalStreamQuicImpl::OnError(int error) {
-  NotifyError(error);
+  // Avoid reentrancy by notifying the delegate asynchronously.
+  NotifyErrorImpl(error, /*notify_delegate_later*/ true);
 }
 
 void BidirectionalStreamQuicImpl::OnStreamReady(int rv) {
@@ -358,6 +369,11 @@ void BidirectionalStreamQuicImpl::OnReadDataComplete(int rv) {
 }
 
 void BidirectionalStreamQuicImpl::NotifyError(int error) {
+  NotifyErrorImpl(error, /*notify_delegate_later*/ false);
+}
+
+void BidirectionalStreamQuicImpl::NotifyErrorImpl(int error,
+                                                  bool notify_delegate_later) {
   DCHECK_NE(OK, error);
   DCHECK_NE(ERR_IO_PENDING, error);
 
@@ -368,19 +384,29 @@ void BidirectionalStreamQuicImpl::NotifyError(int error) {
     delegate_ = nullptr;
     // Cancel any pending callback.
     weak_factory_.InvalidateWeakPtrs();
-    delegate->OnFailed(error);
-    // |this| might be destroyed at this point.
+    if (notify_delegate_later) {
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyFailure,
+                                weak_factory_.GetWeakPtr(), delegate, error));
+    } else {
+      NotifyFailure(delegate, error);
+      // |this| might be destroyed at this point.
+    }
   }
 }
 
+void BidirectionalStreamQuicImpl::NotifyFailure(
+    BidirectionalStreamImpl::Delegate* delegate,
+    int error) {
+  delegate->OnFailed(error);
+  // |this| might be destroyed at this point.
+}
+
 void BidirectionalStreamQuicImpl::NotifyStreamReady() {
-  if (send_request_headers_automatically_) {
-    // Sending the request might result in |this| being deleted.
-    auto guard = weak_factory_.GetWeakPtr();
-    SendRequestHeaders();
-    if (!guard.get())
-      return;
-  }
+  // Sending the request might result in the stream being closed.
+  if (send_request_headers_automatically_ && !WriteHeaders())
+    return;
+
   if (delegate_)
     delegate_->OnStreamReady(has_sent_headers_);
 }

net/quic/chromium/bidirectional_stream_quic_impl.h

@@ -61,6 +61,10 @@ class NET_EXPORT_PRIVATE BidirectionalStreamQuicImpl
   void OnClose() override;
   void OnError(int error) override;
 
+  // Write headers to the stream and returns true on success. Posts a task to
+  // notify the delegate asynchronously and returns false on failure
+  bool WriteHeaders();
+
   void OnStreamReady(int rv);
   void OnSendDataComplete(int rv);
   void ReadInitialHeaders();
@@ -69,12 +73,20 @@ class NET_EXPORT_PRIVATE BidirectionalStreamQuicImpl
   void OnReadTrailingHeadersComplete(int rv);
   void OnReadDataComplete(int rv);
 
-  // Notifies the delegate of an error.
+  // Notifies the delegate of an error, clears |stream_| and |delegate_|,
+  // and cancels any pending callbacks.
   void NotifyError(int error);
+  // Notifies the delegate of an error, clears |stream_| and |delegate_|,
+  // and cancels any pending callbacks. If |notify_delegate_later| is true
+  // then the delegate will be notified asynchronously via a posted task,
+  // otherwise the notification will be synchronous.
+  void NotifyErrorImpl(int error, bool notify_delegate_later);
   // Notifies the delegate that the stream is ready.
   void NotifyStreamReady();
   // Resets the stream and ensures that |delegate_| won't be called back.
   void ResetStream();
+  // Invokes OnFailure(error) on |delegate|.
+  void NotifyFailure(BidirectionalStreamImpl::Delegate* delegate, int error);
 
   const std::unique_ptr<QuicChromiumClientSession::Handle> session_;
   std::unique_ptr<QuicChromiumClientStream::Handle> stream_;

net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc

@@ -1248,6 +1248,75 @@ TEST_P(BidirectionalStreamQuicImplTest,
       delegate->GetTotalReceivedBytes());
 }
 
+// Tests that when request headers are delayed and SendData triggers the
+// headers to be sent, if that write fails the stream does not crash.
+TEST_P(BidirectionalStreamQuicImplTest,
+       SendDataWriteErrorCoalesceDataBufferAndHeaderFrame) {
+  QuicStreamOffset header_stream_offset = 0;
+  AddWrite(ConstructInitialSettingsPacket(1, &header_stream_offset));
+  AddWriteError(SYNCHRONOUS, ERR_CONNECTION_REFUSED);
+
+  Initialize();
+
+  BidirectionalStreamRequestInfo request;
+  request.method = "POST";
+  request.url = GURL("http://www.google.com/");
+  request.end_stream_on_headers = false;
+  request.priority = DEFAULT_PRIORITY;
+  request.extra_headers.SetHeader("cookie", std::string(2048, 'A'));
+
+  scoped_refptr<IOBuffer> read_buffer(new IOBuffer(kReadBufferSize));
+  std::unique_ptr<DeleteStreamDelegate> delegate(new DeleteStreamDelegate(
+      read_buffer.get(), kReadBufferSize, DeleteStreamDelegate::ON_FAILED));
+  delegate->DoNotSendRequestHeadersAutomatically();
+  delegate->Start(&request, net_log().bound(), session()->CreateHandle());
+  ConfirmHandshake();
+  delegate->WaitUntilNextCallback(kOnStreamReady);
+
+  // Attempt to send the headers and data.
+  const char kBody1[] = "here are some data";
+  scoped_refptr<StringIOBuffer> buf1(new StringIOBuffer(kBody1));
+  delegate->SendData(buf1, buf1->size(), !kFin);
+
+  delegate->WaitUntilNextCallback(kOnFailed);
+}
+
+// Tests that when request headers are delayed and SendvData triggers the
+// headers to be sent, if that write fails the stream does not crash.
+TEST_P(BidirectionalStreamQuicImplTest,
+       SendvDataWriteErrorCoalesceDataBufferAndHeaderFrame) {
+  QuicStreamOffset header_stream_offset = 0;
+  AddWrite(ConstructInitialSettingsPacket(1, &header_stream_offset));
+  AddWriteError(SYNCHRONOUS, ERR_CONNECTION_REFUSED);
+
+  Initialize();
+
+  BidirectionalStreamRequestInfo request;
+  request.method = "POST";
+  request.url = GURL("http://www.google.com/");
+  request.end_stream_on_headers = false;
+  request.priority = DEFAULT_PRIORITY;
+  request.extra_headers.SetHeader("cookie", std::string(2048, 'A'));
+
+  scoped_refptr<IOBuffer> read_buffer(new IOBuffer(kReadBufferSize));
+  std::unique_ptr<DeleteStreamDelegate> delegate(new DeleteStreamDelegate(
+      read_buffer.get(), kReadBufferSize, DeleteStreamDelegate::ON_FAILED));
+  delegate->DoNotSendRequestHeadersAutomatically();
+  delegate->Start(&request, net_log().bound(), session()->CreateHandle());
+  ConfirmHandshake();
+  delegate->WaitUntilNextCallback(kOnStreamReady);
+
+  // Attempt to send the headers and data.
+  const char kBody1[] = "here are some data";
+  const char kBody2[] = "data keep coming";
+  scoped_refptr<StringIOBuffer> buf1(new StringIOBuffer(kBody1));
+  scoped_refptr<StringIOBuffer> buf2(new StringIOBuffer(kBody2));
+  std::vector<int> lengths = {buf1->size(), buf2->size()};
+  delegate->SendvData({buf1, buf2}, lengths, !kFin);
+
+  delegate->WaitUntilNextCallback(kOnFailed);
+}
+
 TEST_P(BidirectionalStreamQuicImplTest, PostRequest) {
   SetRequest("POST", "/", DEFAULT_PRIORITY);
   size_t spdy_request_headers_frame_length;
@@ -1520,7 +1589,7 @@ TEST_P(BidirectionalStreamQuicImplTest, ServerSendsRstAfterHeaders) {
   // Server sends a Rst.
   ProcessPacket(ConstructServerRstStreamPacket(1));
 
-  EXPECT_TRUE(delegate->on_failed_called());
+  delegate->WaitUntilNextCallback(kOnFailed);
 
   TestCompletionCallback cb;
   EXPECT_THAT(delegate->ReadData(cb.callback()),
@@ -1584,7 +1653,7 @@ TEST_P(BidirectionalStreamQuicImplTest, ServerSendsRstAfterReadData) {
   // Server sends a Rst.
   ProcessPacket(ConstructServerRstStreamPacket(3));
 
-  EXPECT_TRUE(delegate->on_failed_called());
+  delegate->WaitUntilNextCallback(kOnFailed);
 
   EXPECT_THAT(delegate->ReadData(cb.callback()),
               IsError(ERR_QUIC_PROTOCOL_ERROR));
@@ -1638,7 +1707,7 @@ TEST_P(BidirectionalStreamQuicImplTest, SessionClosedBeforeReadData) {
   EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
   session()->connection()->CloseConnection(
       QUIC_NO_ERROR, "test", ConnectionCloseBehavior::SILENT_CLOSE);
-  EXPECT_TRUE(delegate->on_failed_called());
+  delegate->WaitUntilNextCallback(kOnFailed);
 
   // Try to send data after OnFailed(), should not get called back.
   scoped_refptr<StringIOBuffer> buf(new StringIOBuffer(kUploadData));