Add caching to phishing client side detection.

chrome/browser/safe_browsing/client_side_detection_service.cc

Index: chrome/browser/safe_browsing/client_side_detection_service.cc
diff --git a/chrome/browser/safe_browsing/client_side_detection_service.cc b/chrome/browser/safe_browsing/client_side_detection_service.cc
index 008a2d2de44c1f90cd4cdaba323824fc6d4d285b..6f5698388eccad0ef51ec27c8bf4fe65e91776ad 100644
--- a/chrome/browser/safe_browsing/client_side_detection_service.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_service.cc
@@ -28,6 +28,11 @@ namespace safe_browsing {
 
 const int ClientSideDetectionService::kMaxReportsPerDay = 3;
 
+const base::TimeDelta ClientSideDetectionService::kNegativeCacheInterval =
+    base::TimeDelta::FromDays(1);
+const base::TimeDelta ClientSideDetectionService::kPositiveCacheInterval =
+    base::TimeDelta::FromMinutes(30);
+
 const char ClientSideDetectionService::kClientReportPhishingUrl[] =
     "https://sb-ssl.google.com/safebrowsing/clientreport/phishing";
 const char ClientSideDetectionService::kClientModelUrl[] =
@@ -226,7 +231,23 @@ void ClientSideDetectionService::StartClientReportPhishingRequest(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   scoped_ptr<ClientReportPhishingRequestCallback> cb(callback);
 
-  if (GetNumReportsPerDay() > kMaxReportsPerDay) {
+  bool is_phishing;
+  if (GetCachedResult(phishing_url, &is_phishing)) {
+    LOG(INFO) << "Satisfying request for " << phishing_url << " from cache";

[Brian Ryner, 2011/02/08 20:16:48]

This is probably not something that should always log.  Can you make it a VLOG?

[gcasto (DO NOT USE), 2011/02/09 00:10:52]

Done.

+    UMA_HISTOGRAM_COUNTS("SBClientPhishing.RequestSatisfiedFromCache", 1);
+    cb->Run(phishing_url, is_phishing);
+    return;
+  }
+
+  // We only limit the number of distinct urls to kMaxReportsPerDay, but we
+  // currently don't count urls already in the cache against this number. We
+  // don't want to start classifying too many pages as phishing, but for those
+  // that we already think are phishing we want to give ourselves a chance to
+  // fix false positives.
+  if (cache_.find(phishing_url) != cache_.end()) {
+    LOG(INFO) << "Refreshing cache for " << phishing_url;

[Brian Ryner, 2011/02/08 20:16:48]

Ditto here.

[gcasto (DO NOT USE), 2011/02/09 00:10:52]

Done.

+    UMA_HISTOGRAM_COUNTS("SBClientPhishing.CacheRefresh", 1);
+  } else if (GetNumReportsPerDay() > kMaxReportsPerDay) {
     LOG(WARNING) << "Too many report phishing requests sent in the last day, "

[Brian Ryner, 2011/02/08 20:16:48]

It's not directly part of this change, but this is also something that we may
not want to log in release builds.

[gcasto (DO NOT USE), 2011/02/09 00:10:52]

Done.

                  << "not checking " << phishing_url;
     UMA_HISTOGRAM_COUNTS("SBClientPhishing.RequestNotSent", 1);
@@ -306,8 +327,11 @@ void ClientSideDetectionService::HandlePhishingVerdict(
     const std::string& data) {
   ClientPhishingResponse response;
   scoped_ptr<ClientReportInfo> info(client_phishing_reports_[source]);
-  if (status.is_success() && RC_REQUEST_OK == response_code  &&
+  if (status.is_success() && RC_REQUEST_OK == response_code &&
       response.ParseFromString(data)) {
+    // Cache response, possibly flushing an old one.

[Brian Ryner, 2011/02/08 20:16:48]

I'm not totally sure I understand the "possibly flushing an old one" part of
this comment.  If we're not using a cached result for this URL (because it's too
old), won't we have already removed that entry in UpdateCache()?  If so, this
map insertion shouldn't actually cause anything to be removed from the cache.

[gcasto (DO NOT USE), 2011/02/09 00:10:52]

Your correct that this comment is currently wrong, though it's a problem with
the code not with the comment. UpdateCache really shouldn't get rid of an
element if it's not a day old regardless of if it can be used to satisfy a
request as we want to allow cache refreshes to happen. I think that I changed
the code to actually do this now. 

+    cache_[info->phishing_url] =
+        make_linked_ptr(new CacheState(response.phishy(), base::Time::Now()));
     info->callback->Run(info->phishing_url, response.phishy());
   } else {
     DLOG(ERROR) << "Unable to get the server verdict for URL: "
@@ -318,6 +342,34 @@ void ClientSideDetectionService::HandlePhishingVerdict(
   delete source;
 }
 
+bool ClientSideDetectionService::GetCachedResult(GURL url,
+                                                 bool* is_phishing) {
+  UpdateCache();
+
+  PhishingCache::iterator it = cache_.find(url);
+  if (it == cache_.end()) {
+    return false;
+  }
+
+  // Result is guaranteed to be relevant since we just updated the cache.
+  *is_phishing = it->second->is_phishing;
+  return true;
+}
+
+void ClientSideDetectionService::UpdateCache() {
+  // Remove elements from the cache that will no longer be used.
+  for (PhishingCache::iterator it = cache_.begin(); it != cache_.end();) {
+    if ((it->second->is_phishing &&

[Brian Ryner, 2011/02/08 20:16:48]

This might be more readable if you use a temporary reference variable for
it->second.  Up to you.

[gcasto (DO NOT USE), 2011/02/09 00:10:52]

Done.

+         it->second->timestamp > base::Time::Now() - kPositiveCacheInterval) ||
+        (!it->second->is_phishing &&
+         it->second->timestamp > base::Time::Now() - kNegativeCacheInterval)) {
+      ++it;
+    } else {
+      cache_.erase(it++);
+    }
+  }
+}
+
 int ClientSideDetectionService::GetNumReportsPerDay() {
   base::Time cutoff = base::Time::Now() - base::TimeDelta::FromDays(1);