QUIC - chromium server push support.

net/quic/quic_http_stream.cc

Index: net/quic/quic_http_stream.cc
diff --git a/net/quic/quic_http_stream.cc b/net/quic/quic_http_stream.cc
index b355b134bb37cf258ff423dfbf54e8923af12dc2..40071e8725a7e9156cac9c640457759490df7c50 100644
--- a/net/quic/quic_http_stream.cc
+++ b/net/quic/quic_http_stream.cc
@@ -6,6 +6,7 @@
 
 #include "base/callback_helpers.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
@@ -13,6 +14,7 @@
 #include "net/http/http_util.h"
 #include "net/quic/quic_chromium_client_session.h"
 #include "net/quic/quic_chromium_client_stream.h"
+#include "net/quic/quic_client_promised_info.h"
 #include "net/quic/quic_http_utils.h"
 #include "net/quic/quic_utils.h"
 #include "net/quic/spdy_utils.h"
@@ -43,6 +45,8 @@ QuicHttpStream::QuicHttpStream(
       closed_stream_sent_bytes_(0),
       user_buffer_len_(0),
       quic_connection_error_(QUIC_NO_ERROR),
+      found_promise_(false),
+      push_handle_(nullptr),
       weak_factory_(this) {
   DCHECK(session_);
   session_->AddObserver(this);
@@ -54,6 +58,71 @@ QuicHttpStream::~QuicHttpStream() {
     session_->RemoveObserver(this);
 }
 
+void QuicHttpStream::ConvertHeaderBlockToHttpRequestHeaders(
+    const SpdyHeaderBlock& spdy_headers,
+    HttpRequestHeaders* http_headers) {
+  for (auto it : spdy_headers) {
+    StringPiece key = it.first;
+    if (key[0] == ':') {
+      key.remove_prefix(1);
+    }
+    std::vector<StringPiece> values = base::SplitStringPiece(
+        it.second, "\0", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+    for (auto value : values) {
+      http_headers->SetHeader(key, value);
+    }
+  }
+}
+
+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, HTTP2,
+                                 &promise_response_info)) {
+    DLOG(WARNING) << "Invalid headers";
+    return false;
+  }
+
+  HttpVaryData vary_data;
+  if (vary_data.Init(promise_request_info,
+                     *promise_response_info.headers.get())) {
+    // Now compare the client request for matching.
+    return vary_data.MatchesRequest(client_request_info,
+                                    *promise_response_info.headers.get());
+  } else {
+    // Promise didn't contain valid vary info, so URL match was sufficient.
+    return true;

[Ryan Hamilton, 2016/02/24 00:34:00]

nit: early return

if (!vary_data.Init(...)) {
  // 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

Or you could be really clever and do:
  return !var_data.Init() || vary_data.Match()
but I suspect that won't be terribly readable

[Buck, 2016/02/26 23:54:16]

Done.

+  }
+}
+
+void QuicHttpStream::OnRendezvousResult(QuicSpdyStream* stream) {
+  push_handle_ = nullptr;
+  if (stream) {
+    stream_ = static_cast<QuicChromiumClientStream*>(stream);
+    stream_->SetDelegate(this);
+  }
+  if (!callback_.is_null()) {

[Ryan Hamilton, 2016/02/24 00:34:00]

Can you comment about under what circumstances will the callback be non-null?

[Buck, 2016/02/26 23:54:16]

Done.

+    // Try returned QUIC_PENDING
+    if (stream) {
+      next_state_ = STATE_OPEN;
+      DoCallback(OK);

[Ryan Hamilton, 2016/02/24 00:34:00]

Instead of DoCallback, can you simply do OnIOComplete(OK)? Does that work? The
asymetry between these two paths feel tingly but it might be totally fine.

[Buck, 2016/02/26 23:54:16]

No, because OnIoComplete() calls DoLoop() which toggles next_state_ back from
STATE_OPEN to STATE_NONE, and later code, namely IsResponseComplete(), wants to
see STATE_OPEN.

Unless maybe... change DoLoop to be a no-op on next_state_ == STATE_OPEN?

+      return;
+    }
+    // rendezvous has failed so proceed as with a non-push request.
+    next_state_ = STATE_STREAM_REQUEST;
+    OnIOComplete(OK);
+  }
+}
+
 int QuicHttpStream::InitializeStream(const HttpRequestInfo* request_info,
                                      RequestPriority priority,
                                      const BoundNetLog& stream_net_log,
@@ -76,29 +145,85 @@ int QuicHttpStream::InitializeStream(const HttpRequestInfo* request_info,
   DCHECK(success);
   DCHECK(ssl_info_.cert.get());
 
+  if (session_->push_promise_index()->GetPromised(request_info->url.spec())) {
+    found_promise_ = true;
+    return OK;

[Ryan Hamilton, 2016/02/24 00:34:00]

Can you add a comment here which explains that the push will be coming later (or
some such)

[Buck, 2016/02/26 23:54:16]

Done.

+  }
+
+  next_state_ = STATE_STREAM_REQUEST;
+  int rv = DoLoop(OK);
+  if (rv == ERR_IO_PENDING)
+    callback_ = callback;
+
+  return rv;
+}
+
+int QuicHttpStream::DoStreamRequest() {
   int rv = stream_request_.StartRequest(
       session_, &stream_,
       base::Bind(&QuicHttpStream::OnStreamReady, weak_factory_.GetWeakPtr()));
-  if (rv == ERR_IO_PENDING) {
-    callback_ = callback;
-  } else if (rv == OK) {
+  if (rv == OK) {
     stream_->SetDelegate(this);
-  } else if (!was_handshake_confirmed_) {
+    if (response_info_) {
+      EnterStateSendHeaders();

[Ryan Hamilton, 2016/02/24 00:34:00]

I think it would be more common to rename EnterStateSendHeaders to
 DoStreamRequestComplete
and make it an explicit state.

+    }
+  } else if (rv != ERR_IO_PENDING && !was_handshake_confirmed_) {
     rv = ERR_QUIC_HANDSHAKE_FAILED;
   }
-
   return rv;
 }
 
+void QuicHttpStream::EnterStateSendHeaders() {
+  // Only advance to STATE_SEND_HEADERS if the stream is ready and
+  // SendRequest has been called (i.e. if response_info_ is a
+  // non-null value, so priority_ etc. are also set).

[Ryan Hamilton, 2016/02/24 00:34:00]

This comment implies some conditional logic which I don't seem to see. Does that
logic live elsewhere?

[Buck, 2016/02/26 23:54:16]

I updated the comment.   [ The conditional logic *was* here in a pre-review
version, but I later lifted it out to the callsites.  ]

+  DCHECK(stream_);
+  DCHECK(response_info_);
+  SpdyPriority priority = ConvertRequestPriorityToQuicPriority(priority_);
+  stream_->SetPriority(priority);
+  next_state_ = STATE_SEND_HEADERS;
+}
+
 void QuicHttpStream::OnStreamReady(int rv) {
   DCHECK(rv == OK || !stream_);
   if (rv == OK) {
     stream_->SetDelegate(this);
+    if (response_info_) {
+      EnterStateSendHeaders();
+      rv = DoLoop(OK);
+    }

[Ryan Hamilton, 2016/02/24 00:34:00]

My brain is working slowly. What circumstances lead to this new path?

[Buck, 2016/02/26 23:54:16]

Added a comment to explain.

   } else if (!was_handshake_confirmed_) {
     rv = ERR_QUIC_HANDSHAKE_FAILED;
   }
+  if (rv != ERR_IO_PENDING) {
+    DoCallback(rv);
+  }
+}
 
-  base::ResetAndReturn(&callback_).Run(rv);
+int QuicHttpStream::HandlePromise() {
+  QuicAsyncStatus push_status = session_->push_promise_index()->Try(
+      request_headers_, this, &this->push_handle_);
+  if (push_status == QUIC_FAILURE) {

[Ryan Hamilton, 2016/02/24 00:34:00]

nit: consider a switch statement here.

[Buck, 2016/02/26 23:54:16]

Done.

+    // Push rendezvous failed.
+    next_state_ = STATE_STREAM_REQUEST;
+  } else if (request_body_stream_) {
+    // Method type or request with body ineligble for push.
+    this->push_handle_->Cancel();
+    this->push_handle_ = nullptr;
+    next_state_ = STATE_STREAM_REQUEST;
+  } else if (push_status == QUIC_SUCCESS) {
+    next_state_ = STATE_OPEN;
+  } else {
+    DCHECK(push_status == QUIC_PENDING);
+    // Have a promise but the promised stream doesn't exist yet.
+    // Still have to do validation before accepting the promised
+    // stream for sure.
+    return ERR_IO_PENDING;
+  }
+  if (next_state_ != STATE_OPEN) {

[Ryan Hamilton, 2016/02/24 00:34:00]

How 'bout avoiding this conditional by doing a return; in the QUIC_SUCCESS case?
(And add a comment to explain why)

[Buck, 2016/02/26 23:54:16]

Done.

+    return DoLoop(OK);
+  }
+  return OK;
 }
 
 int QuicHttpStream::SendRequest(const HttpRequestHeaders& request_headers,
@@ -117,12 +242,14 @@ int QuicHttpStream::SendRequest(const HttpRequestHeaders& request_headers,
     UMA_HISTOGRAM_BOOLEAN("Net.QuicSession.CookieSentToAccountsOverChannelId",
                           ssl_info_.channel_id_sent);
   }
-  if (!stream_) {
-    return ERR_CONNECTION_CLOSED;
+  if (!found_promise_) {
+    if (!stream_)
+      return ERR_CONNECTION_CLOSED;

[Ryan Hamilton, 2016/02/24 00:34:00]

I wonder if this should have the same logic as below?

if (!session_ || (!found_promise! && !stream_)) {
  return was_handshake_confirmed_ ? ERR_CONNECTION_CLOSED
                                                             :
ERR_QUIC_HANDSHAKE_FAILED;

but I'm not sure.

[Buck, 2016/02/26 23:54:16]

Done.

+  } else if (!session_) {
+    return was_handshake_confirmed_ ? ERR_CONNECTION_CLOSED
+                                    : ERR_QUIC_HANDSHAKE_FAILED;
   }
 
-  SpdyPriority priority = ConvertRequestPriorityToQuicPriority(priority_);
-  stream_->SetPriority(priority);
   // Store the serialized request headers.
   CreateSpdyHeadersFromHttpRequest(*request_info_, request_headers, HTTP2,
                                    /*direct=*/true, &request_headers_);
@@ -146,8 +273,15 @@ int QuicHttpStream::SendRequest(const HttpRequestHeaders& request_headers,
   // Store the response info.
   response_info_ = response;
 
-  next_state_ = STATE_SEND_HEADERS;
-  int rv = DoLoop(OK);
+  int rv;
+
+  if (found_promise_) {
+    rv = HandlePromise();
+  } else {
+    EnterStateSendHeaders();
+    rv = DoLoop(OK);
+  }
+
   if (rv == ERR_IO_PENDING)
     callback_ = callback;
 
@@ -390,6 +524,9 @@ int QuicHttpStream::DoLoop(int rv) {
     State state = next_state_;
     next_state_ = STATE_NONE;
     switch (state) {
+      case STATE_STREAM_REQUEST:
+        rv = DoStreamRequest();
+        break;
       case STATE_SEND_HEADERS:
         CHECK_EQ(OK, rv);
         rv = DoSendHeaders();
@@ -585,6 +722,10 @@ void QuicHttpStream::ResetStream() {
     // |stream_| is going away when Read is called. Should never happen??
     CHECK(false);
   }
+  if (push_handle_) {
+    push_handle_->Cancel();
+    push_handle_ = nullptr;
+  }
   if (!stream_)
     return;
   closed_stream_received_bytes_ = stream_->stream_bytes_read();