Add caching to phishing client side detection.

chrome/browser/safe_browsing/client_side_detection_service.cc

 // Copyright (c) 2011 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 "chrome/browser/safe_browsing/client_side_detection_service.h"
 
 #include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/file_util_proxy.h"
 #include "base/logging.h"
@@ skipping 10 matching lines @@
 #include "chrome/common/net/url_fetcher.h"
 #include "chrome/common/net/url_request_context_getter.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/load_flags.h"
 #include "net/url_request/url_request_status.h"
 
 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[] =
     "https://ssl.gstatic.com/safebrowsing/csd/client_model_v0.pb";
 
 struct ClientSideDetectionService::ClientReportInfo {
   scoped_ptr<ClientReportPhishingRequestCallback> callback;
   GURL phishing_url;
 };
 
@@ skipping 178 matching lines @@
   }
 }
 
 void ClientSideDetectionService::StartClientReportPhishingRequest(
     const GURL& phishing_url,
     double score,
     ClientReportPhishingRequestCallback* callback) {
   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);
     cb->Run(phishing_url, false);
     return;
   }
 
   ClientPhishingRequest request;
   request.set_url(phishing_url.spec());
   request.set_client_score(static_cast<float>(score));
   std::string request_data;
@@ skipping 58 matching lines @@
 
 void ClientSideDetectionService::HandlePhishingVerdict(
     const URLFetcher* source,
     const GURL& url,
     const net::URLRequestStatus& status,
     int response_code,
     const ResponseCookies& cookies,
     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: "
                 << info->phishing_url;
     info->callback->Run(info->phishing_url, false);
   }
   client_phishing_reports_.erase(source);
   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);
 
   // Erase elements older than a day because we will never care about them
   // again.
   while (!phishing_report_times_.empty() &&
          phishing_report_times_.front() < cutoff) {
     phishing_report_times_.pop();
   }
 
   // Return the number of elements that are above the cutoff.
   return phishing_report_times_.size();
 }
 
 }  // namespace safe_browsing