Fix potential crash bug with BidirectionalStreamQuicImpl::SendRequestHeaders

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 5d2ae0445bd8c4126f750d7d803060f4cae81154..315b400306cd37239cebf7d9a37cf04b26647c6f 100644
--- a/net/quic/chromium/bidirectional_stream_quic_impl.cc
+++ b/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,15 @@ 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.

[Ryan Hamilton, 2017/06/01 17:10:14]

Since this CL no longer notifies the delegate synchronously on write errors
(well, on any errors) we no longer need to use a WeakPtr here to detect |this|
being deleted. We do, however, need to handle |stream_| being deleted so we
don't dereference it later.

It would be possible to do this detection at the various call sites if you
prefer, though I think it's cleaner for this method to signal failure with the
return value.

[xunjieli, 2017/06/01 21:24:02]

Acknowledged. If the upcalls (OnClose and OnError) are removed
(https://codereview.chromium.org/2908243002/), WriteHeaders() should only return
true, right?
If so, should we add a TODO here?

[Ryan Hamilton, 2017/06/01 21:43:48]

Yeah, either a bool, or an int. TODO added.

   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;

[xunjieli, 2017/06/01 21:24:02]

Could you add a comment here that NotifyError() will be called asynchronously
through OnClose()?

The caller of WriteHeader() doesn't handle the error when WriteHeader() returns
false here. I think it's worth a comment.

[Ryan Hamilton, 2017/06/01 21:43:48]

In both places that this method returns false, a task will have been posted to
notify the delegate. I've added a comment to the method declaration. 

+
   headers_bytes_sent_ += headers_bytes_sent;
   has_sent_headers_ = true;
+  return true;
 }
 
 int BidirectionalStreamQuicImpl::ReadData(IOBuffer* buffer, int buffer_len) {
@@ -157,7 +162,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 +198,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 +251,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 +269,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 +367,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 +382,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_);
 }