Fix potential crash bug with BidirectionalStreamQuicImpl::SendRequestHeaders

net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/chromium/bidirectional_stream_quic_impl.h"
 
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/memory/ptr_util.h"
@@ skipping 146 matching lines @@
     stream_ = base::MakeUnique<BidirectionalStreamQuicImpl>(std::move(session));
     stream_->Start(request_info, net_log, send_request_headers_automatically_,
                    this, nullptr);
   }
 
   void SendRequestHeaders() { stream_->SendRequestHeaders(); }
 
   void SendData(const scoped_refptr<IOBuffer>& data,
                 int length,
                 bool end_of_stream) {
-    not_expect_callback_ = true;

[Ryan Hamilton, 2017/06/01 00:45:33]

This scares me, because it sure seems like the class and the tests are going to
pains to avoid calling reentrantly into the delegate. We discussed this issue in
chat, and you're confident that such reentrancy is not problematic so I'm
calling NotifyError directly in WriteHeaders(). But I could easily post a task
instead which would be safer but possibly paranoid?

     stream_->SendData(data, length, end_of_stream);
-    not_expect_callback_ = false;
   }
 
   void SendvData(const std::vector<scoped_refptr<IOBuffer>>& data,
                  const std::vector<int>& lengths,
                  bool end_of_stream) {
-    not_expect_callback_ = true;
     stream_->SendvData(data, lengths, end_of_stream);
-    not_expect_callback_ = false;
   }
 
   // Waits until next Delegate callback.
   void WaitUntilNextCallback() {
     loop_->Run();
     loop_.reset(new base::RunLoop);
   }
 
   // Calls ReadData on the |stream_| and updates |data_received_|.
   int ReadData(const CompletionCallback& callback) {
@@ skipping 1026 matching lines @@
       static_cast<int64_t>(spdy_request_headers_frame_length + strlen(kBody1) +
                            strlen(kBody2) + strlen(kBody3) + strlen(kBody4) +
                            strlen(kBody5)),
       delegate->GetTotalSentBytes());
   EXPECT_EQ(
       static_cast<int64_t>(spdy_response_headers_frame_length +
                            strlen(kResponseBody) + spdy_trailers_frame_length),
       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();  // OnStreamReady
+
+  // 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();  // OnFailed
+  EXPECT_TRUE(delegate->on_failed_called());
+}
+
+// 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();  // OnStreamReady
+
+  // 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();  // OnFailed
+  EXPECT_TRUE(delegate->on_failed_called());
+}
+
 TEST_P(BidirectionalStreamQuicImplTest, PostRequest) {
   SetRequest("POST", "/", DEFAULT_PRIORITY);
   size_t spdy_request_headers_frame_length;
   QuicStreamOffset header_stream_offset = 0;
   AddWrite(ConstructInitialSettingsPacket(1, &header_stream_offset));
   AddWrite(ConstructRequestHeadersPacketInner(
       2, GetNthClientInitiatedStreamId(0), !kFin, DEFAULT_PRIORITY,
       &spdy_request_headers_frame_length, &header_stream_offset));
   AddWrite(ConstructDataPacket(3, kIncludeVersion, kFin, 0, kUploadData,
                                &client_maker_));
@@ skipping 737 matching lines @@
 
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(1, delegate->on_data_read_count());
   EXPECT_EQ(0, delegate->on_data_sent_count());
 }
 
 }  // namespace test
 
 }  // namespace net