Add an async method to QuicChromiumClientSession::Handle for

rendezvousing with promised streams. This fixes a bug where
a session could be closed, and the push handle deleted
before the QuicHttpStream was notified resulting in
a use-after-free of the push handle.

BUG=719461

Review-Url: https://codereview.chromium.org/2948143002
Cr-Commit-Position: refs/heads/master@{#481886}

net/quic/chromium/quic_chromium_client_session.cc

@@ -199,12 +199,19 @@ QuicChromiumClientSession::Handle::Handle(
       error_(OK),
       port_migration_detected_(false),
       server_id_(session_->server_id()),
-      quic_version_(session->connection()->version()) {
+      quic_version_(session->connection()->version()),
+      push_handle_(nullptr) {
   DCHECK(session_);
   session_->AddHandle(this);
 }
 
 QuicChromiumClientSession::Handle::~Handle() {
+  if (push_handle_) {
+    auto* push_handle = push_handle_;
+    push_handle_ = nullptr;
+    push_handle->Cancel();
+  }
+
   if (session_)
     session_->RemoveHandle(this);
 }
@@ -223,6 +230,7 @@ void QuicChromiumClientSession::Handle::OnSessionClosed(
   error_ = error;
   quic_version_ = quic_version;
   connect_timing_ = connect_timing;
+  push_handle_ = nullptr;
 }
 
 bool QuicChromiumClientSession::Handle::IsConnected() const {
@@ -289,6 +297,28 @@ bool QuicChromiumClientSession::Handle::SharesSameSession(
   return session_.get() == other.session_.get();
 }
 
+int QuicChromiumClientSession::Handle::RendezvousWithPromised(
+    const SpdyHeaderBlock& headers,
+    const CompletionCallback& callback) {
+  if (!session_)
+    return ERR_CONNECTION_CLOSED;
+
+  QuicAsyncStatus push_status =
+      session_->push_promise_index()->Try(headers, this, &push_handle_);
+
+  switch (push_status) {
+    case QUIC_FAILURE:
+      return ERR_FAILED;
+    case QUIC_SUCCESS:
+      return OK;
+    case QUIC_PENDING:
+      push_callback_ = callback;
+      return ERR_IO_PENDING;
+  }
+  NOTREACHED();
+  return ERR_UNEXPECTED;
+}
+
 int QuicChromiumClientSession::Handle::RequestStream(
     bool requires_confirmation,
     const CompletionCallback& callback) {
@@ -313,6 +343,12 @@ QuicChromiumClientSession::Handle::ReleaseStream() {
   return handle;
 }
 
+std::unique_ptr<QuicChromiumClientStream::Handle>
+QuicChromiumClientSession::Handle::ReleasePromisedStream() {
+  DCHECK(push_stream_);
+  return std::move(push_stream_);
+}
+
 int QuicChromiumClientSession::Handle::WaitForHandshakeConfirmation(
     const CompletionCallback& callback) {
   if (!session_)
@@ -350,6 +386,51 @@ int QuicChromiumClientSession::Handle::GetPeerAddress(
   return OK;
 }
 
+bool QuicChromiumClientSession::Handle::CheckVary(
+    const SpdyHeaderBlock& client_request,
+    const SpdyHeaderBlock& promise_request,
+    const SpdyHeaderBlock& promise_response) {
+  HttpRequestInfo promise_request_info;
+  ConvertHeaderBlockToHttpRequestHeaders(promise_request,
+                                         &promise_request_info.extra_headers);
+  HttpRequestInfo client_request_info;
+  ConvertHeaderBlockToHttpRequestHeaders(client_request,
+                                         &client_request_info.extra_headers);
+
+  HttpResponseInfo promise_response_info;
+  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 QuicChromiumClientSession::Handle::OnRendezvousResult(
+    QuicSpdyStream* stream) {
+  DCHECK(!push_stream_);
+  int rv = ERR_FAILED;
+  if (stream) {
+    rv = OK;
+    push_stream_ =
+        static_cast<QuicChromiumClientStream*>(stream)->CreateHandle();
+  }
+
+  if (push_callback_) {
+    DCHECK(push_handle_);
+    push_handle_ = nullptr;
+    base::ResetAndReturn(&push_callback_).Run(rv);
+  }
+}
+
 QuicChromiumClientSession::StreamRequest::StreamRequest(
     QuicChromiumClientSession::Handle* session,
     bool requires_confirmation)

net/quic/chromium/quic_chromium_client_session.h

@@ -32,6 +32,7 @@
 #include "net/quic/chromium/quic_chromium_packet_reader.h"
 #include "net/quic/chromium/quic_chromium_packet_writer.h"
 #include "net/quic/chromium/quic_connection_logger.h"
+#include "net/quic/core/quic_client_push_promise_index.h"
 #include "net/quic/core/quic_client_session_base.h"
 #include "net/quic/core/quic_crypto_client_stream.h"
 #include "net/quic/core/quic_packets.h"
@@ -71,7 +72,9 @@ class NET_EXPORT_PRIVATE QuicChromiumClientSession
   // Wrapper for interacting with the session in a restricted fashion which
   // hides the details of the underlying session's lifetime. All methods of
   // the Handle are safe to use even after the underlying session is destroyed.
-  class NET_EXPORT_PRIVATE Handle : public MultiplexedSessionHandle {
+  class NET_EXPORT_PRIVATE Handle
+      : public MultiplexedSessionHandle,
+        public QuicClientPushPromiseIndex::Delegate {
    public:
     explicit Handle(const base::WeakPtr<QuicChromiumClientSession>& session);
     Handle(const Handle& other) = delete;
@@ -83,6 +86,13 @@ class NET_EXPORT_PRIVATE QuicChromiumClientSession
     // Returns true if the handshake has been confirmed.
     bool IsCryptoHandshakeConfirmed() const;
 
+    // Starts a request to rendezvous with a promised a stream.  If OK is
+    // returned, then |push_stream_| will be updated with the promised
+    // stream.  If ERR_IO_PENDING is returned, then when the rendezvous is
+    // eventually completed |callback| will be called.
+    int RendezvousWithPromised(const SpdyHeaderBlock& headers,
+                               const CompletionCallback& callback);
+
     // Starts a request to create a stream.  If OK is returned, then
     // |stream_| will be updated with the newly created stream.  If
     // ERR_IO_PENDING is returned, then when the request is eventuallly
@@ -93,6 +103,9 @@ class NET_EXPORT_PRIVATE QuicChromiumClientSession
     // Releases |stream_| to the caller.
     std::unique_ptr<QuicChromiumClientStream::Handle> ReleaseStream();
 
+    // Releases |push_stream_| to the caller.
+    std::unique_ptr<QuicChromiumClientStream::Handle> ReleasePromisedStream();
+
     // Sends Rst for the stream, and makes sure that future calls to
     // IsClosedStream(id) return true, which ensures that any subsequent
     // frames related to this stream will be ignored (modulo flow
@@ -135,6 +148,12 @@ class NET_EXPORT_PRIVATE QuicChromiumClientSession
     // Returns the session's net log.
     const NetLogWithSource& net_log() const { return net_log_; }
 
+    // QuicClientPushPromiseIndex::Delegate implementation
+    bool CheckVary(const SpdyHeaderBlock& client_request,
+                   const SpdyHeaderBlock& promise_request,
+                   const SpdyHeaderBlock& promise_response) override;
+    void OnRendezvousResult(QuicSpdyStream* stream) override;
+
    private:
     friend class QuicChromiumClientSession;
     friend class QuicChromiumClientSession::StreamRequest;
@@ -177,6 +196,14 @@ class NET_EXPORT_PRIVATE QuicChromiumClientSession
     QuicVersion quic_version_;
     LoadTimingInfo::ConnectTiming connect_timing_;
     QuicClientPushPromiseIndex* push_promise_index_;
+
+    // |QuicClientPromisedInfo| owns this. It will be set when |Try()|
+    // is asynchronous, i.e. it returned QUIC_PENDING, and remains valid
+    // until |OnRendezvouResult()| fires or |push_handle_->Cancel()| is
+    // invoked.
+    QuicClientPushPromiseIndex::TryHandle* push_handle_;
+    CompletionCallback push_callback_;
+    std::unique_ptr<QuicChromiumClientStream::Handle> push_stream_;
   };
 
   // A helper class used to manage a request to create a stream.

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,22 @@ 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;
-  }
-  return OK;
+  next_state_ = STATE_HANDLE_PROMISE_COMPLETE;
+  return quic_session()->RendezvousWithPromised(
+      request_headers_,
+      base::Bind(&QuicHttpStream::OnIOComplete, weak_factory_.GetWeakPtr()));
 }
 
 int QuicHttpStream::DoHandlePromiseComplete(int rv) {
-  if (rv != OK)
-    return rv;
+  DCHECK_NE(ERR_IO_PENDING, rv);
+  DCHECK_GE(OK, 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 +457,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 +686,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_)

net/quic/chromium/quic_http_stream.h

@@ -35,9 +35,7 @@ class QuicHttpStreamPeer;
 // The QuicHttpStream is a QUIC-specific HttpStream subclass.  It holds a
 // non-owning pointer to a QuicChromiumClientStream which it uses to
 // send and receive data.
-class NET_EXPORT_PRIVATE QuicHttpStream
-    : public QuicClientPushPromiseIndex::Delegate,
-      public MultiplexedHttpStream {
+class NET_EXPORT_PRIVATE QuicHttpStream : public MultiplexedHttpStream {
  public:
   explicit QuicHttpStream(
       std::unique_ptr<QuicChromiumClientSession::Handle> session);
@@ -67,15 +65,6 @@ class NET_EXPORT_PRIVATE QuicHttpStream
   void PopulateNetErrorDetails(NetErrorDetails* details) override;
   void SetPriority(RequestPriority priority) override;
 
-  // QuicClientPushPromiseIndex::Delegate implementation
-  bool CheckVary(const SpdyHeaderBlock& client_request,
-                 const SpdyHeaderBlock& promise_request,
-                 const SpdyHeaderBlock& promise_response) override;
-  // TODO(rch): QuicClientPushPromiseIndex::Delegate is part of shared code.
-  // Figure out how to make the QuicHttpStream receive a Handle in this
-  // case instead of a QuicSpdyStream.
-  void OnRendezvousResult(QuicSpdyStream* stream) override;
-
   static HttpResponseInfo::ConnectionInfo ConnectionInfoFromQuicVersion(
       QuicVersion quic_version);
 
@@ -217,11 +206,6 @@ class NET_EXPORT_PRIVATE QuicHttpStream
   int session_error_;  // Error code from the connection shutdown.
 
   bool found_promise_;
-  // |QuicClientPromisedInfo| owns this. It will be set when |Try()|
-  // is asynchronous, i.e. it returned QUIC_PENDING, and remains valid
-  // until |OnRendezvouResult()| fires or |push_handle_->Cancel()| is
-  // invoked.
-  QuicClientPushPromiseIndex::TryHandle* push_handle_;
 
   // Set to true when DoLoop() is being executed, false otherwise.
   bool in_loop_;

net/quic/chromium/quic_network_transaction_unittest.cc

@@ -4300,6 +4300,77 @@ TEST_P(QuicNetworkTransactionTest, QuicServerPush) {
   EXPECT_LT(0, pos);
 }
 
+// Regression test for http://crbug.com/719461 in which a promised stream
+// is closed before the pushed headers arrive, but after the connection
+// is closed and before the callbacks are executed.
+TEST_P(QuicNetworkTransactionTest, CancelServerPushAfterConnectionClose) {
+  session_params_.origins_to_force_quic_on.insert(
+      HostPortPair::FromString("mail.example.org:443"));
+
+  MockQuicData mock_quic_data;
+  QuicStreamOffset header_stream_offset = 0;
+  // Initial SETTINGS frame.
+  mock_quic_data.AddWrite(
+      ConstructInitialSettingsPacket(1, &header_stream_offset));
+  // First request: GET https://mail.example.org/
+  mock_quic_data.AddWrite(ConstructClientRequestHeadersPacket(
+      2, GetNthClientInitiatedStreamId(0), true, true,
+      GetRequestHeaders("GET", "https", "/"), &header_stream_offset));
+  QuicStreamOffset server_header_offset = 0;
+  // Server promise for: https://mail.example.org/pushed.jpg
+  mock_quic_data.AddRead(ConstructServerPushPromisePacket(
+      1, GetNthClientInitiatedStreamId(0), GetNthServerInitiatedStreamId(0),
+      false, GetRequestHeaders("GET", "https", "/pushed.jpg"),
+      &server_header_offset, &server_maker_));
+  // Response headers for first request.
+  mock_quic_data.AddRead(ConstructServerResponseHeadersPacket(
+      2, GetNthClientInitiatedStreamId(0), false, false,
+      GetResponseHeaders("200 OK"), &server_header_offset));
+  // Client ACKs the response headers.
+  mock_quic_data.AddWrite(ConstructClientAckPacket(3, 2, 1, 1));
+  // Response body for first request.
+  mock_quic_data.AddRead(ConstructServerDataPacket(
+      3, GetNthClientInitiatedStreamId(0), false, true, 0, "hello!"));
+  // Write error for the third request.
+  mock_quic_data.AddWrite(SYNCHRONOUS, ERR_FAILED);
+  mock_quic_data.AddRead(ASYNC, ERR_IO_PENDING);  // No more data to read
+  mock_quic_data.AddRead(ASYNC, 0);               // EOF
+  mock_quic_data.AddSocketDataToFactory(&socket_factory_);
+
+  CreateSession();
+
+  // Send a request which triggers a push promise from the server.
+  SendRequestAndExpectQuicResponse("hello!");
+
+  // Start a push transaction that will be cancelled after the connection
+  // is closed, but before the callback is executed.
+  request_.url = GURL("https://mail.example.org/pushed.jpg");
+  auto trans2 = base::MakeUnique<HttpNetworkTransaction>(DEFAULT_PRIORITY,
+                                                         session_.get());
+  TestCompletionCallback callback2;
+  int rv = trans2->Start(&request_, callback2.callback(), net_log_.bound());
+  EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
+  base::RunLoop().RunUntilIdle();
+
+  // Cause the connection to close on a write error.
+  HttpRequestInfo request3;
+  request3.method = "GET";
+  request3.url = GURL("https://mail.example.org/");
+  request3.load_flags = 0;
+  HttpNetworkTransaction trans3(DEFAULT_PRIORITY, session_.get());
+  TestCompletionCallback callback3;
+  EXPECT_THAT(trans3.Start(&request3, callback3.callback(), net_log_.bound()),
+              IsError(ERR_IO_PENDING));
+
+  base::RunLoop().RunUntilIdle();
+
+  // When |trans2| is destroyed, the underlying stream will be closed.
+  EXPECT_FALSE(callback2.have_result());
+  trans2 = nullptr;
+
+  EXPECT_THAT(callback3.WaitForResult(), IsError(ERR_QUIC_PROTOCOL_ERROR));
+}
+
 TEST_P(QuicNetworkTransactionTest, QuicForceHolBlocking) {
   session_params_.quic_force_hol_blocking = true;
   session_params_.origins_to_force_quic_on.insert(