Fix potential crash bug with BidirectionalStreamQuicImpl::SendRequestHeaders

net/quic/chromium/bidirectional_stream_quic_impl.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/bind.h"
 #include "base/location.h"
@@ skipping 62 matching lines @@
     return;
 
   if (rv == OK) {
     OnStreamReady(rv);
   } else if (!session_->IsCryptoHandshakeConfirmed()) {
     NotifyError(ERR_QUIC_HANDSHAKE_FAILED);
   }
 }
 
 void BidirectionalStreamQuicImpl::SendRequestHeaders() {
+  WriteHeaders();
+}
+
+bool BidirectionalStreamQuicImpl::WriteHeaders() {
   DCHECK(!has_sent_headers_);
   if (!stream_) {
     LOG(ERROR)
         << "Trying to send request headers after stream has been destroyed.";
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError,
                               weak_factory_.GetWeakPtr(), ERR_UNEXPECTED));
-    return;
+    return false;
   }
 
   SpdyHeaderBlock headers;
   HttpRequestInfo http_request_info;
   http_request_info.url = request_info_->url;
   http_request_info.method = request_info_->method;
   http_request_info.extra_headers = request_info_->extra_headers;
 
   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);

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

If this fails synchronously, then the stream will end up calling OnError on the
bidi stream which will call NotifyError synchronously. That makes me nervous,
because NotifyError() is called via PostTask up at the start of this method. But
maybe it's fine to call into the delegate reentrantly?

   if (!guard.get())
-    return;
+    return false;
+
   headers_bytes_sent_ += headers_bytes_sent;
   has_sent_headers_ = true;
+  return true;
 }
 
 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_;
   }
@@ skipping 30 matching lines @@
     return;
   }
 
   std::unique_ptr<QuicConnection::ScopedPacketBundler> bundler;
   if (!has_sent_headers_) {
     DCHECK(!send_request_headers_automatically_);
     // Creates a bundler only if there are headers to be sent along with the
     // single data buffer.
     bundler =
         session_->CreatePacketBundler(QuicConnection::SEND_ACK_IF_PENDING);
-    SendRequestHeaders();
+    // Sending the request might result in |this| being deleted.
+    if (!WriteHeaders())
+      return;
   }
 
   QuicStringPiece string_data(data->data(), length);
   int rv = stream_->WriteStreamData(
       string_data, end_stream,
       base::Bind(&BidirectionalStreamQuicImpl::OnSendDataComplete,
                  weak_factory_.GetWeakPtr()));
   DCHECK(rv == OK || rv == ERR_IO_PENDING);
   if (rv == OK) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ skipping 13 matching lines @@
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError,
                               weak_factory_.GetWeakPtr(), ERR_UNEXPECTED));
     return;
   }
 
   std::unique_ptr<QuicConnection::ScopedPacketBundler> bundler(
       session_->CreatePacketBundler(QuicConnection::SEND_ACK_IF_PENDING));
   if (!has_sent_headers_) {
     DCHECK(!send_request_headers_automatically_);
-    SendRequestHeaders();
+    // Sending the request might result in |this| being deleted.
+    if (!WriteHeaders())
+      return;
   }
 
   int rv = stream_->WritevStreamData(
       buffers, lengths, end_stream,
       base::Bind(&BidirectionalStreamQuicImpl::OnSendDataComplete,
                  weak_factory_.GetWeakPtr()));
 
   DCHECK(rv == OK || rv == ERR_IO_PENDING);
   if (rv == OK) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ skipping 162 matching lines @@
     BidirectionalStreamImpl::Delegate* delegate = delegate_;
     delegate_ = nullptr;
     // Cancel any pending callback.
     weak_factory_.InvalidateWeakPtrs();
     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 |this| being deleted.
+  if (send_request_headers_automatically_ && !WriteHeaders())
+    return;
+
   if (delegate_)
     delegate_->OnStreamReady(has_sent_headers_);
 }
 
 void BidirectionalStreamQuicImpl::ResetStream() {
   if (!stream_)
     return;
   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