Better header value parsing for Server-Timing.

third_party/WebKit/Source/platform/network/HTTPParsers.cpp

Index: third_party/WebKit/Source/platform/network/HTTPParsers.cpp
diff --git a/third_party/WebKit/Source/platform/network/HTTPParsers.cpp b/third_party/WebKit/Source/platform/network/HTTPParsers.cpp
index e0ee06a2f6ca87eaa8264a91b6c0711b21ea5d3e..3a7e91bfabd301cb8fa24a2a347810a5a33e3a2e 100644
--- a/third_party/WebKit/Source/platform/network/HTTPParsers.cpp
+++ b/third_party/WebKit/Source/platform/network/HTTPParsers.cpp
@@ -38,6 +38,7 @@
 #include "platform/HTTPNames.h"
 #include "platform/json/JSONParser.h"
 #include "platform/loader/fetch/ResourceResponse.h"
+#include "platform/network/HeaderFieldTokenizer.h"
 #include "platform/weborigin/Suborigin.h"
 #include "platform/wtf/DateMath.h"
 #include "platform/wtf/MathExtras.h"
@@ -864,41 +865,6 @@ bool ParseContentRangeHeaderFor206(const String& content_range,
       last_byte_position, instance_length);
 }
 
-template <typename CharType>
-inline bool IsNotServerTimingHeaderDelimiter(CharType c) {
-  return c != '=' && c != ';' && c != ',';
-}
-
-const LChar* ParseServerTimingToken(const LChar* begin,
-                                    const LChar* end,
-                                    String& result) {
-  const LChar* position = begin;
-  skipWhile<LChar, IsNotServerTimingHeaderDelimiter>(position, end);
-  result = String(begin, position - begin).StripWhiteSpace();
-  return position;
-}
-
-String CheckDoubleQuotedString(const String& value) {
-  if (value.length() < 2 || value[0] != '"' ||
-      value[value.length() - 1] != '"') {
-    return value;
-  }
-
-  StringBuilder out;
-  unsigned pos = 1;                   // Begin after the opening DQUOTE.
-  unsigned len = value.length() - 1;  // End before the closing DQUOTE.
-
-  // Skip past backslashes, but include everything else.
-  while (pos < len) {
-    if (value[pos] == '\\')
-      pos++;
-    if (pos < len)
-      out.Append(value[pos++]);
-  }
-
-  return out.ToString();
-}
-
 std::unique_ptr<ServerTimingHeaderVector> ParseServerTimingHeader(
     const String& headerValue) {
   std::unique_ptr<ServerTimingHeaderVector> headers =
@@ -907,24 +873,63 @@ std::unique_ptr<ServerTimingHeaderVector> ParseServerTimingHeader(
   if (!headerValue.IsNull()) {
     DCHECK(headerValue.Is8Bit());
 
-    const LChar* position = headerValue.Characters8();
-    const LChar* end = position + headerValue.length();
-    while (position < end) {
-      String metric, value, description = "";
-      position = ParseServerTimingToken(position, end, metric);
-      if (position != end && *position == '=') {
-        position = ParseServerTimingToken(position + 1, end, value);
+    unsigned index = 0;
+    HeaderFieldTokenizer tokenizer(headerValue);

[Yoav Weiss, 2017/06/08 06:07:55]

Should we call SkipSpaces() here before consuming our first token? (as
ConsumeToken() seems to skip spaces after a token but not before it)

[e_hakkinen, 2017/06/08 07:35:24]

No, the HeaderFieldTokenizer ctor does that. Also, SkipSpaces is private and
cannot thus be called here.
It does not have to skip spaces before a token, because HeaderFieldTokenizer
ensures that it is always in a state where all preceding spaces are already been
skipped.

[Yoav Weiss, 2017/06/08 07:47:48]

OK, cool. missed the ctor bit

+    while (index < headerValue.length()) {

[e_hakkinen, 2017/06/08 07:35:24]

This should be
         while (!tokenizer.IsConsumed()) {
(the code works with a wrong while condition because the if block on lines
880-882 will eventually stop the loop).

+      StringView metric;
+      if (!tokenizer.ConsumeToken(Mode::kNormal, metric)) {
+        break;
       }
-      if (position != end && *position == ';') {
-        position = ParseServerTimingToken(position + 1, end, description);
+
+      StringView duration;
+      String description;
+      if (tokenizer.Consume('=')) {
+        tokenizer.ConsumeToken(Mode::kNormal, duration);

[Yoav Weiss, 2017/06/08 06:07:55]

Can you call ToDouble here only if there is a duration? (so if ConsumeToken did
not fail?) That way we're not allocating a String a calling a long chain of
functions in the potentially-common case where duration is not specified.

+      }
+      if (tokenizer.Consume(';')) {
+        tokenizer.ConsumeTokenOrQuotedString(Mode::kNormal, description);
       }
-      position++;
 
       headers->push_back(WTF::MakeUnique<ServerTimingHeader>(
-          metric, value.ToDouble(), CheckDoubleQuotedString(description)));
+          metric.ToString(), duration.ToString().ToDouble(),

[Yoav Weiss, 2017/06/08 06:07:55]

RE "duration.ToString().ToDouble()", we could have avoided the string allocation
here but it will result in significantly less readable code. Let's leave it as
is

+          description ? description : ""));
+
+      if (!tokenizer.Consume(',')) {
+        break;
+      }
     }
   }
   return headers;
 }
 
+bool IsTokenCharacter(Mode mode, UChar c) {

[Yoav Weiss, 2017/06/08 06:07:55]

As the same function defined in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo...
and this is not used here, I'm assuming this is a leftover from a previous patch

+  if (c >= 128)
+    return false;
+  if (c < 0x20)
+    return false;
+
+  switch (c) {
+    case ' ':
+    case ';':
+    case '"':
+      return false;
+    case '(':
+    case ')':
+    case '<':
+    case '>':
+    case '@':
+    case ',':
+    case ':':
+    case '\\':
+    case '/':
+    case '[':
+    case ']':
+    case '?':
+    case '=':
+      return mode == Mode::kRelaxed;
+    default:
+      return true;
+  }
+}
+
 }  // namespace blink