Clean up error handling in BidirectionalStreamQuicImpl::Start

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 35 matching lines @@
 }
 
 void BidirectionalStreamQuicImpl::Start(
     const BidirectionalStreamRequestInfo* request_info,
     const NetLogWithSource& net_log,
     bool send_request_headers_automatically,
     BidirectionalStreamImpl::Delegate* delegate,
     std::unique_ptr<base::Timer> /* timer */) {
   DCHECK(!stream_);
   CHECK(delegate);
+  DLOG_IF(WARNING, !session_->IsConnected())
+      << "Trying to start request headers after session has been closed.";
 
   send_request_headers_automatically_ = send_request_headers_automatically;
-  if (!session_->IsConnected()) {
-    NotifyError(session_->IsCryptoHandshakeConfirmed()
-                    ? ERR_QUIC_PROTOCOL_ERROR
-                    : ERR_QUIC_HANDSHAKE_FAILED);
-    return;
-  }

[Ryan Hamilton, 2017/05/31 21:51:59]

Since both the handle's RequestStream and the session's RequestStream return an
error if the session is closed, and the session handle is always valid, we don't
need to check IsConnected().

[xunjieli, 2017/05/31 23:40:12]

Acknowledged.

-
   delegate_ = delegate;
   request_info_ = request_info;
 
   int rv = session_->RequestStream(
       request_info_->method == "POST",
       base::Bind(&BidirectionalStreamQuicImpl::OnStreamReady,
                  weak_factory_.GetWeakPtr()));
   if (rv == ERR_IO_PENDING)
     return;
 
-  if (rv == OK) {
-    OnStreamReady(rv);
-  } else if (!session_->IsCryptoHandshakeConfirmed()) {
-    NotifyError(ERR_QUIC_HANDSHAKE_FAILED);
+  if (rv != OK) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError,

[Ryan Hamilton, 2017/05/31 21:51:59]

I'm pretty sure we need to do a PostTask here, otherwise we'd call NotifyError
while under the callstack of Start(), which I think is problematic?

[xunjieli, 2017/05/31 23:40:12]

Per offline discussion, I think the PostTask is not necessary because
|delegate_| shouldn't call us back after being notified with an error. 
I am fine leaving it as it is since other methods have this problem as well.

[Ryan Hamilton, 2017/06/01 02:15:36]

I'm not worried about the delegate calling back into the BidiStream. Instead, if
NotifyError() is called synchronously here, then the BidiStream will be
reentrantly calling into the delegate while the delegate might not be expecting
to be called.
SGTM. Let's clean up all of these in a follow up (after we've sorted out the
crash bug fallout)

[xunjieli, 2017/06/01 11:57:54]

I thought about this some more yesterday. I think you are right.
If Start() can call NotifyError() synchronously, the BidiStream  (delegate of
BidiStreamQuicImpl) might not be expecting that OnFailed() callback being called
in Start(). If BidiStream accesses BidiStreamQuicImpl's states synchronously
after calling BidiStreamQuicImpl::Start() (right now it doesn't), we will be in
trouble.

OnStreamReady() on line 80 can call |delegate_|::OnStreamReady() synchronously
during Start(). I think that needs to be changed later on so we are consistent
about not calling into |delegate_| synchronously?

Sorry that this class is a mess! 

+                              weak_factory_.GetWeakPtr(),
+                              session_->IsCryptoHandshakeConfirmed()
+                                  ? ERR_QUIC_PROTOCOL_ERROR

[Ryan Hamilton, 2017/05/31 21:51:59]

We could use |rv| instead of ERR_QUIC_PROTOCOL_ERROR if we wished. (It'd likely
be ERR_CONNECTION_CLOSED). Your call.

[xunjieli, 2017/05/31 23:40:12]

SG. Let's use |rv| here.

[Ryan Hamilton, 2017/06/01 02:15:36]

Done.

+                                  : ERR_QUIC_HANDSHAKE_FAILED));
+    return;
   }
+
+  OnStreamReady(rv);
 }
 
 void BidirectionalStreamQuicImpl::SendRequestHeaders() {
   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));
@@ skipping 298 matching lines @@
   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