QuicChromiumClientSession::StreamRequest to be created by the session

net/quic/chromium/quic_chromium_client_session.cc

Index: net/quic/chromium/quic_chromium_client_session.cc
diff --git a/net/quic/chromium/quic_chromium_client_session.cc b/net/quic/chromium/quic_chromium_client_session.cc
index e7a334289a587bff6ee4f8c3fc7457ad29395f57..0701f6a1599e92df8acaf55aff1c7af1f3c3a83b 100644
--- a/net/quic/chromium/quic_chromium_client_session.cc
+++ b/net/quic/chromium/quic_chromium_client_session.cc
@@ -186,37 +186,43 @@ class QuicServerPushHelper : public ServerPushDelegate::ServerPushHelper {
 
 }  // namespace
 
-QuicChromiumClientSession::StreamRequest::StreamRequest() : stream_(nullptr) {}
+QuicChromiumClientSession::StreamRequest::StreamRequest(
+    const base::WeakPtr<QuicChromiumClientSession>& session)
+    : session_(session), stream_(nullptr) {}
 
 QuicChromiumClientSession::StreamRequest::~StreamRequest() {
-  CancelRequest();
+  if (stream_)
+    stream_->Reset(QUIC_STREAM_CANCELLED);
+
+  if (session_)
+    session_->CancelRequest(this);
 }
 
 int QuicChromiumClientSession::StreamRequest::StartRequest(
-    const base::WeakPtr<QuicChromiumClientSession>& session,
-    QuicChromiumClientStream** stream,
     const CompletionCallback& callback) {
-  session_ = session;
-  stream_ = stream;
-  int rv = session_->TryCreateStream(this, stream_);
+  DCHECK(session_);
+  int rv = session_->TryCreateStream(this);
   if (rv == ERR_IO_PENDING) {
     callback_ = callback;
+  } else {
+    session_.reset();

[Jana, 2017/04/25 19:30:20]

TryCreateStream() can return OK -- why should the session_ member be reset in
that case?

[Ryan Hamilton, 2017/04/25 20:31:48]

It doesn't *need* to be reset and I'm happy to remove this if you prefer. By
resetting it, we explicitly invalidate the pointer since we don't use it again.
But that being said, that's probably going away in a follow up anyway. I'm
inclined to leave it for now, but happy to defer to your preference.

   }
 
   return rv;
 }
 
-void QuicChromiumClientSession::StreamRequest::CancelRequest() {
-  if (session_)
-    session_->CancelRequest(this);
-  session_.reset();
-  callback_.Reset();
+QuicChromiumClientStream*
+QuicChromiumClientSession::StreamRequest::ReleaseStream() {
+  DCHECK(stream_);
+  QuicChromiumClientStream* stream = stream_;
+  stream_ = nullptr;
+  return stream;
 }
 
 void QuicChromiumClientSession::StreamRequest::OnRequestCompleteSuccess(
     QuicChromiumClientStream* stream) {
   session_.reset();
-  *stream_ = stream;
+  stream_ = stream;
   base::ResetAndReturn(&callback_).Run(OK);
 }
 
@@ -456,9 +462,15 @@ void QuicChromiumClientSession::RemoveObserver(Observer* observer) {
   observers_.erase(observer);
 }
 
-int QuicChromiumClientSession::TryCreateStream(
-    StreamRequest* request,
-    QuicChromiumClientStream** stream) {
+std::unique_ptr<QuicChromiumClientSession::StreamRequest>
+QuicChromiumClientSession::CreateStreamRequest() {
+  // base::MakeUnique does not work because the StreamRequest constructor
+  // is private.
+  return std::unique_ptr<StreamRequest>(
+      new StreamRequest(weak_factory_.GetWeakPtr()));

[Jana, 2017/04/25 19:30:20]

How's this private constructor visible to QuicChromiumClientSession? I've never
used a private constructor, so this is new to me.

[Ryan Hamilton, 2017/04/25 20:31:48]

QuicChromiumClientSession is a "friend class" of StreamRequest in the .h file.

+}
+
+int QuicChromiumClientSession::TryCreateStream(StreamRequest* request) {
   if (goaway_received()) {
     DVLOG(1) << "Going away.";
     return ERR_CONNECTION_CLOSED;
@@ -475,7 +487,7 @@ int QuicChromiumClientSession::TryCreateStream(
   }
 
   if (GetNumOpenOutgoingStreams() < max_open_outgoing_streams()) {
-    *stream = CreateOutgoingReliableStreamImpl();
+    request->stream_ = CreateOutgoingReliableStreamImpl();
     return OK;
   }