Implementing EvictedDomainCookieCounter to keep user metrics on cookie eviction and reinstatement.

net/cookies/evicted_domain_cookie_counter.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/cookies/evicted_domain_cookie_counter.h"
+
+#include <limits>

[erikwright (departed), 2013/03/25 17:29:41]

What is limits used for?

[huangs, 2013/03/25 21:02:12]

Was to find max int for rand, but no longer needed.  Removed.

+#include <queue>
+#include <vector>
+
+#include "base/metrics/histogram.h"
+#include "base/stl_util.h"
+#include "base/string_util.h"
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
+#include "net/cookies/canonical_cookie.h"
+
+namespace net {
+
+using base::Time;
+using base::TimeDelta;
+
+namespace {
+
+const size_t kMaxEvictedDomainCookies = 500;
+const size_t kPurgeEvictedDomainCookies = 100;
+
+class DelegateImpl : public EvictedDomainCookieCounter::Delegate {
+ public:
+  DelegateImpl();
+
+  // EvictedDomainCookieCounter::Delegate implementation.
+  virtual void Report(const EvictedDomainCookieCounter::EvictedCookie& ec,
+                      const Time& reinstatement_time) OVERRIDE;
+  virtual Time CurrentTime() OVERRIDE;
+};
+
+DelegateImpl::DelegateImpl() {}
+
+void DelegateImpl::Report(const EvictedDomainCookieCounter::EvictedCookie& ec,
+                          const Time& reinstatement_time) {
+  TimeDelta reinstatement_delay(reinstatement_time - ec.eviction_time_);
+  // Keep separate blocks, since HISTOGRAM_CUSTOM_TIMES is a macro.

[erikwright (departed), 2013/03/25 17:29:41]

What does this comment mean?

[huangs, 2013/03/25 21:02:12]

It is tempting to "refactor" the code by removing duplicate code and use a
temporary string "Cookie.ReinstatedCookiesGoogle" and
"Cookie.ReinstatedCookiesOther". However, *I think* this "refactoring" task
would be problematic, since HISTOGRAM_CUSTOM_TIMES() macro block with static
object to store the histogram.  Hence the comment.

Rephrased.

+  if (ec.is_google_) {
+    HISTOGRAM_CUSTOM_TIMES("Cookie.ReinstatedCookiesGoogle",
+                           reinstatement_delay,
+                           TimeDelta::FromSeconds(1),
+                           TimeDelta::FromDays(7),
+                           50);
+  } else {
+    HISTOGRAM_CUSTOM_TIMES("Cookie.ReinstatedCookiesOther",
+                           reinstatement_delay,
+                           TimeDelta::FromSeconds(1),
+                           TimeDelta::FromDays(7),
+                           50);
+  }
+}
+
+Time DelegateImpl::CurrentTime() {
+  return Time::Now();
+}
+
+}  // namespace
+
+EvictedDomainCookieCounter::EvictedDomainCookieCounter(
+    scoped_refptr<CookieMonster::Delegate> next_cookie_monster_delegate)
+    : next_cookie_monster_delegate_(next_cookie_monster_delegate),
+      delegate_(new DelegateImpl),
+      max_size_(kMaxEvictedDomainCookies),
+      purge_count_(kPurgeEvictedDomainCookies) {
+}
+
+EvictedDomainCookieCounter::EvictedDomainCookieCounter(
+    scoped_refptr<CookieMonster::Delegate> next_cookie_monster_delegate,
+    scoped_ptr<Delegate> delegate,
+    size_t max_size,
+    size_t purge_count)
+    : next_cookie_monster_delegate_(next_cookie_monster_delegate),
+      delegate_(delegate.Pass()),
+      max_size_(max_size),
+      purge_count_(purge_count) {
+  DCHECK(delegate_);
+  DCHECK(purge_count < max_size_);
+}
+
+EvictedDomainCookieCounter::~EvictedDomainCookieCounter() {
+  STLDeleteContainerPairSecondPointers(evicted_cookies_.begin(),
+                                       evicted_cookies_.end());
+}
+
+// static
+// Simplified from google_util.cc: IsGoogleHostname().
+bool EvictedDomainCookieCounter::UnsafeIsGoogleCookie(

[erikwright (departed), 2013/03/25 17:29:41]

Why not just use IsGoogleHostname(cc.Domain(), ALLOW_SUBDOMAIN)?

If the only reason is because you can't access it from here, I would be happy to
see this code moved to chrome/browser/net/

[huangs, 2013/03/25 21:02:12]

It's due to target dependency; "net" should not depend on "browser"!

+    const CanonicalCookie& cc) {
+  const std::string& host(cc.Domain());
+  size_t tld_length =
+      RegistryControlledDomainService::GetRegistryLength(host, false);
+  if ((tld_length == 0) || (tld_length == std::string::npos))
+    return false;
+
+  std::string host_minus_tld(host, 0, host.length() - tld_length);
+  return LowerCaseEqualsASCII(host_minus_tld, "google.") ||
+      EndsWith(host_minus_tld, ".google.", false);
+}
+
+size_t EvictedDomainCookieCounter::GetStorageSize() const {
+  return evicted_cookies_.size();
+}
+
+void EvictedDomainCookieCounter::OnCookieChanged(const CanonicalCookie& cc,
+                                                 bool removed,
+                                                 ChangeCause cause) {
+  EvictedDomainCookieCounter::EvictedCookieKey key(GetKey(cc));
+  Time current(delegate_->CurrentTime());
+  if (removed) {
+    if (cause == CookieMonster::Delegate::CHANGE_COOKIE_EVICTED)
+      StoreEvictedCookie(key, cc, current);
+    else  // Explicit deletion, so stop tracking.
+      FreeEvictedCookie(key);
+  } else {  // Includes adds or updates.
+    ProcessNewCookie(key, cc, current);
+  }
+
+  if (next_cookie_monster_delegate_)
+    next_cookie_monster_delegate_->OnCookieChanged(cc, removed, cause);
+}
+
+// static
+EvictedDomainCookieCounter::EvictedCookieKey
+    EvictedDomainCookieCounter::GetKey(const CanonicalCookie& cc) {
+  return cc.Domain() + ";" + cc.Path() + ";" + cc.Name();
+}
+
+void EvictedDomainCookieCounter::FreeEvictedCookie(
+    const EvictedCookieKey& key) {
+  EvictedCookieMap::iterator it = evicted_cookies_.find(key);
+  if (it != evicted_cookies_.end()) {
+    delete it->second;
+    evicted_cookies_.erase(it);
+  }
+}
+
+// |current| is passed by value, in case it came from a to-be-deleted element.
+void EvictedDomainCookieCounter::FreeExpiredEvictedCookies(Time current) {
+  EvictedCookieMap::iterator it = evicted_cookies_.begin();
+  while (it != evicted_cookies_.end()) {
+    if (it->second->is_expired(current)) {
+      delete it->second;
+      evicted_cookies_.erase(it++);  // Post-increment idiom.
+    } else {
+      ++it;
+    }
+  }
+}
+
+void EvictedDomainCookieCounter::GarbageCollect(const Time& current) {
+  if (evicted_cookies_.size() <= max_size_)
+    return;
+
+  // From |evicted_cookies_|, removed all expired cookies, and remove cookies
+  // with the oldest |eviction_time_| until |size_goal| is attained.
+  size_t size_goal = max_size_ - purge_count_;
+  // Bound on number of non-expired cookies to remove.
+  size_t remove_quota = evicted_cookies_.size() - size_goal;
+  DCHECK(remove_quota > 0);
+
+  typedef EvictedDomainCookieCounter::EvictedCookie* EvictedCookiePtr;
+  // Comparator for a priority_queue<> whose top() element specifies the most
+  // recently evicted cookie.
+  struct EvictedCookieComparator {
+    bool operator() (EvictedCookiePtr ec1, EvictedCookiePtr ec2) {
+      return ec1->eviction_time_ < ec2->eviction_time_;
+    }
+  };
+
+  // Use priority queue to store oldest cookies, resize as necessary.
+  std::priority_queue<EvictedCookiePtr, std::vector<EvictedCookiePtr>,
+                      EvictedCookieComparator> oldest_set;
+  for (EvictedCookieMap::iterator it = evicted_cookies_.begin();
+      it != evicted_cookies_.end(); ++it) {
+    if (it->second->is_expired(current))  // Will be removed, so decrease quota.
+      remove_quota = remove_quota ? remove_quota - 1 : 0;  // Cap to 0 (size_t).
+    else
+      oldest_set.push(it->second);
+
+    if (oldest_set.size() > remove_quota)
+      oldest_set.pop();  // Spare the youngest element from deletion.
+  }
+  DCHECK(oldest_set.size() <= remove_quota);
+
+  // Mark oldest non-expired cookies as expired, for removal.
+  for (; !oldest_set.empty(); oldest_set.pop())
+    oldest_set.top()->set_expired();
+
+  FreeExpiredEvictedCookies(current);
+  // Apply stricter check if non-expired cookies were deleted.
+  DCHECK(remove_quota ? evicted_cookies_.size() == size_goal :
+         evicted_cookies_.size() <= size_goal);
+}
+
+void EvictedDomainCookieCounter::StoreEvictedCookie(const EvictedCookieKey& key,
+                                                    const CanonicalCookie& cc,
+                                                    const Time& current) {
+  if (!cc.IsExpired(current)) {
+    FreeEvictedCookie(key);  // Deallocate explicitly.
+
+    EvictedCookie* ec =
+        new EvictedCookie(current, cc.ExpiryDate(), UnsafeIsGoogleCookie(cc));
+    evicted_cookies_.insert(EvictedCookieMap::value_type(key, ec));
+
+    GarbageCollect(current);
+  }
+}
+
+void EvictedDomainCookieCounter::ProcessNewCookie(const EvictedCookieKey& key,
+                                                  const CanonicalCookie& cc,
+                                                  const Time& current) {
+  if (!cc.IsExpired(current)) {

[erikwright (departed), 2013/03/25 17:29:41]

This should never happen. If the pre-expired cookie matches an existing cookie,
the change reported would be a deletion with CHANGE_COOKIE_EXPIRED_OVERWRITE.
Otherwise there is no report, as no change occurs to the cookie store.

See cookie_monster.cc (SetCanonicalCookie) for the code that avoids calling
InternalInsertCookie (and therefore OnCookieChanged) in that  case.

Also, it introduces an (admittedly academic) race condition because the time can
advance between the check in cookie_monster and the check here.

So just assume (correctly) that if OnCookieChanged reports a new cookie that it
is actually being added.

[huangs, 2013/03/25 21:02:12]

Done (removed check).

+    EvictedCookieMap::iterator it = evicted_cookies_.find(key);
+    if (it != evicted_cookies_.end()) {
+      if (!it->second->is_expired(current))
+        delegate_->Report(*it->second, current);  // Reinstatement.
+      delete it->second;
+      evicted_cookies_.erase(it);
+    }
+  }
+}
+
+}  // namespace net