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 63 matching lines @@
                               session_->IsCryptoHandshakeConfirmed()
                                   ? rv
                                   : ERR_QUIC_HANDSHAKE_FAILED));
     return;
   }
 
   OnStreamReady(rv);
 }
 
 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();
+  // 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) {
   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 the stream being closed.
+    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 the stream being closed.
+    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 30 matching lines @@
     load_timing_info->socket_reused = true;
   }
   return true;
 }
 
 void BidirectionalStreamQuicImpl::OnClose() {
   DCHECK(stream_);
 
   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;
   }
 
   // The connection was closed normally so there is no need to notify
   // the delegate.
   ResetStream();
 }
 
 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) {
   DCHECK_NE(ERR_IO_PENDING, rv);
   DCHECK(rv == OK || !stream_);
   if (rv != OK) {
     NotifyError(rv);
     return;
   }
 
@@ skipping 76 matching lines @@
     // If the write side is closed, OnFinRead() will call
     // BidirectionalStreamQuicImpl::OnClose().
     stream_->OnFinRead();
   }
 
   if (delegate_)
     delegate_->OnDataRead(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);
 
   ResetStream();
   if (delegate_) {
     response_status_ = error;
     BidirectionalStreamImpl::Delegate* delegate = delegate_;
     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_);
 }
 
 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