Change the way BidirectionalStreamQuicImpl detects that a stream/session

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 159d1d18cd8c4d7aca60a5991cee919add37f017..338401042803050ed70b7a8e3021b25c2c3a77dc 100644
--- a/net/quic/chromium/bidirectional_stream_quic_impl.cc
+++ b/net/quic/chromium/bidirectional_stream_quic_impl.cc
@@ -268,15 +268,26 @@ void BidirectionalStreamQuicImpl::OnDataAvailable() {
 }
 
 void BidirectionalStreamQuicImpl::OnClose() {
+  DCHECK(session_);
   DCHECK(stream_);
 
-  if (stream_->connection_error() == QUIC_NO_ERROR &&
-      stream_->stream_error() == QUIC_STREAM_NO_ERROR) {
-    ResetStream();
+  if (stream_->connection_error() != QUIC_NO_ERROR ||
+      stream_->stream_error() != QUIC_STREAM_NO_ERROR) {
+    NotifyError(was_handshake_confirmed_ ? 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);
     return;
   }
-  NotifyError(was_handshake_confirmed_ ? ERR_QUIC_PROTOCOL_ERROR
-                                       : ERR_QUIC_HANDSHAKE_FAILED);
+
+  // The connection was closed normally so there is no need to notify
+  // the delegate.

[xunjieli, 2017/05/01 20:22:02]

nice commenting!

[Ryan Hamilton, 2017/05/01 21:49:23]

Thanks!

+  ResetStream();
 }
 
 void BidirectionalStreamQuicImpl::OnError(int error) {
@@ -350,6 +361,11 @@ void BidirectionalStreamQuicImpl::NotifyStreamReady() {
 }
 
 void BidirectionalStreamQuicImpl::ResetStream() {
+  if (session_) {

[xunjieli, 2017/05/01 20:22:01]

Same question here: if Session guarantees to outlive BidiStream after you
eliminate re-entrancy, maybe we could use a raw ptr for |session_| to make the
lifetime relationship between session and bidistream simpler?

[Ryan Hamilton, 2017/05/01 21:49:23]

I believe that, like QuicHttpStream, the BidiStream is owned by the upper-layer,
not by the session and is not guaranteed to outlive the session. This detail
will be hidden in the Handle CL, thankfully. 

I did spend a good deal of time investigating what it would take to keep the
sessions until there are no more "things" which want it (streams, factory, etc).
It's possible, but I think it might actually be worse. If we switch to a,
basically, reference counted implementation then it's possible to "leak"
sessions if we're not careful. To solve this, we could have an explicit deletion
path which, say, the Factory could invoke when it's under pressure. But then
we're back to not having the lifetime guaranteed.

I *think* the Handle approach is a good compromise. The callers will have a
Handle, which they own, that will serve as a facade to the session. If the
session goes away, the Handle will still be valid and will, for example, return
ERR_CONNECTION_CLOSED (or some such) if the caller tries to write to the Handle.
I think I'm about a day or two away (once this current CL lands) from having
that follow up ready for review.

*fingers crossed*

[xunjieli, 2017/05/02 17:32:54]

Acknowledged. Ah, I see. CL description mentioned that OnSessionClosed will be
gone, so I thought we'd have the session outlive all requests/streams. The
"Handle" part is the missing piece. I agree that it's a cleaner approach for the
lifetime issues.

+    session_->RemoveObserver(this);
+    session_ = nullptr;
+  }
+
   if (!stream_)
     return;
   closed_stream_received_bytes_ = stream_->stream_bytes_read();