Add an async method to QuicChromiumClientSession::Handle for

net/quic/chromium/quic_http_stream.cc

Index: net/quic/chromium/quic_http_stream.cc
diff --git a/net/quic/chromium/quic_http_stream.cc b/net/quic/chromium/quic_http_stream.cc
index 8e27f4386b278c6eda60be6263b31ddf99515c2d..8e6a0461c63e7e71d7b5ef8bf783ec03b35104cf 100644
--- a/net/quic/chromium/quic_http_stream.cc
+++ b/net/quic/chromium/quic_http_stream.cc
@@ -65,7 +65,6 @@ QuicHttpStream::QuicHttpStream(
       user_buffer_len_(0),
       session_error_(ERR_UNEXPECTED),
       found_promise_(false),
-      push_handle_(nullptr),
       in_loop_(false),
       weak_factory_(this) {}
 
@@ -74,54 +73,6 @@ QuicHttpStream::~QuicHttpStream() {
   Close(false);
 }
 
-bool QuicHttpStream::CheckVary(const SpdyHeaderBlock& client_request,
-                               const SpdyHeaderBlock& promise_request,
-                               const SpdyHeaderBlock& promise_response) {
-  HttpResponseInfo promise_response_info;
-
-  HttpRequestInfo promise_request_info;
-  ConvertHeaderBlockToHttpRequestHeaders(promise_request,
-                                         &promise_request_info.extra_headers);
-  HttpRequestInfo client_request_info;
-  ConvertHeaderBlockToHttpRequestHeaders(client_request,
-                                         &client_request_info.extra_headers);
-
-  if (!SpdyHeadersToHttpResponse(promise_response, &promise_response_info)) {
-    DLOG(WARNING) << "Invalid headers";
-    return false;
-  }
-
-  HttpVaryData vary_data;
-  if (!vary_data.Init(promise_request_info,
-                      *promise_response_info.headers.get())) {
-    // Promise didn't contain valid vary info, so URL match was sufficient.
-    return true;
-  }
-  // Now compare the client request for matching.
-  return vary_data.MatchesRequest(client_request_info,
-                                  *promise_response_info.headers.get());
-}
-
-void QuicHttpStream::OnRendezvousResult(QuicSpdyStream* stream) {
-  push_handle_ = nullptr;
-  if (stream) {
-    stream_ = static_cast<QuicChromiumClientStream*>(stream)->CreateHandle();
-  }
-
-  // callback_ should only be non-null in the case of asynchronous
-  // rendezvous; i.e. |Try()| returned QUIC_PENDING.
-  if (callback_.is_null())
-    return;
-
-  DCHECK_EQ(STATE_HANDLE_PROMISE_COMPLETE, next_state_);
-  if (!stream) {
-    // rendezvous has failed so proceed as with a non-push request.
-    next_state_ = STATE_REQUEST_STREAM;
-  }
-
-  OnIOComplete(OK);
-}
-
 HttpResponseInfo::ConnectionInfo QuicHttpStream::ConnectionInfoFromQuicVersion(
     QuicVersion quic_version) {
   switch (quic_version) {
@@ -194,27 +145,33 @@ int QuicHttpStream::InitializeStream(const HttpRequestInfo* request_info,
 }
 
 int QuicHttpStream::DoHandlePromise() {
-  QuicAsyncStatus push_status = quic_session()->GetPushPromiseIndex()->Try(
-      request_headers_, this, &this->push_handle_);
-
-  switch (push_status) {
-    case QUIC_FAILURE:
-      // Push rendezvous failed.
-      next_state_ = STATE_REQUEST_STREAM;
-      break;
-    case QUIC_SUCCESS:
-      next_state_ = STATE_HANDLE_PROMISE_COMPLETE;
-      break;
-    case QUIC_PENDING:
-      next_state_ = STATE_HANDLE_PROMISE_COMPLETE;
-      return ERR_IO_PENDING;
+  int rv = quic_session()->RendezvousWithPromised(
+      request_headers_,
+      base::Bind(&QuicHttpStream::OnIOComplete, weak_factory_.GetWeakPtr()));

[xunjieli, 2017/06/21 20:55:28]

Should this be "base::Unretained(this)" given that QuicHttpStream owns the
handle?

[Ryan Hamilton, 2017/06/21 22:24:51]

Hm! Interesting. I think that's possible! But since all the other calls to the
handles base::Bind() with a WeakPtr, I'm inclined to leave this as-is in this CL
and then do a follow-up to switch over.

Though now that I think about it.. The HttpStream could pass an Unretained()
callback into the Handle. Could the Handle post a task to execute that callback,
which would end up running after the Handle is destroyed? I believe the Handle
does not actually do that, but I'm curious about the contract...

[xunjieli, 2017/06/22 14:43:30]

Acknowledged. Let's do it as a follow-up. If the Handle wants to take the
callback and post it as a task, it should wrap it in a base::Bind(). The handle
can use weak_ptr or base::Unretained() as appropriate for the wrapped callback.

+
+  if (rv == ERR_IO_PENDING) {
+    next_state_ = STATE_HANDLE_PROMISE_COMPLETE;
+    return ERR_IO_PENDING;
   }
-  return OK;
+
+  if (rv < 0) {

[xunjieli, 2017/06/21 20:55:28]

nit: This branch is not needed. We can go directly to
STATE_HANDLE_PROMISE_COMPLETE, because DoHandlePromiseComplete() handles the
error case.

[Ryan Hamilton, 2017/06/21 22:24:51]

Hah! Clever! In fact, we don't need any of these branches.

Done.

+    // Push rendezvous failed.
+    next_state_ = STATE_REQUEST_STREAM;
+    return OK;
+  }
+
+  next_state_ = STATE_HANDLE_PROMISE_COMPLETE;
+  return rv;
 }
 
 int QuicHttpStream::DoHandlePromiseComplete(int rv) {

[xunjieli, 2017/06/21 20:55:28]

optional nit: add a 
DCHECK_NE(ERR_IO_PENDING, rv)
DCHECK_LE(rv, OK)
here just to make the expected range of values for |rv| clearer.

[Ryan Hamilton, 2017/06/21 22:24:51]

Done.

-  if (rv != OK)
-    return rv;
+  if (rv != OK) {
+    // rendezvous has failed so proceed as with a non-push request.
+    next_state_ = STATE_REQUEST_STREAM;
+    return OK;
+  }
+
+  stream_ = quic_session()->ReleasePromisedStream();
 
   next_state_ = STATE_OPEN;
   stream_net_log_.AddEvent(
@@ -511,7 +468,6 @@ int QuicHttpStream::DoLoop(int rv) {
         rv = DoHandlePromise();
         break;
       case STATE_HANDLE_PROMISE_COMPLETE:
-        CHECK_EQ(OK, rv);
         rv = DoHandlePromiseComplete(rv);
         break;
       case STATE_REQUEST_STREAM:
@@ -741,11 +697,6 @@ int QuicHttpStream::HandleReadComplete(int rv) {
 }
 
 void QuicHttpStream::ResetStream() {
-  if (push_handle_) {
-    push_handle_->Cancel();
-    push_handle_ = nullptr;
-  }
-
   // If |request_body_stream_| is non-NULL, Reset it, to abort any in progress
   // read.
   if (request_body_stream_)