Add a new QuicChromiumClientSession::Handle class

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"
 #include "base/logging.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/timer/timer.h"
 #include "net/http/bidirectional_stream_request_info.h"
 #include "net/quic/core/quic_connection.h"
 #include "net/quic/platform/api/quic_string_piece.h"
 #include "net/socket/next_proto.h"
 #include "net/spdy/chromium/spdy_http_utils.h"
 #include "net/spdy/core/spdy_header_block.h"
 
 namespace net {
 
 BidirectionalStreamQuicImpl::BidirectionalStreamQuicImpl(
-    const base::WeakPtr<QuicChromiumClientSession>& session)
+    QuicChromiumClientSession::Handle session)
     : session_(session),
-      was_handshake_confirmed_(session->IsCryptoHandshakeConfirmed()),
       stream_(nullptr),
       request_info_(nullptr),
       delegate_(nullptr),
       response_status_(OK),
       negotiated_protocol_(kProtoUnknown),
       read_buffer_len_(0),
       headers_bytes_received_(0),
       headers_bytes_sent_(0),
       closed_stream_received_bytes_(0),
       closed_stream_sent_bytes_(0),
       closed_is_first_stream_(false),
       has_sent_headers_(false),
       has_received_headers_(false),
       send_request_headers_automatically_(true),
-      weak_factory_(this) {
-  DCHECK(session_);
-  session_->AddObserver(this);
-}
+      weak_factory_(this) {}
 
 BidirectionalStreamQuicImpl::~BidirectionalStreamQuicImpl() {
   if (stream_) {
     delegate_ = nullptr;
     stream_->Reset(QUIC_STREAM_CANCELLED);
   }
-
-  if (session_)
-    session_->RemoveObserver(this);
 }
 
 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);
 
   send_request_headers_automatically_ = send_request_headers_automatically;
-  if (!session_) {
-    NotifyError(was_handshake_confirmed_ ? ERR_QUIC_PROTOCOL_ERROR
-                                         : ERR_QUIC_HANDSHAKE_FAILED);
+  if (!session_.IsConnected()) {
+    NotifyError(session_.IsCryptoHandshakeConfirmed()
+                    ? ERR_QUIC_PROTOCOL_ERROR
+                    : ERR_QUIC_HANDSHAKE_FAILED);
     return;
   }
 
   delegate_ = delegate;
   request_info_ = request_info;
 
   stream_request_ =
-      session_->CreateStreamRequest(request_info_->method == "POST");
+      session_.CreateStreamRequest(request_info_->method == "POST");
   int rv = stream_request_->StartRequest(base::Bind(

[xunjieli, 2017/05/04 16:54:42]

Instead of requiring BidiStream/HttpStream to know about both StreamRequest and
Handle, have you considered eliminating one of these two from a consumer point
of view?

For example, we can make StreamRequest a nested class within Handle.
Handle::Start() calls StreamRequest::StartRequest(). Then BidiStream/HttpStream
doesn't need to know about StreamRequest.

If StreamRequest::StartRequest() finishes with an error that leads to the
Session being synchronously destroyed, we can handle that in the Handle class.

Right now Handle and StreamRequest have references to each other. This
cross-reference takes some effort to understand.

[Ryan Hamilton, 2017/05/04 18:45:53]

I hear ya! I spent a bunch of time thinking about this because, as you say, it
seems like hiding the Request in the Handle makes sense. However, I think it
complicates the Handle a bit, because I think it makes copy semantics
problematic. If I have a handle and start a request and then make a copy of the
handle do they both have the same request? That can't work. Do they get
different requests? That also sounds problematic.

So one solution would be to make the Handle non-copyable, but I'm not in love
with that approach either 'cause I think it makes life a bit harder on the
caller.  

Long story short, I'm inclined to leave it as-is, but happy to revisit if you
feel strongly.

[Ryan Hamilton, 2017/05/05 03:50:24]

Now that we decided to make the handle non-copyable this is a non-issue, so I've
made this change. It's a nice cleanup! thanks

       &BidirectionalStreamQuicImpl::OnStreamReady, weak_factory_.GetWeakPtr()));
   if (rv == ERR_IO_PENDING)
     return;
 
   if (rv == OK) {
     OnStreamReady(rv);
-  } else if (!was_handshake_confirmed_) {
+  } else if (!session_.IsCryptoHandshakeConfirmed()) {
     NotifyError(ERR_QUIC_HANDSHAKE_FAILED);
   }
 }
 
 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(
@@ skipping 50 matching lines @@
         FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError,
                               weak_factory_.GetWeakPtr(), ERR_UNEXPECTED));
     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.reset(new QuicConnection::ScopedPacketBundler(
-        session_->connection(), QuicConnection::SEND_ACK_IF_PENDING));
+    bundler = session_.CreatePacketBundler(QuicConnection::SEND_ACK_IF_PENDING);
     SendRequestHeaders();
   }
 
   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) {
@@ skipping 10 matching lines @@
   DCHECK_EQ(buffers.size(), lengths.size());
 
   if (!stream_) {
     LOG(ERROR) << "Trying to send data after stream has been destroyed.";
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError,
                               weak_factory_.GetWeakPtr(), ERR_UNEXPECTED));
     return;
   }
 
-  QuicConnection::ScopedPacketBundler bundler(
-      session_->connection(), QuicConnection::SEND_ACK_IF_PENDING);
+  std::unique_ptr<QuicConnection::ScopedPacketBundler> bundler(
+      session_.CreatePacketBundler(QuicConnection::SEND_ACK_IF_PENDING));
   if (!has_sent_headers_) {
     DCHECK(!send_request_headers_automatically_);
     SendRequestHeaders();
   }
 
   int rv = stream_->WritevStreamData(

[xunjieli, 2017/05/04 16:54:42]

Potential crash?
With OnSessionClosed() removed, if the |session| is gone synchronously because
SendRequestHeaders() errors out synchronously, we won't know |stream_| is
invalid.

[Ryan Hamilton, 2017/05/05 03:50:24]

Seems like a potential crash, but as discussed, it's pre-existing.

       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(
         FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::OnSendDataComplete,
                               weak_factory_.GetWeakPtr(), OK));
   }
@@ skipping 29 matching lines @@
   return true;
 }
 
 void BidirectionalStreamQuicImpl::OnHeadersAvailable(
     const SpdyHeaderBlock& headers,
     size_t frame_len) {
   headers_bytes_received_ += frame_len;
   negotiated_protocol_ = kProtoQUIC;
   if (!has_received_headers_) {
     has_received_headers_ = true;
-    connect_timing_ = session_->GetConnectTiming();
+    connect_timing_ = session_.GetConnectTiming();
     if (delegate_)
       delegate_->OnHeadersReceived(headers);
   } else {
     if (delegate_)
       delegate_->OnTrailersReceived(headers);
     // |this| can be destroyed after this point.
   }
 }
 
 void BidirectionalStreamQuicImpl::OnDataAvailable() {
   // Return early if ReadData has not been called.
   if (!read_buffer_)
     return;
 
   int rv = ReadData(read_buffer_.get(), read_buffer_len_);
   if (rv == ERR_IO_PENDING) {
     // Spurrious notification. Wait for the next one.
     return;
   }
   read_buffer_ = nullptr;
   read_buffer_len_ = 0;
   if (delegate_)
     delegate_->OnDataRead(rv);
 }
 
 void BidirectionalStreamQuicImpl::OnClose() {
-  DCHECK(session_);
   DCHECK(stream_);
 
   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);
+    NotifyError(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);
     return;
   }
 
   // The connection was closed normally so there is no need to notify
   // the delegate.
   ResetStream();
 }
 
 void BidirectionalStreamQuicImpl::OnError(int error) {
   NotifyError(error);
 }
 
 bool BidirectionalStreamQuicImpl::HasSendHeadersComplete() {
   return has_sent_headers_;
 }
 
-void BidirectionalStreamQuicImpl::OnCryptoHandshakeConfirmed() {
-  was_handshake_confirmed_ = true;
-}
-
-void BidirectionalStreamQuicImpl::OnSuccessfulVersionNegotiation(
-    const QuicVersion& version) {}
-
-void BidirectionalStreamQuicImpl::OnSessionClosed(
-    int error,
-    bool /*port_migration_detected*/) {
-  DCHECK_NE(OK, error);
-  session_.reset();
-  NotifyError(error);
-}
-
 void BidirectionalStreamQuicImpl::OnStreamReady(int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
   DCHECK(rv == OK || !stream_);
   if (rv == OK) {
     stream_ = stream_request_->ReleaseStream();
     stream_request_.reset();
     stream_->SetDelegate(this);
     NotifyStreamReady();
   } else {
     NotifyError(rv);
@@ skipping 28 matching lines @@
 
 void BidirectionalStreamQuicImpl::NotifyStreamReady() {
   if (send_request_headers_automatically_) {
     SendRequestHeaders();
   }
   if (delegate_)
     delegate_->OnStreamReady(has_sent_headers_);
 }
 
 void BidirectionalStreamQuicImpl::ResetStream() {
-  if (session_) {
-    session_->RemoveObserver(this);
-    session_ = nullptr;
-  }
-
   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_->SetDelegate(nullptr);
   stream_ = nullptr;
 }
 
 }  // namespace net