Added UMA measurement for how often we see control characters during cookie parsing.

net/cookies/parsed_cookie.cc

Index: net/cookies/parsed_cookie.cc
diff --git a/net/cookies/parsed_cookie.cc b/net/cookies/parsed_cookie.cc
index 8d382152b32c7e12e2df45834521c4bb39c56111..a39ec84dab909e7036f0b40ec803020e2310327c 100644
--- a/net/cookies/parsed_cookie.cc
+++ b/net/cookies/parsed_cookie.cc
@@ -1,4 +1,3 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[erikwright (departed), 2013/05/24 14:15:56]

Was this removed intentionally?

[jww, 2013/05/24 17:56:33]

Nope, not at all. Fixed.
On 2013/05/24 14:15:56, erikwright wrote:

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -45,6 +44,7 @@
 #include "net/cookies/parsed_cookie.h"
 
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
 #include "base/string_util.h"
 
 namespace {
@@ -130,9 +130,12 @@ bool IsValidCookieValue(const std::string& value) {
          (*i >= 0x2D && *i <= 0x3A) ||
          (*i >= 0x3C && *i <= 0x5B) ||
          (*i >= 0x5D && *i <= 0x7E));
-    if (!valid_octet)
+    if (!valid_octet) {
+      UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieValue", false);

[erikwright (departed), 2013/05/24 14:15:56]

This will be called for changing the value but not changing the name via the
setters.

Consider moving it to SetName and SetValue instead.

[jww, 2013/05/24 17:56:33]

Done.

       return false;
+    }
   }
+  UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieValue", true);
   return true;
 }
 
@@ -140,9 +143,12 @@ bool IsValidCookieAttributeValue(const std::string& value) {
   // The greatest common denominator of cookie attribute values is
   // <any CHAR except CTLs or ";"> according to RFC 6265.
   for (std::string::const_iterator i = value.begin(); i != value.end(); ++i) {
-    if ((*i >= 0 && *i <= 31) || *i == ';')
+    if ((*i >= 0 && *i <= 31) || *i == ';') {
+      UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieAttributeValue", false);

[erikwright (departed), 2013/05/24 14:15:56]

These metrics will be clouded because they cover both changes to attributes via
the accessors and the parsing of a cookie-line (both value and attributes)
during the constructor.

Consider moving them to SetAttributePair instead.

[jww, 2013/05/24 17:56:33]

The goal of this measurement is to get an overall view of how often there are
cookie values that are rejected because of control characters

On a related note, in SetAttributePair, it seems like the IsValidAttrbitueValue
is a redundant call because IsValidToken includes a check for control
characters. Am I misunderstanding something there? I'm actually going to add a
check in the cookie line parser to check how often parsed cookies are not valid
tokens either.
On 2013/05/24 14:15:56, erikwright wrote:

[erikwright (departed), 2013/05/27 19:19:41]

The key is checked with IsValidToken and the value is checked against
IsValidAttributeValue.

       return false;
+    }
   }
+  UMA_HISTOGRAM_BOOLEAN("Cookies.IsValidCookieAttributeValue", true);
   return true;
 }
 
@@ -157,7 +163,8 @@ ParsedCookie::ParsedCookie(const std::string& cookie_line)
       maxage_index_(0),
       secure_index_(0),
       httponly_index_(0),
-      priority_index_(0) {
+      priority_index_(0),
+      parsed_invalid_control_char_(false) {
 
   if (cookie_line.size() > kMaxCookieSize) {
     VLOG(1) << "Not parsing cookie, too large: " << cookie_line.size();
@@ -165,6 +172,8 @@ ParsedCookie::ParsedCookie(const std::string& cookie_line)
   }
 
   ParseTokenValuePairs(cookie_line);
+  UMA_HISTOGRAM_BOOLEAN("Cookies.ParsedInvalidControlCharacter",
+      parsed_invalid_control_char_);
   if (!pairs_.empty())
     SetupAttributes();
 }
@@ -381,8 +390,14 @@ void ParsedCookie::ParseTokenValuePairs(const std::string& cookie_line) {
     // OK, now try to parse a value.
     std::string::const_iterator value_start, value_end;
     ParseValue(&it, end, &value_start, &value_end);
+
+    std::string value(value_start, value_end);
+
+    if (!IsValidCookieAttributeValue(value))

[erikwright (departed), 2013/05/24 14:15:56]

Why not just put this after the assignment to pair.second and do
IsValidCookieAttributeValue(pair.second)?

Saves a copy, but also makes it easier to just rip this code out later once you
have your data.

[erikwright (departed), 2013/05/24 14:15:56]

Note that the call to IsValidCookieAttributeValue also sends its own histogram.

[jww, 2013/05/24 17:56:33]

Done.

[jww, 2013/05/24 17:56:33]

See my other responses. We want to measure a couple of different things.
On 2013/05/24 14:15:56, erikwright wrote:

+      parsed_invalid_control_char_ = true;

[erikwright (departed), 2013/05/24 14:15:56]

I suggest converting this to a local boolean and doing the UMA collection at the
end of this method rather than adding member state.

[palmer, 2013/05/24 17:38:43]

I'll second that.

[jww, 2013/05/24 17:56:33]

Done.

[jww, 2013/05/24 17:56:33]

Done.

+
     // OK, we're finished with a Token/Value.
-    pair.second = std::string(value_start, value_end);
+    pair.second = value;
 
     // From RFC2109: "Attributes (names) (attr) are case-insensitive."
     if (pair_num != 0)