Refactor net::HttpUtil::NameValuePairsIterator to relieve clients of the need to think about quoted values while also avoiding a string copy if the value is unquoted.

The iterator now holds a (normally empty) string member that it uses only if the currently accessed value is quoted. In this case, the value_begin and value_end iterators point into this string (holding the unquoted value) as opposed to the original buffer (holding the quoted value). The value is only unquoted if it is accessed.

As a result, the interface is simplified to not expose whether the current value is quoted. This simplifies the work of all clients. Furthermore, this implementation is optimized to only construct a string if it is required, whereas most clients previously (for simplicity) constructed a new string whether or not it was required. They will therefore benefit from a slight increase in efficiency.

BUG=52601
TEST=net_unittests / HttpUtilTest.NameValuePairs*, HttpAuthTest.*
Review URL: http://codereview.chromium.org/3777012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@63514 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 135 insertions, 81 deletions

diff --git a/net/http/http_auth_handler_basic.cc b/net/http/http_auth_handler_basic.cc
index 054357a..35b088f 100644
--- a/net/http/http_auth_handler_basic.cc
+++ b/net/http/http_auth_handler_basic.cc
@@ -41,7 +41,7 @@ bool HttpAuthHandlerBasic::ParseChallenge(
   std::string realm;
   while (parameters.GetNext()) {
     if (LowerCaseEqualsASCII(parameters.name(), "realm"))
-      realm = parameters.unquoted_value();
+      realm = parameters.value();
   }
 
   if (!parameters.valid())
diff --git a/net/http/http_auth_handler_digest.cc b/net/http/http_auth_handler_digest.cc
index f7f178c..82aa9af 100644
--- a/net/http/http_auth_handler_digest.cc
+++ b/net/http/http_auth_handler_digest.cc
@@ -221,7 +221,7 @@ HttpAuth::AuthorizationResult HttpAuthHandlerDigest::HandleAnotherChallenge(
   while (parameters.GetNext()) {
     if (!LowerCaseEqualsASCII(parameters.name(), "stale"))
       continue;
-    if (LowerCaseEqualsASCII(parameters.unquoted_value(), "true"))
+    if (LowerCaseEqualsASCII(parameters.value(), "true"))
       return HttpAuth::AUTHORIZATION_RESULT_STALE;
   }
 
@@ -266,14 +266,9 @@ bool HttpAuthHandlerDigest::ParseChallenge(
 
   // Loop through all the properties.
   while (parameters.GetNext()) {
-    if (parameters.value().empty()) {
-      DVLOG(1) << "Invalid digest property";
-      return false;
-    }
-
     // FAIL -- couldn't parse a property.
     if (!ParseChallengeProperty(parameters.name(),
-                                parameters.unquoted_value()))
+                                parameters.value()))
       return false;
   }
 
diff --git a/net/http/http_auth_unittest.cc b/net/http/http_auth_unittest.cc
index 3068135..60f2b0e 100644
--- a/net/http/http_auth_unittest.cc
+++ b/net/http/http_auth_unittest.cc
@@ -249,9 +249,7 @@ TEST(HttpAuthTest, ChallengeTokenizer) {
   EXPECT_TRUE(parameters.GetNext());
   EXPECT_TRUE(parameters.valid());
   EXPECT_EQ(std::string("realm"), parameters.name());
-  EXPECT_EQ(std::string("foobar"), parameters.unquoted_value());
-  EXPECT_EQ(std::string("\"foobar\""), parameters.value());
-  EXPECT_TRUE(parameters.value_is_quoted());
+  EXPECT_EQ(std::string("foobar"), parameters.value());
   EXPECT_FALSE(parameters.GetNext());
 }
 
@@ -268,8 +266,6 @@ TEST(HttpAuthTest, ChallengeTokenizerNoQuotes) {
   EXPECT_TRUE(parameters.valid());
   EXPECT_EQ(std::string("realm"), parameters.name());
   EXPECT_EQ(std::string("foobar@baz.com"), parameters.value());
-  EXPECT_EQ(std::string("foobar@baz.com"), parameters.unquoted_value());
-  EXPECT_FALSE(parameters.value_is_quoted());
   EXPECT_FALSE(parameters.GetNext());
 }
 
@@ -286,8 +282,6 @@ TEST(HttpAuthTest, ChallengeTokenizerMismatchedQuotes) {
   EXPECT_TRUE(parameters.valid());
   EXPECT_EQ(std::string("realm"), parameters.name());
   EXPECT_EQ(std::string("foobar@baz.com"), parameters.value());
-  EXPECT_EQ(std::string("foobar@baz.com"), parameters.unquoted_value());
-  EXPECT_FALSE(parameters.value_is_quoted());
   EXPECT_FALSE(parameters.GetNext());
 }
 
@@ -304,7 +298,6 @@ TEST(HttpAuthTest, ChallengeTokenizerMismatchedQuotesNoValue) {
   EXPECT_TRUE(parameters.valid());
   EXPECT_EQ(std::string("realm"), parameters.name());
   EXPECT_EQ(std::string(""), parameters.value());
-  EXPECT_FALSE(parameters.value_is_quoted());
   EXPECT_FALSE(parameters.GetNext());
 }
 
@@ -322,15 +315,13 @@ TEST(HttpAuthTest, ChallengeTokenizerMismatchedQuotesSpaces) {
   EXPECT_TRUE(parameters.valid());
   EXPECT_EQ(std::string("realm"), parameters.name());
   EXPECT_EQ(std::string("foo bar"), parameters.value());
-  EXPECT_EQ(std::string("foo bar"), parameters.unquoted_value());
-  EXPECT_FALSE(parameters.value_is_quoted());
   EXPECT_FALSE(parameters.GetNext());
 }
 
 // Use multiple name=value properties with mismatching quote marks in the last
 // value.
 TEST(HttpAuthTest, ChallengeTokenizerMismatchedQuotesMultiple) {
-  std::string challenge_str = "Digest qop=, algorithm=md5, realm=\"foo";
+  std::string challenge_str = "Digest qop=auth-int, algorithm=md5, realm=\"foo";
   HttpAuth::ChallengeTokenizer challenge(challenge_str.begin(),
                                          challenge_str.end());
   HttpUtil::NameValuePairsIterator parameters = challenge.param_pairs();
@@ -340,20 +331,15 @@ TEST(HttpAuthTest, ChallengeTokenizerMismatchedQuotesMultiple) {
   EXPECT_TRUE(parameters.GetNext());
   EXPECT_TRUE(parameters.valid());
   EXPECT_EQ(std::string("qop"), parameters.name());
-  EXPECT_EQ(std::string(""), parameters.value());
-  EXPECT_FALSE(parameters.value_is_quoted());
+  EXPECT_EQ(std::string("auth-int"), parameters.value());
   EXPECT_TRUE(parameters.GetNext());
   EXPECT_TRUE(parameters.valid());
   EXPECT_EQ(std::string("algorithm"), parameters.name());
   EXPECT_EQ(std::string("md5"), parameters.value());
-  EXPECT_EQ(std::string("md5"), parameters.unquoted_value());
-  EXPECT_FALSE(parameters.value_is_quoted());
   EXPECT_TRUE(parameters.GetNext());
   EXPECT_TRUE(parameters.valid());
   EXPECT_EQ(std::string("realm"), parameters.name());
   EXPECT_EQ(std::string("foo"), parameters.value());
-  EXPECT_EQ(std::string("foo"), parameters.unquoted_value());
-  EXPECT_FALSE(parameters.value_is_quoted());
   EXPECT_FALSE(parameters.GetNext());
 }
 
@@ -366,12 +352,8 @@ TEST(HttpAuthTest, ChallengeTokenizerNoValue) {
 
   EXPECT_TRUE(parameters.valid());
   EXPECT_EQ(std::string("Digest"), challenge.scheme());
-  EXPECT_TRUE(parameters.GetNext());
-  EXPECT_TRUE(parameters.valid());
-  EXPECT_EQ(std::string("qop"), parameters.name());
-  EXPECT_EQ(std::string(""), parameters.value());
-  EXPECT_FALSE(parameters.value_is_quoted());
   EXPECT_FALSE(parameters.GetNext());
+  EXPECT_FALSE(parameters.valid());
 }
 
 // Specify multiple properties, comma separated.
@@ -388,18 +370,16 @@ TEST(HttpAuthTest, ChallengeTokenizerMultiple) {
   EXPECT_TRUE(parameters.valid());
   EXPECT_EQ(std::string("algorithm"), parameters.name());
   EXPECT_EQ(std::string("md5"), parameters.value());
-  EXPECT_FALSE(parameters.value_is_quoted());
   EXPECT_TRUE(parameters.GetNext());
   EXPECT_TRUE(parameters.valid());
   EXPECT_EQ(std::string("realm"), parameters.name());
-  EXPECT_EQ(std::string("Oblivion"), parameters.unquoted_value());
-  EXPECT_TRUE(parameters.value_is_quoted());
+  EXPECT_EQ(std::string("Oblivion"), parameters.value());
   EXPECT_TRUE(parameters.GetNext());
   EXPECT_TRUE(parameters.valid());
   EXPECT_EQ(std::string("qop"), parameters.name());
   EXPECT_EQ(std::string("auth-int"), parameters.value());
-  EXPECT_FALSE(parameters.value_is_quoted());
   EXPECT_FALSE(parameters.GetNext());
+  EXPECT_TRUE(parameters.valid());
 }
 
 // Use a challenge which has no property.
diff --git a/net/http/http_util.cc b/net/http/http_util.cc
index 5c7a4fc..641c43c 100644
--- a/net/http/http_util.cc
+++ b/net/http/http_util.cc
@@ -828,12 +828,12 @@ bool HttpUtil::NameValuePairsIterator::GetNext() {
   // Scan for the equals sign.
   std::string::const_iterator equals = std::find(value_begin_, value_end_, '=');
   if (equals == value_end_ || equals == value_begin_)
-    return valid_ = false;  // Malformed
+    return valid_ = false;  // Malformed, no equals sign
 
   // Verify that the equals sign we found wasn't inside of quote marks.
   for (std::string::const_iterator it = value_begin_; it != equals; ++it) {
     if (HttpUtil::IsQuote(*it))
-      return valid_ = false;  // Malformed
+      return valid_ = false;  // Malformed, quote appears before equals sign
   }
 
   name_begin_ = value_begin_;
@@ -843,25 +843,28 @@ bool HttpUtil::NameValuePairsIterator::GetNext() {
   TrimLWS(&name_begin_, &name_end_);
   TrimLWS(&value_begin_, &value_end_);
   value_is_quoted_ = false;
-  if (value_begin_ != value_end_ && HttpUtil::IsQuote(*value_begin_)) {
+  unquoted_value_.clear();
+
+  if (value_begin_ == value_end_)
+    return valid_ = false;  // Malformed, value is empty
+
+  if (HttpUtil::IsQuote(*value_begin_)) {
     // Trim surrounding quotemarks off the value
-    if (*value_begin_ != *(value_end_ - 1) || value_begin_ + 1 == value_end_)
+    if (*value_begin_ != *(value_end_ - 1) || value_begin_ + 1 == value_end_) {
       // NOTE: This is not as graceful as it sounds:
       // * quoted-pairs will no longer be unquoted
       //   (["\"hello] should give ["hello]).
       // * Does not detect when the final quote is escaped
       //   (["value\"] should give [value"])
       ++value_begin_;  // Gracefully recover from mismatching quotes.
-    else
+    } else {
       value_is_quoted_ = true;
+      // Do not store iterators into this. See declaration of unquoted_value_.
+      unquoted_value_ = HttpUtil::Unquote(value_begin_, value_end_);
+    }
   }
 
   return true;
 }
 
-// If value() has quotemarks, unquote it.
-std::string HttpUtil::NameValuePairsIterator::unquoted_value() const {
-  return HttpUtil::Unquote(value_begin_, value_end_);
-}
-
 }  // namespace net
diff --git a/net/http/http_util.h b/net/http/http_util.h
index f41c4e4..2f5bd85 100644
--- a/net/http/http_util.h
+++ b/net/http/http_util.h
@@ -275,6 +275,9 @@ class HttpUtil {
   // Each pair consists of a token (the name), an equals sign, and either a
   // token or quoted-string (the value). Arbitrary HTTP LWS is permitted outside
   // of and between names, values, and delimiters.
+  //
+  // String iterators returned from this class' methods may be invalidated upon
+  // calls to GetNext() or after the NameValuePairsIterator is destroyed.
   class NameValuePairsIterator {
    public:
     NameValuePairsIterator(std::string::const_iterator begin,
@@ -295,15 +298,16 @@ class HttpUtil {
     std::string name() const { return std::string(name_begin_, name_end_); }
 
     // The value of the current name-value pair.
-    std::string::const_iterator value_begin() const { return value_begin_; }
-    std::string::const_iterator value_end() const { return value_end_; }
-    std::string value() const { return std::string(value_begin_, value_end_); }
-
-    // If value() has quotemarks, unquote it.
-    std::string unquoted_value() const;
-
-    // True if the name-value pair's value has quote marks.
-    bool value_is_quoted() const { return value_is_quoted_; }
+    std::string::const_iterator value_begin() const {
+      return value_is_quoted_ ? unquoted_value_.begin() : value_begin_;
+    }
+    std::string::const_iterator value_end() const {
+      return value_is_quoted_ ? unquoted_value_.end() : value_end_;
+    }
+    std::string value() const {
+      return value_is_quoted_ ? unquoted_value_ : std::string(value_begin_,
+                                                               value_end_);
+    }
 
    private:
     HttpUtil::ValuesIterator props_;
@@ -318,6 +322,11 @@ class HttpUtil {
     std::string::const_iterator value_begin_;
     std::string::const_iterator value_end_;
 
+    // Do not store iterators into this string. The NameValuePairsIterator
+    // is copyable/assignable, and if copied the copy's iterators would point
+    // into the original's unquoted_value_ member.
+    std::string unquoted_value_;
+
     bool value_is_quoted_;
   };
 };
diff --git a/net/http/http_util_unittest.cc b/net/http/http_util_unittest.cc
index 4ebbd2e..47242d2 100644
--- a/net/http/http_util_unittest.cc
+++ b/net/http/http_util_unittest.cc
@@ -673,27 +673,48 @@ TEST(HttpUtilTest, ParseRanges) {
 }
 
 namespace {
-
-void CheckNameValuePair(HttpUtil::NameValuePairsIterator* parser,
-                        bool expect_next,
-                        bool expect_valid,
-                        std::string expected_name,
-                        std::string expected_value,
-                        std::string expected_unquoted_value) {
-  ASSERT_EQ(expect_next, parser->GetNext());
+void CheckCurrentNameValuePair(HttpUtil::NameValuePairsIterator* parser,
+                               bool expect_valid,
+                               std::string expected_name,
+                               std::string expected_value) {
   ASSERT_EQ(expect_valid, parser->valid());
-  if (!expect_next || !expect_valid) {
+  if (!expect_valid) {
     return;
   }
+
+  // Let's make sure that these never change (i.e., when a quoted value is
+  // unquoted, it should be cached on the first calls and not regenerated
+  // later).
+  std::string::const_iterator first_value_begin = parser->value_begin();
+  std::string::const_iterator first_value_end = parser->value_end();
+
   ASSERT_EQ(expected_name, std::string(parser->name_begin(),
                                        parser->name_end()));
   ASSERT_EQ(expected_name, parser->name());
   ASSERT_EQ(expected_value, std::string(parser->value_begin(),
                                         parser->value_end()));
   ASSERT_EQ(expected_value, parser->value());
-  ASSERT_EQ(expected_unquoted_value, parser->unquoted_value());
-  ASSERT_EQ(expected_value != expected_unquoted_value,
-            parser->value_is_quoted());
+
+  // Make sure they didn't/don't change.
+  ASSERT_TRUE(first_value_begin == parser->value_begin());
+  ASSERT_TRUE(first_value_end == parser->value_end());
+}
+
+void CheckNextNameValuePair(HttpUtil::NameValuePairsIterator* parser,
+                            bool expect_next,
+                            bool expect_valid,
+                            std::string expected_name,
+                            std::string expected_value) {
+  ASSERT_EQ(expect_next, parser->GetNext());
+  ASSERT_EQ(expect_valid, parser->valid());
+  if (!expect_next || !expect_valid) {
+    return;
+  }
+
+  CheckCurrentNameValuePair(parser,
+                            expect_valid,
+                            expected_name,
+                            expected_value);
 }
 
 void CheckInvalidNameValuePair(std::string valid_part,
@@ -731,35 +752,78 @@ void CheckInvalidNameValuePair(std::string valid_part,
 
 }  // anonymous namespace
 
+TEST(HttpUtilTest, NameValuePairsIteratorCopyAndAssign) {
+  std::string data = "alpha='\\'a\\''; beta=\" b \"; cappa='c;'; delta=\"d\"";
+  HttpUtil::NameValuePairsIterator parser_a(data.begin(), data.end(), ';');
+
+  EXPECT_TRUE(parser_a.valid());
+  ASSERT_NO_FATAL_FAILURE(
+      CheckNextNameValuePair(&parser_a, true, true, "alpha", "'a'"));
+
+  HttpUtil::NameValuePairsIterator parser_b(parser_a);
+  // a and b now point to same location
+  ASSERT_NO_FATAL_FAILURE(
+      CheckCurrentNameValuePair(&parser_b, true, "alpha", "'a'"));
+  ASSERT_NO_FATAL_FAILURE(
+      CheckCurrentNameValuePair(&parser_a, true, "alpha", "'a'"));
+
+  // advance a, no effect on b
+  ASSERT_NO_FATAL_FAILURE(
+      CheckNextNameValuePair(&parser_a, true, true, "beta", " b "));
+  ASSERT_NO_FATAL_FAILURE(
+      CheckCurrentNameValuePair(&parser_b, true, "alpha", "'a'"));
+
+  // assign b the current state of a, no effect on a
+  parser_b = parser_a;
+  ASSERT_NO_FATAL_FAILURE(
+      CheckCurrentNameValuePair(&parser_b, true, "beta", " b "));
+  ASSERT_NO_FATAL_FAILURE(
+      CheckCurrentNameValuePair(&parser_a, true, "beta", " b "));
+
+  // advance b, no effect on a
+  ASSERT_NO_FATAL_FAILURE(
+      CheckNextNameValuePair(&parser_b, true, true, "cappa", "c;"));
+  ASSERT_NO_FATAL_FAILURE(
+      CheckCurrentNameValuePair(&parser_a, true, "beta", " b "));
+}
+
 TEST(HttpUtilTest, NameValuePairsIteratorEmptyInput) {
   std::string data = "";
   HttpUtil::NameValuePairsIterator parser(data.begin(), data.end(), ';');
 
   EXPECT_TRUE(parser.valid());
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, false, true, "", "", ""));
+      CheckNextNameValuePair(&parser, false, true, "", ""));
 }
 
 TEST(HttpUtilTest, NameValuePairsIterator) {
-  std::string data = "alpha=1; beta= 2 ;cappa =' 3 ';"
-                     "delta= \" 4 \"; e= \" '5'\"; e=6";
+  std::string data = "alpha=1; beta= 2 ;cappa =' 3; ';"
+                     "delta= \" \\\"4\\\" \"; e= \" '5'\"; e=6;"
+                     "f='\\'\\h\\e\\l\\l\\o\\ \\w\\o\\r\\l\\d\\'';"
+                     "g=''; h='hello'";
   HttpUtil::NameValuePairsIterator parser(data.begin(), data.end(), ';');
   EXPECT_TRUE(parser.valid());
 
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, true, true, "alpha", "1", "1"));
+      CheckNextNameValuePair(&parser, true, true, "alpha", "1"));
+  ASSERT_NO_FATAL_FAILURE(
+      CheckNextNameValuePair(&parser, true, true, "beta", "2"));
+  ASSERT_NO_FATAL_FAILURE(
+      CheckNextNameValuePair(&parser, true, true, "cappa", " 3; "));
+  ASSERT_NO_FATAL_FAILURE(
+      CheckNextNameValuePair(&parser, true, true, "delta", " \"4\" "));
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, true, true, "beta", "2", "2"));
+      CheckNextNameValuePair(&parser, true, true, "e", " '5'"));
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, true, true, "cappa", "' 3 '", " 3 "));
+      CheckNextNameValuePair(&parser, true, true, "e", "6"));
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, true, true, "delta", "\" 4 \"", " 4 "));
+      CheckNextNameValuePair(&parser, true, true, "f", "'hello world'"));
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, true, true, "e", "\" '5'\"", " '5'"));
+      CheckNextNameValuePair(&parser, true, true, "g", ""));
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, true, true, "e", "6", "6"));
+      CheckNextNameValuePair(&parser, true, true, "h", "hello"));
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, false, true, "", "", ""));
+      CheckNextNameValuePair(&parser, false, true, "", ""));
 }
 
 TEST(HttpUtilTest, NameValuePairsIteratorIllegalInputs) {
@@ -768,6 +832,9 @@ TEST(HttpUtilTest, NameValuePairsIteratorIllegalInputs) {
 
   ASSERT_NO_FATAL_FAILURE(CheckInvalidNameValuePair("alpha=1", "; 'beta'=2"));
   ASSERT_NO_FATAL_FAILURE(CheckInvalidNameValuePair("", "'beta'=2"));
+  ASSERT_NO_FATAL_FAILURE(CheckInvalidNameValuePair("alpha=1", ";beta="));
+  ASSERT_NO_FATAL_FAILURE(CheckInvalidNameValuePair("alpha=1",
+                                                    ";beta=;cappa=2"));
 
   // According to the spec this is an error, but it doesn't seem appropriate to
   // change our behaviour to be less permissive at this time.
@@ -783,13 +850,13 @@ TEST(HttpUtilTest, NameValuePairsIteratorExtraSeparators) {
   EXPECT_TRUE(parser.valid());
 
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, true, true, "alpha", "1", "1"));
+      CheckNextNameValuePair(&parser, true, true, "alpha", "1"));
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, true, true, "beta", "2", "2"));
+      CheckNextNameValuePair(&parser, true, true, "beta", "2"));
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, true, true, "cappa", "3", "3"));
+      CheckNextNameValuePair(&parser, true, true, "cappa", "3"));
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, false, true, "", "", ""));
+      CheckNextNameValuePair(&parser, false, true, "", ""));
 }
 
 // See comments on the implementation of NameValuePairsIterator::GetNext
@@ -800,7 +867,7 @@ TEST(HttpUtilTest, NameValuePairsIteratorMissingEndQuote) {
   EXPECT_TRUE(parser.valid());
 
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, true, true, "name", "value", "value"));
+      CheckNextNameValuePair(&parser, true, true, "name", "value"));
   ASSERT_NO_FATAL_FAILURE(
-    CheckNameValuePair(&parser, false, true, "", "", ""));
+      CheckNextNameValuePair(&parser, false, true, "", ""));
 }