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..f19928d4fa77e9031503d584adc7badae1c13a9d 100644
--- a/net/quic/quic_in_memory_cache.cc
+++ b/net/quic/quic_in_memory_cache.cc
@@ -7,7 +7,8 @@
 #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 "net/http/http_util.h"
+#include "net/quic/string_piece_utils.h"
 
 using base::FilePath;
 using base::StringPiece;
@@ -21,45 +22,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 +34,53 @@ 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(url.spec());

[wtc, 2014/06/25 22:22:23]

The GetKey method in the original code ignores the http:// or https:// prefix of
the URL. Should we preserve that behavior?

[dmziegler, 2014/06/26 01:23:28]

Done.

   if (it == responses_.end()) {
     return NULL;
   }
   return it->second;
 }
 
-void QuicInMemoryCache::AddSimpleResponse(StringPiece method,
-                                          StringPiece path,
-                                          StringPiece version,
+void QuicInMemoryCache::AddSimpleResponse(StringPiece path,
                                           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",
+  GURL url(path.as_string());

[wtc, 2014/06/25 22:22:23]

1. IMPORTANT: |path| is missing the scheme ("http" or "https") and host. Can you
find out how the GURL constructor supply them?

2. Find out whether we should always prepend "http://", as on line 149.

[dmziegler, 2014/06/26 01:23:28]

Done.

+
+  scoped_refptr<HttpResponseHeaders> response_headers =
+      new HttpResponseHeaders("");

[wtc, 2014/06/25 22:22:23]

1. Please add a TODO comment to add a default HttpResponseHeaders constructor
that allows us to build HTTP response headers more elegantly.

2. Does it work to replace "" with std::string()?

[dmziegler, 2014/06/26 01:23:28]

1. I think this is not necessary anymore.

2. I think it would (but I've also eliminated that).

+  response_headers->ReplaceStatusLine(response_code.as_string() + " " +
+                                      response_detail.as_string() + " " +
+                                      body.as_string());

[wtc, 2014/06/25 22:22:24]

1. BUG: I think this should use |version|, and NOT use |body|. This requires
restoring the |version| parameter.

2. Should we just pass the status line to the HttpResponseHeaders constructor?

[dmziegler, 2014/06/26 01:23:28]

1. Done.

2. Yes, in fact I've passed in the content-length header as well.

+  response_headers->AddHeader(
+      "content-length: " +
       base::Uint64ToString(static_cast<uint64>(body.length())));
 
-  AddResponse(request_headers, response_headers, body);
+  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: " << url.spec();
+  if (ContainsKey(responses_, url.spec())) {
     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_[url.spec()] = 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(path.as_string());
+
+  AddResponse(url, new HttpResponseHeaders(""), "");

[wtc, 2014/06/25 22:22:23]

1. IMPORTANT: do we need to put new HttpResponseHeaders("") inside a
scoped_refptr?

2. IMPORTANT: Is HttpResponseHeaders("") equivalent to a default BalsaHeaders
object in the original code?

[dmziegler, 2014/06/26 01:23:28]

1. This should call the scoped_refptr constructor implicitly.

2. It doesn't really matter, since the HttpResponseHeaders that are passed in
here are never used. I've changed it to NULL.

+  responses_[url.spec()]->response_type_ = response_type;
 }
 
 QuicInMemoryCache::QuicInMemoryCache() {
@@ -147,74 +109,51 @@ void QuicInMemoryCache::Initialize() {
   FilePath file = file_list.Next();
   while (!file.empty()) {

[wtc, 2014/06/25 22:22:23]

I think this should be written as a for loop:

  for (; !file.empty(); file = file_list.Next()) {
  }

[dmziegler, 2014/06/26 01:23:28]

Done.

     // 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();
       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.c_str(),

[wtc, 2014/06/25 22:22:24]

Nit: use data() instead of c_str() if you also supply the string length.

Also fix lines 127 and 153.

[dmziegler, 2014/06/26 01:23:28]

Done.

+                                                   file_contents.size());

[wtc, 2014/06/25 23:17:32]

Nit: use length() instead of size().

+    if (headers_end < 1) {
+      LOG(DFATAL) << "Could not find end of headers in file: " << file.value();

[wtc, 2014/06/25 22:22:23]

1. I think we should have a "continue" here.

2. Why don't you test headers_end == -1 or headers_end < 0?

[dmziegler, 2014/06/26 01:23:28]

1. Ah, I see, LOG(DFATAL) only FATALs in debug mode.

2. I copied this from other pieces of code that used
HttpUtil::LocateEndOfHeaders -- if the headers are empty, then that also means
something is wrong. The error message now makes this clear.

     }
 
-    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.c_str(), headers_end);
 
-    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.
+    scoped_refptr<HttpResponseHeaders> response_headers =
+        new HttpResponseHeaders(raw_headers);
+
+    string base;
+    if (response_headers->HasHeader("X-Original-Url")) {
+      if (!response_headers->GetNormalizedHeader("X-Original-Url", &base)) {

[wtc, 2014/06/25 22:22:23]

I think you can replace the nested if statements with just a
response_headers->GetNormalizedHeader call:

  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.
    ...
  } else {
    base = file.AsUTF8Unsafe();
  }

GetNormalizedHeader returns false if the header is not found, so we don't need a
separate HasHeader check.

[dmziegler, 2014/06/26 01:23:28]

Done.

+        LOG(DFATAL) << "Failed to read X-Original-Url in file: "
+                    << file.value();
+      }
+      response_headers->RemoveHeader("X-Original-Url");
+      // Remove the protocol so we can add it below.
       if (StringPieceUtils::StartsWithIgnoreCase(base, "https://")) {
-        base.remove_prefix(8);
+        base = base.substr(8);
       } else if (StringPieceUtils::StartsWithIgnoreCase(base, "http://")) {
-        base.remove_prefix(7);
+        base = base.substr(7);
       }
+    } else {
+      base = file.AsUTF8Unsafe();

[wtc, 2014/06/25 23:17:32]

1. We should make sure this is an absolute pathname.

2. We should find out if this starts with '/', even on Windows. So the drive
letter such as C: needs to be escaped.

[dmziegler, 2014/06/26 01:23:28]

I don't think it's supposed to be absolute. The DCHECK in the old code makes
sure that the first '/' is *not* the first character. When I've tested this, it
has just generated urls like "http://relative/path/to/cache/index.html". I've
made an equivalent DCHECK in the new code.

     }
-    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);

[wtc, 2014/06/25 23:17:32]

Should we also remove the trailing ',' in the file path?

[dmziegler, 2014/06/26 01:23:28]

Oh yes.

-    }
-    // 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)
-            << "' into QuicInMemoryCache.";
+    GURL url("http://" + base);
 
-    AddResponse(request_headers, response_headers, caching_visitor.body());
+    VLOG(1) << "Inserting '" << url.spec() << "' into QuicInMemoryCache.";
+
+    StringPiece body(file_contents.c_str() + headers_end,
+                     file_contents.size() - headers_end);
+
+    AddResponse(url, response_headers, body);
 
     file = file_list.Next();
   }
@@ -224,20 +163,4 @@ 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();
-}
-
 }  // namespace net