Clean up error handling in BidirectionalStreamQuicImpl::Start

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 c3864fce8561b7ad838f3baef728c3a58ca391ac..96c880783b16efac96f36ccdc2cc495422b91d7f 100644
--- a/net/quic/chromium/bidirectional_stream_quic_impl.cc
+++ b/net/quic/chromium/bidirectional_stream_quic_impl.cc
@@ -53,15 +53,10 @@ void BidirectionalStreamQuicImpl::Start(
     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;
 
@@ -72,11 +67,17 @@ void BidirectionalStreamQuicImpl::Start(
   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() {