Converted CanonicalCookies over to containing a domain.

Added perftest for getters in nested subdomains.

BUG=8850
TEST=[Linux] net_unittests --gtest_filter=CookieMonsterTest.*:ParsedCookieTest.*, net_perftests --gtest_filter=CookieMonsterTest.*

Review URL: http://codereview.chromium.org/2847046

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

4 files changed, 175 insertions, 32 deletions

diff --git a/net/base/cookie_monster.cc b/net/base/cookie_monster.cc
index 414bf15..bd14890 100644
--- a/net/base/cookie_monster.cc
+++ b/net/base/cookie_monster.cc
@@ -174,15 +174,46 @@ void CookieMonster::EnsureCookiesMapIsValid() {
 //     {list of cookies with this signature, sorted by creation time}.
 // (2) For each list with more than 1 entry, keep the cookie having the
 //     most recent creation time, and delete the others.
+namespace {
+// Two cookies are considered equivalent if they have the same domain,
+// name, and path.
+struct CookieSignature {
+ public:
+  CookieSignature(const std::string& name, const std::string& domain,
+                  const std::string& path)
+      : name(name),
+        domain(domain),
+        path(path) {}
+
+  // To be a key for a map this class needs to be assignable, copyable,
+  // and have an operator<.  The default assignment operator
+  // and copy constructor are exactly what we want.
+
+  bool operator<(const CookieSignature& cs) const {
+    // Name compare dominates, then domain, then path.
+    int diff = name.compare(cs.name);
+    if (diff != 0)
+      return diff < 0;
+
+    diff = domain.compare(cs.domain);
+    if (diff != 0)
+      return diff < 0;
+
+    return path.compare(cs.path) < 0;
+  }
+
+  std::string name;
+  std::string domain;
+  std::string path;
+};
+}
+
 int CookieMonster::TrimDuplicateCookiesForHost(
     const std::string& key,
     CookieMap::iterator begin,
     CookieMap::iterator end) {
   lock_.AssertAcquired();
 
-  // Two cookies are considered equivalent if they have the same name and path.
-  typedef std::pair<std::string, std::string> CookieSignature;
-
   // List of cookies ordered by creation time.
   typedef std::set<CookieMap::iterator, OrderByCreationTimeDesc> CookieList;
 
@@ -199,7 +230,8 @@ int CookieMonster::TrimDuplicateCookiesForHost(
     DCHECK_EQ(key, it->first);
     CanonicalCookie* cookie = it->second;
 
-    CookieSignature signature(cookie->Name(), cookie->Path());
+    CookieSignature signature(cookie->Name(), cookie->Domain(),
+                              cookie->Path());
     CookieList& list = equivalent_cookies[signature];
 
     // We found a duplicate!
@@ -231,11 +263,12 @@ int CookieMonster::TrimDuplicateCookiesForHost(
     dupes.erase(dupes.begin());
 
     LOG(ERROR) << StringPrintf("Found %d duplicate cookies for host='%s', "
-                               "with {name='%s', path='%s'}",
+                               "with {name='%s', domain='%s', path='%s'}",
                                static_cast<int>(dupes.size()),
                                key.c_str(),
-                               signature.first.c_str(),
-                               signature.second.c_str());
+                               signature.name.c_str(),
+                               signature.domain.c_str(),
+                               signature.path.c_str());
 
     // Remove all the cookies identified by |dupes|. It is valid to delete our
     // list of iterators one at a time, since |cookies_| is a multimap (they
@@ -606,7 +639,8 @@ bool CookieMonster::SetCookieWithCreationTimeAndOptions(
   scoped_ptr<CanonicalCookie> cc;
   Time cookie_expires = CanonExpiration(pc, creation_time, options);
 
-  cc.reset(new CanonicalCookie(pc.Name(), pc.Value(), cookie_path,
+  cc.reset(new CanonicalCookie(pc.Name(), pc.Value(), cookie_domain,
+                               cookie_path,
                                pc.IsSecure(), pc.IsHttpOnly(),
                                creation_time, creation_time,
                                !cookie_expires.is_null(), cookie_expires));
@@ -623,14 +657,6 @@ bool CookieMonster::SetCookieWithDetails(
     const std::string& domain, const std::string& path,
     const base::Time& expiration_time, bool secure, bool http_only) {
 
-  // Expect a valid domain attribute with no illegal characters.
-  std::string parsed_domain = ParsedCookie::ParseValueString(domain);
-  if (parsed_domain != domain)
-    return false;
-  std::string cookie_domain;
-  if (!GetCookieDomainKeyWithString(url, parsed_domain, &cookie_domain))
-    return false;
-
   AutoLock autolock(lock_);
 
   if (!HasCookieableScheme(url))
@@ -643,7 +669,8 @@ bool CookieMonster::SetCookieWithDetails(
 
   scoped_ptr<CanonicalCookie> cc;
   cc.reset(CanonicalCookie::Create(
-      url, name, value, path, creation_time, expiration_time,
+      url, name, value, domain, path,
+      creation_time, expiration_time,
       secure, http_only));
 
   if (!cc.get())
@@ -651,7 +678,7 @@ bool CookieMonster::SetCookieWithDetails(
 
   CookieOptions options;
   options.set_include_httponly();
-  return SetCanonicalCookie(&cc, cookie_domain, creation_time, options);
+  return SetCanonicalCookie(&cc, cc->Domain(), creation_time, options);
 }
 
 bool CookieMonster::SetCanonicalCookie(scoped_ptr<CanonicalCookie>* cc,
@@ -1500,12 +1527,26 @@ CookieMonster::CanonicalCookie::CanonicalCookie(const GURL& url,
       httponly_(pc.IsHttpOnly()) {
   if (has_expires_)
     expiry_date_ = CanonExpiration(pc, creation_date_, CookieOptions());
+
+  // Do the best we can with the domain.
+  std::string cookie_domain;
+  std::string domain_string;
+  if (pc.HasDomain()) {
+    domain_string = pc.Domain();
+  }
+  bool result
+      = GetCookieDomainKeyWithString(url, domain_string,
+                                     &cookie_domain);
+  // Caller is responsible for passing in good arguments.
+  DCHECK(result);
+  domain_ = cookie_domain;
 }
 
 CookieMonster::CanonicalCookie* CookieMonster::CanonicalCookie::Create(
       const GURL& url, const std::string& name, const std::string& value,
-      const std::string& path, const base::Time& creation_time,
-      const base::Time& expiration_time, bool secure, bool http_only) {
+      const std::string& domain, const std::string& path,
+      const base::Time& creation_time, const base::Time& expiration_time,
+      bool secure, bool http_only) {
   // Expect valid attribute tokens and values, as defined by the ParsedCookie
   // logic, otherwise don't create the cookie.
   std::string parsed_name = ParsedCookie::ParseTokenString(name);
@@ -1514,6 +1555,14 @@ CookieMonster::CanonicalCookie* CookieMonster::CanonicalCookie::Create(
   std::string parsed_value = ParsedCookie::ParseValueString(value);
   if (parsed_value != value)
     return NULL;
+
+  std::string parsed_domain = ParsedCookie::ParseValueString(domain);
+  if (parsed_domain != domain)
+    return NULL;
+  std::string cookie_domain;
+  if (!GetCookieDomainKeyWithString(url, parsed_domain, &cookie_domain))
+    return false;
+
   std::string parsed_path = ParsedCookie::ParseValueString(path);
   if (parsed_path != path)
     return NULL;
@@ -1531,8 +1580,9 @@ CookieMonster::CanonicalCookie* CookieMonster::CanonicalCookie::Create(
   cookie_path = std::string(canon_path.data() + canon_path_component.begin,
                             canon_path_component.len);
 
-  return new CanonicalCookie(parsed_name, parsed_value, cookie_path,
-                             secure, http_only, creation_time, creation_time,
+  return new CanonicalCookie(parsed_name, parsed_value, cookie_domain,
+                             cookie_path, secure, http_only,
+                             creation_time, creation_time,
                              !expiration_time.is_null(), expiration_time);
 }
 
@@ -1573,8 +1623,10 @@ bool CookieMonster::CanonicalCookie::IsOnPath(
 }
 
 std::string CookieMonster::CanonicalCookie::DebugString() const {
-  return StringPrintf("name: %s value: %s path: %s creation: %" PRId64,
-                      name_.c_str(), value_.c_str(), path_.c_str(),
+  return StringPrintf("name: %s value: %s domain: %s path: %s creation: %"
+                      PRId64,
+                      name_.c_str(), value_.c_str(),
+                      domain_.c_str(), path_.c_str(),
                       static_cast<int64>(creation_date_.ToTimeT()));
 }
 
diff --git a/net/base/cookie_monster.h b/net/base/cookie_monster.h
index 33d0a69..4986cd4 100644
--- a/net/base/cookie_monster.h
+++ b/net/base/cookie_monster.h
@@ -318,9 +318,15 @@ class CookieMonster : public CookieStore {
 
 class CookieMonster::CanonicalCookie {
  public:
+
+  // These constructors do no validation or canonicalization of their inputs;
+  // the resulting CanonicalCookies should not be relied on to be canonical
+  // unless the caller has done appropriate validation and canonicalization
+  // themselves.
   CanonicalCookie() { }
   CanonicalCookie(const std::string& name,
                   const std::string& value,
+                  const std::string& domain,
                   const std::string& path,
                   bool secure,
                   bool httponly,
@@ -330,6 +336,7 @@ class CookieMonster::CanonicalCookie {
                   const base::Time& expires)
       : name_(name),
         value_(value),
+        domain_(domain),
         path_(path),
         creation_date_(creation),
         last_access_date_(last_access),
@@ -338,19 +345,26 @@ class CookieMonster::CanonicalCookie {
         secure_(secure),
         httponly_(httponly) {
   }
+
+  // This constructor does canonicalization but not validation.
+  // The result of this constructor should not be relied on in contexts
+  // in which pre-validation of the ParsedCookie has not been done.
   CanonicalCookie(const GURL& url, const ParsedCookie& pc);
 
   // Supports the default copy constructor.
 
-  // Creates a canonical cookie from unparsed attribute values. May return
-  // NULL if an attribute value is invalid.
+  // Creates a canonical cookie from unparsed attribute values.
+  // Canonicalizes and validates inputs.  May return NULL if an attribute
+  // value is invalid.
   static CanonicalCookie* Create(
       const GURL& url, const std::string& name, const std::string& value,
-      const std::string& path, const base::Time& creation_time,
-      const base::Time& expiration_time, bool secure, bool http_only);
+      const std::string& domain, const std::string& path,
+      const base::Time& creation_time, const base::Time& expiration_time,
+      bool secure, bool http_only);
 
   const std::string& Name() const { return name_; }
   const std::string& Value() const { return value_; }
+  const std::string& Domain() const { return domain_; }
   const std::string& Path() const { return path_; }
   const base::Time& CreationDate() const { return creation_date_; }
   const base::Time& LastAccessDate() const { return last_access_date_; }
@@ -364,13 +378,18 @@ class CookieMonster::CanonicalCookie {
     return has_expires_ && current >= expiry_date_;
   }
 
-  // Are the cookies considered equivalent in the eyes of the RFC.
-  // This says that the domain and path should string match identically.
+  // Are the cookies considered equivalent in the eyes of RFC 2965.
+  // The RFC says that name must match (case-sensitive), domain must
+  // match (case insensitive), and path must match (case sensitive).
+  // For the case insensitive domain compare, we rely on the domain
+  // having been canonicalized (in
+  // GetCookieDomainKeyWithString->CanonicalizeHost).
   bool IsEquivalent(const CanonicalCookie& ecc) const {
     // It seems like it would make sense to take secure and httponly into
     // account, but the RFC doesn't specify this.
     // NOTE: Keep this logic in-sync with TrimDuplicateCookiesForHost().
-    return name_ == ecc.Name() && path_ == ecc.Path();
+    return (name_ == ecc.Name() && domain_ == ecc.Domain()
+            && path_ == ecc.Path());
   }
 
   void SetLastAccessDate(const base::Time& date) {
@@ -383,6 +402,7 @@ class CookieMonster::CanonicalCookie {
  private:
   std::string name_;
   std::string value_;
+  std::string domain_;
   std::string path_;
   base::Time creation_date_;
   base::Time last_access_date_;
diff --git a/net/base/cookie_monster_perftest.cc b/net/base/cookie_monster_perftest.cc
index 3445b43..3d1e7a7 100644
--- a/net/base/cookie_monster_perftest.cc
+++ b/net/base/cookie_monster_perftest.cc
@@ -93,3 +93,71 @@ TEST(CookieMonsterTest, TestAddCookieOnManyHosts) {
   cm->DeleteAll(false);
   timer3.Done();
 }
+
+TEST(CookieMonsterTest, TestDomainTree) {
+  scoped_refptr<net::CookieMonster> cm(new net::CookieMonster(NULL, NULL));
+  std::string cookie(kCookieLine);
+  std::string domain_base("top.com");
+
+  std::vector<GURL> gurl_list;
+
+  // Create a balanced binary tree of domains on which the cookie is set.
+  for (int i1=0; i1 < 2; i1++) {
+    std::string domain_base_1((i1 ? "a." : "b.") + domain_base);
+    gurl_list.push_back(GURL(StringPrintf("http://%s",
+                                          domain_base_1.c_str())));
+    for (int i2=0; i2 < 2; i2++) {
+      std::string domain_base_2((i1 ? "a." : "b.") + domain_base_1);
+      gurl_list.push_back(GURL(StringPrintf("http://%s",
+                                            domain_base_2.c_str())));
+      for (int i3=0; i3 < 2; i3++) {
+        std::string domain_base_3((i1 ? "a." : "b.") + domain_base_2);
+        gurl_list.push_back(GURL(StringPrintf("http://%s",
+                                              domain_base_3.c_str())));
+        for (int i4=0; i4 < 2; i4++) {
+          gurl_list.push_back(GURL(StringPrintf("http://%s.%s",
+                                                i4 ? "a" : "b",
+                                                domain_base_3.c_str())));
+        }
+      }
+    }
+  }
+
+  for (std::vector<GURL>::const_iterator it = gurl_list.begin();
+       it != gurl_list.end(); it++) {
+    EXPECT_TRUE(cm->SetCookie(*it, cookie));
+  }
+
+  GURL probe_gurl("http://b.a.b.a.top.com/");
+  PerfTimeLogger timer("Cookie_monster_query_domain_tree");
+  for (int i = 0; i < kNumCookies; i++) {
+    cm->GetCookies(probe_gurl);
+  }
+  timer.Done();
+
+  cm->DeleteAll(false);
+  gurl_list.clear();
+
+  // Create a line of 32 domain cookies such that all cookies stored
+  // by effective TLD+1 will apply to probe GURL.
+  // (TLD + 1 is the level above .com/org/net/etc, e.g. "top.com"
+  // or "google.com".  "Effective" is added to include sites like
+  // bbc.co.uk, where the effetive TLD+1 is more than one level
+  // below the top level.)
+  gurl_list.push_back(GURL("http://a.top.com"));
+  gurl_list.push_back(GURL("http://b.a.top.com"));
+  gurl_list.push_back(GURL("http://a.b.a.top.com"));
+  gurl_list.push_back(GURL("http://b.a.b.a.top.com"));
+
+  for (int i = 0; i < 8; i++) {
+    for (std::vector<GURL>::const_iterator it = gurl_list.begin();
+         it != gurl_list.end(); it++) {
+      EXPECT_TRUE(cm->SetCookie(*it, StringPrintf("a%03d=b", i)));
+    }
+  }
+  PerfTimeLogger timer2("Cookie_monster_query_domain_line");
+  for (int i = 0; i < kNumCookies; i++) {
+    cm->GetCookies(probe_gurl);
+  }
+  timer2.Done();
+}
diff --git a/net/base/cookie_monster_unittest.cc b/net/base/cookie_monster_unittest.cc
index 21d6df8..e9ffc7b 100644
--- a/net/base/cookie_monster_unittest.cc
+++ b/net/base/cookie_monster_unittest.cc
@@ -152,7 +152,7 @@ void AddKeyedCookieToList(
 
   scoped_ptr<net::CookieMonster::CanonicalCookie> cookie(
       new net::CookieMonster::CanonicalCookie(
-          pc.Name(), pc.Value(), cookie_path,
+          pc.Name(), pc.Value(), key, cookie_path,
           pc.IsSecure(), pc.IsHttpOnly(),
           creation_time, creation_time,
           !cookie_expires.is_null(),
@@ -1684,6 +1684,7 @@ TEST(CookieMonsterTest, SetCookieWithDetails) {
   EXPECT_EQ("A", it->second.Name());
   EXPECT_EQ("B", it->second.Value());
   EXPECT_EQ("www.google.izzle", it->first);
+  EXPECT_EQ("www.google.izzle", it->second.Domain());
   EXPECT_EQ("/foo", it->second.Path());
   EXPECT_FALSE(it->second.DoesExpire());
   EXPECT_FALSE(it->second.IsSecure());
@@ -1698,6 +1699,7 @@ TEST(CookieMonsterTest, SetCookieWithDetails) {
   EXPECT_EQ("C", it->second.Name());
   EXPECT_EQ("D", it->second.Value());
   EXPECT_EQ(".google.izzle", it->first);
+  EXPECT_EQ(".google.izzle", it->second.Domain());
   EXPECT_EQ("/bar", it->second.Path());
   EXPECT_FALSE(it->second.IsSecure());
   EXPECT_TRUE(it->second.IsHttpOnly());
@@ -1711,6 +1713,7 @@ TEST(CookieMonsterTest, SetCookieWithDetails) {
   EXPECT_EQ("E", it->second.Name());
   EXPECT_EQ("F", it->second.Value());
   EXPECT_EQ("/", it->second.Path());
+  EXPECT_EQ("www.google.izzle", it->second.Domain());
   EXPECT_TRUE(it->second.IsSecure());
   EXPECT_FALSE(it->second.IsHttpOnly());