QuicServer: Use Chrome's header parsing rather than Balsa

net/quic/quic_in_memory_cache.cc

Index: net/quic/quic_in_memory_cache.cc
diff --git a/net/quic/quic_in_memory_cache.cc b/net/quic/quic_in_memory_cache.cc
index 9d40fa50667cce9635edb12d620f4fc7da068823..84f68f41e6380b13b659866705e00388df655ba6 100644
--- a/net/quic/quic_in_memory_cache.cc
+++ b/net/quic/quic_in_memory_cache.cc
@@ -7,7 +7,9 @@
 #include "base/files/file_enumerator.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
-#include "net/tools/balsa/balsa_headers.h"
+#include "base/strings/string_util.h"
+#include "net/http/http_util.h"
+#include "url/gurl.h"
 
 using base::FilePath;
 using base::StringPiece;
@@ -21,45 +23,11 @@ namespace net {
 
 FilePath::StringType g_quic_in_memory_cache_dir = FILE_PATH_LITERAL("");
 
-namespace {
-
-// BalsaVisitor implementation (glue) which caches response bodies.
-class CachingBalsaVisitor : public NoOpBalsaVisitor {
- public:
-  CachingBalsaVisitor() : done_framing_(false) {}
-  virtual void ProcessBodyData(const char* input, size_t size) OVERRIDE {
-    AppendToBody(input, size);
-  }
-  virtual void MessageDone() OVERRIDE {
-    done_framing_ = true;
-  }
-  virtual void HandleHeaderError(BalsaFrame* framer) OVERRIDE {
-    UnhandledError();
-  }
-  virtual void HandleHeaderWarning(BalsaFrame* framer) OVERRIDE {
-    UnhandledError();
-  }
-  virtual void HandleChunkingError(BalsaFrame* framer) OVERRIDE {
-    UnhandledError();
-  }
-  virtual void HandleBodyError(BalsaFrame* framer) OVERRIDE {
-    UnhandledError();
-  }
-  void UnhandledError() {
-    LOG(DFATAL) << "Unhandled error framing HTTP.";
-  }
-  void AppendToBody(const char* input, size_t size) {
-    body_.append(input, size);
-  }
-  bool done_framing() const { return done_framing_; }
-  const string& body() const { return body_; }
-
- private:
-  bool done_framing_;
-  string body_;
-};
+QuicInMemoryCache::Response::Response() : response_type_(REGULAR_RESPONSE) {
+}
 
-}  // namespace
+QuicInMemoryCache::Response::~Response() {
+}
 
 // static
 QuicInMemoryCache* QuicInMemoryCache::GetInstance() {
@@ -67,58 +35,55 @@ QuicInMemoryCache* QuicInMemoryCache::GetInstance() {
 }
 
 const QuicInMemoryCache::Response* QuicInMemoryCache::GetResponse(
-    const BalsaHeaders& request_headers) const {
-  ResponseMap::const_iterator it = responses_.find(GetKey(request_headers));
+    const GURL& url) const {
+  ResponseMap::const_iterator it = responses_.find(GetKey(url));
   if (it == responses_.end()) {
     return NULL;
   }
   return it->second;
 }
 
-void QuicInMemoryCache::AddSimpleResponse(StringPiece method,
-                                          StringPiece path,
+void QuicInMemoryCache::AddSimpleResponse(StringPiece path,
                                           StringPiece version,
                                           StringPiece response_code,
                                           StringPiece response_detail,
                                           StringPiece body) {
-  BalsaHeaders request_headers, response_headers;
-  request_headers.SetRequestFirstlineFromStringPieces(method,
-                                                      path,
-                                                      version);
-  response_headers.SetRequestFirstlineFromStringPieces(version,
-                                                       response_code,
-                                                       response_detail);
-  response_headers.AppendHeader(
-      "content-length",
-      base::Uint64ToString(static_cast<uint64>(body.length())));
-
-  AddResponse(request_headers, response_headers, body);
+  GURL url("http://" + path.as_string());

[wtc, 2014/06/26 21:23:17]

|url| doesn't have a |host| component. Is that OK?

[dmziegler, 2014/06/27 01:11:37]

Yes, that would be included in the path string.

+
+  string status_line = version.as_string() + " " +
+                       response_code.as_string() + " " +
+                       response_detail.as_string();
+
+  string header = "content-length: " +
+                  base::Uint64ToString(static_cast<uint64>(body.length()));
+
+  scoped_refptr<HttpResponseHeaders> response_headers =
+      new HttpResponseHeaders(status_line + '\0' + header + '\0' + '\0');
+
+  AddResponse(url, response_headers, body);
 }
 
-void QuicInMemoryCache::AddResponse(const BalsaHeaders& request_headers,
-                                    const BalsaHeaders& response_headers,
-                                    StringPiece response_body) {
-  VLOG(1) << "Adding response for: " << GetKey(request_headers);
-  if (ContainsKey(responses_, GetKey(request_headers))) {
+void QuicInMemoryCache::AddResponse(
+    const GURL& url,
+    scoped_refptr<HttpResponseHeaders> response_headers,
+    StringPiece response_body) {
+  VLOG(1) << "Adding response for: " << GetKey(url);

[wtc, 2014/06/26 21:23:17]

Nit: we should store the return value of GetKey(url) in a local variable, rather
than calling it three times.

[dmziegler, 2014/06/27 01:11:37]

Done.

+  if (ContainsKey(responses_, GetKey(url))) {
     LOG(DFATAL) << "Response for given request already exists!";
     return;
   }
   Response* new_response = new Response();
   new_response->set_headers(response_headers);
   new_response->set_body(response_body);
-  responses_[GetKey(request_headers)] = new_response;
+  responses_[GetKey(url)] = new_response;
 }
 
-void QuicInMemoryCache::AddSpecialResponse(StringPiece method,
-                                           StringPiece path,
-                                           StringPiece version,
+void QuicInMemoryCache::AddSpecialResponse(StringPiece path,
                                            SpecialResponseType response_type) {
-  BalsaHeaders request_headers, response_headers;
-  request_headers.SetRequestFirstlineFromStringPieces(method,
-                                                      path,
-                                                      version);
-  AddResponse(request_headers, response_headers, "");
-  responses_[GetKey(request_headers)]->response_type_ = response_type;
+  GURL url("http://" + path.as_string());
+
+  AddResponse(url, NULL, string());

[wtc, 2014/06/26 21:23:17]

I seem to remember it is necessary to pass scoped_refptr<>() rather than NULL...

[dmziegler, 2014/06/27 01:11:37]

This implicitly calls the scoped_refptr<>() constructor with NULL. NULL is a
valid value of a scoped_refptr. If you like, I can make this more explicit --
it's now not allowed to call .headers() on the Response object, since that would
try to dereference the NULL pointer.

[wtc, 2014/06/27 22:08:04]

Ah, I finally realized what you meant by this.

Should we change headers() to return a const, plain pointer?

  const HttpResponseHeaders* headers() const { return headers_.get(); }

Or should we pass a pointer to an empty HttpResponseHeaders object here?

  scoped_refptr<HttpResponseHeaders> headers = new HttpResponseHeaders("");

  AddResponse(url, headers, string());

[dmziegler, 2014/07/01 18:23:39]

A const pointer looks like the way to go. Will do in cleanup commit.

+  responses_[GetKey(url)]->response_type_ = response_type;
 }
 
 QuicInMemoryCache::QuicInMemoryCache() {
@@ -145,78 +110,55 @@ void QuicInMemoryCache::Initialize() {
                                  base::FileEnumerator::FILES);
 
   FilePath file = file_list.Next();
-  while (!file.empty()) {
+  for (; !file.empty(); file = file_list.Next()) {
     // Need to skip files in .svn directories
-    if (file.value().find(FILE_PATH_LITERAL("/.svn/")) != std::string::npos) {
+    if (file.value().find(FILE_PATH_LITERAL("/.svn/")) != string::npos) {
       file = file_list.Next();

[wtc, 2014/06/26 21:23:16]

IMPORTANT: delete this line (now done by the for loop).

[dmziegler, 2014/06/27 01:11:37]

Done.

       continue;
     }
 
-    BalsaHeaders request_headers, response_headers;
-
     string file_contents;
     base::ReadFileToString(file, &file_contents);
 
-    // Frame HTTP.
-    CachingBalsaVisitor caching_visitor;
-    BalsaFrame framer;
-    framer.set_balsa_headers(&response_headers);
-    framer.set_balsa_visitor(&caching_visitor);
-    size_t processed = 0;
-    while (processed < file_contents.length() &&
-           !caching_visitor.done_framing()) {
-      processed += framer.ProcessInput(file_contents.c_str() + processed,
-                                       file_contents.length() - processed);
+    int headers_end = HttpUtil::LocateEndOfHeaders(file_contents.data(),
+                                                   file_contents.size());
+    if (headers_end < 1) {
+      LOG(DFATAL) << "Headers invalid or empty, ignoring: " << file.value();
+      continue;
     }
 
-    string response_headers_str;
-    response_headers.DumpToString(&response_headers_str);
-    if (!caching_visitor.done_framing()) {
-      LOG(DFATAL) << "Did not frame entire message from file: " << file.value()
-                  << " (" << processed << " of " << file_contents.length()
-                  << " bytes).";
-    }
-    if (processed < file_contents.length()) {
-      // Didn't frame whole file. Assume remainder is body.
-      // This sometimes happens as a result of incompatibilities between
-      // BalsaFramer and wget's serialization of HTTP sans content-length.
-      caching_visitor.AppendToBody(file_contents.c_str() + processed,
-                                   file_contents.length() - processed);
-      processed += file_contents.length();
-    }
+    string raw_headers =
+        HttpUtil::AssembleRawHeaders(file_contents.data(), headers_end);
+
+    scoped_refptr<HttpResponseHeaders> response_headers =
+        new HttpResponseHeaders(raw_headers);
 
-    string utf8_file = file.AsUTF8Unsafe();
-    StringPiece base = utf8_file;
-    if (response_headers.HasHeader("X-Original-Url")) {
-      base = response_headers.GetHeader("X-Original-Url");
-      response_headers.RemoveAllOfHeader("X-Original-Url");
-      // Remove the protocol so that the string is of the form host + path,
-      // which is parsed properly below.
-      if (StringPieceUtils::StartsWithIgnoreCase(base, "https://")) {
-        base.remove_prefix(8);
-      } else if (StringPieceUtils::StartsWithIgnoreCase(base, "http://")) {
-        base.remove_prefix(7);
+    string base;
+    if (response_headers->GetNormalizedHeader("X-Original-Url", &base)) {
+      response_headers->RemoveHeader("X-Original-Url");
+      // Remove the protocol so we can add it below.
+      if (StartsWithASCII(base, "https://", false)) {
+        base = base.substr(8);
+      } else if (StartsWithASCII(base, "http://", false)) {
+        base = base.substr(7);
       }
+    } else {
+      base = file.AsUTF8Unsafe();

[wtc, 2014/06/26 21:23:17]

If this generates a URL like "http://relative/path/to/cache/index.html", that
has a syntax error -- it is missing a '/' after "http://".

[dmziegler, 2014/06/27 01:11:37]

Actually, it's fine the way it is. "relative" just becomes the hostname. (It's
perfectly valid for hostnames to not have normal TLD part like ".com" -- you
just don't get hostnames like that from a public DNS server.)

     }
-    size_t path_start = base.find_first_of('/');
-    DCHECK_LT(0U, path_start);
-    StringPiece host(base.substr(0, path_start));
-    StringPiece path(base.substr(path_start));
-    if (path[path.length() - 1] == ',') {
-      path.remove_suffix(1);
+    DCHECK(base.length() > 0 && base[0] != '/');
+    if (base[base.length() - 1] == ',') {

[wtc, 2014/06/26 21:23:17]

We should test !base.empty(), otherwise the release builds may do base[-1].

[dmziegler, 2014/06/27 01:11:37]

Since that really shouldn't happen, I've just made the DCHECK an actual check.

+      base = base.substr(0, base.length() - 1);
     }
-    // Set up request headers. Assume method is GET and protocol is HTTP/1.1.
-    request_headers.SetRequestFirstlineFromStringPieces("GET",
-                                                        path,
-                                                        "HTTP/1.1");
-    request_headers.ReplaceOrAppendHeader("host", host);
 
-    VLOG(1) << "Inserting 'http://" << GetKey(request_headers)
+    GURL url("http://" + base);
+
+    VLOG(1) << "Inserting 'http://" << GetKey(url)

[wtc, 2014/06/26 21:23:17]

Nit: we probably should omit "http://" in this log message.

[dmziegler, 2014/06/27 01:11:37]

Done.

             << "' into QuicInMemoryCache.";
 
-    AddResponse(request_headers, response_headers, caching_visitor.body());
+    StringPiece body(file_contents.data() + headers_end,
+                     file_contents.size() - headers_end);
 
-    file = file_list.Next();
+    AddResponse(url, response_headers, body);
   }
 }
 
@@ -224,20 +166,9 @@ QuicInMemoryCache::~QuicInMemoryCache() {
   STLDeleteValues(&responses_);
 }
 
-string QuicInMemoryCache::GetKey(const BalsaHeaders& request_headers) const {
-  StringPiece uri = request_headers.request_uri();
-  if (uri.size() == 0) {
-    return "";
-  }
-  StringPiece host;
-  if (uri[0] == '/') {
-    host = request_headers.GetHeader("host");
-  } else if (StringPieceUtils::StartsWithIgnoreCase(uri, "https://")) {
-    uri.remove_prefix(8);
-  } else if (StringPieceUtils::StartsWithIgnoreCase(uri, "http://")) {
-    uri.remove_prefix(7);
-  }
-  return host.as_string() + uri.as_string();
+string QuicInMemoryCache::GetKey(const GURL& url) const {
+  // Take everything but the scheme portion of the URL.
+  return url.host() + url.PathForRequest();

[wtc, 2014/06/26 21:23:17]

Do we need a '/' in between?

I seem to remember there is a better way to omit the scheme part of a GURL. It
seems to have "Components" in the name.

[dmziegler, 2014/06/27 01:11:37]

No, PathForRequest() includes a slash. And I can't find a better way -- where
should it be?

[wtc, 2014/06/27 21:00:38]

If url.PathForRequest() starts with a slash, then that's fine. The slash should
separate the host and the path.

I just checked the URI RFC:
http://tools.ietf.org/html/rfc3986#section-3

The example shows that the path part includes the initial slash. So I think your
code is correct.

 }
 
 }  // namespace net