Supervised user SafeSites: don't cache results for more than one hour

chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc

 // Copyright 2014 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/supervised_user/experimental/supervised_user_async_url_checker.h"
 
 #include <string>
 
 #include "base/callback.h"
 #include "base/json/json_reader.h"
@@ skipping 17 matching lines @@
 using net::URLRequestContextGetter;
 using net::URLRequestStatus;
 
 namespace {
 
 const char kApiUrl[] = "https://safesearch.googleapis.com/v1:classify";
 const char kDataContentType[] = "application/x-www-form-urlencoded";
 const char kDataFormat[] = "key=%s&urls=%s";
 
 const size_t kDefaultCacheSize = 1000;
+const size_t kDefaultCacheTimeoutSeconds = 3600;
 
 // Builds the POST data for SafeSearch API requests.
 std::string BuildRequestData(const std::string& api_key, const GURL& url) {
   std::string query = net::EscapeQueryParamValue(url.spec(), true);
   return base::StringPrintf(kDataFormat, api_key.c_str(), query.c_str());
 }
 
 // Creates a URLFetcher to call the SafeSearch API for |url|.
 scoped_ptr<net::URLFetcher> CreateFetcher(URLFetcherDelegate* delegate,
                                           URLRequestContextGetter* context,
@@ skipping 39 matching lines @@
 
 struct SupervisedUserAsyncURLChecker::Check {
   Check(const GURL& url,
         scoped_ptr<net::URLFetcher> fetcher,
         const CheckCallback& callback);
   ~Check();
 
   GURL url;
   scoped_ptr<net::URLFetcher> fetcher;
   std::vector<CheckCallback> callbacks;
-  base::Time start_time;
+  base::TimeTicks start_time;

[Marc Treib, 2015/10/15 15:28:24]

Unrelated

 };
 
 SupervisedUserAsyncURLChecker::Check::Check(
     const GURL& url,
     scoped_ptr<net::URLFetcher> fetcher,
     const CheckCallback& callback)
     : url(url),
       fetcher(fetcher.Pass()),
       callbacks(1, callback),
-      start_time(base::Time::Now()) {
+      start_time(base::TimeTicks::Now()) {
 }
 
 SupervisedUserAsyncURLChecker::Check::~Check() {}
 
 SupervisedUserAsyncURLChecker::CheckResult::CheckResult(
-    SupervisedUserURLFilter::FilteringBehavior behavior, bool uncertain)
-    : behavior(behavior), uncertain(uncertain) {
-}
+    SupervisedUserURLFilter::FilteringBehavior behavior,
+    bool uncertain)
+    : behavior(behavior),
+      uncertain(uncertain),
+      timestamp(base::TimeTicks::Now()) {}
 
 SupervisedUserAsyncURLChecker::SupervisedUserAsyncURLChecker(
     URLRequestContextGetter* context)
-    : context_(context), cache_(kDefaultCacheSize) {
-}
+    : SupervisedUserAsyncURLChecker(context, kDefaultCacheSize) {}
 
 SupervisedUserAsyncURLChecker::SupervisedUserAsyncURLChecker(
     URLRequestContextGetter* context,
     size_t cache_size)
-    : context_(context), cache_(cache_size) {
-}
+    : context_(context),
+      cache_(cache_size),
+      cache_timeout_(
+          base::TimeDelta::FromSeconds(kDefaultCacheTimeoutSeconds)) {}
 
 SupervisedUserAsyncURLChecker::~SupervisedUserAsyncURLChecker() {}
 
 bool SupervisedUserAsyncURLChecker::CheckURL(const GURL& url,
                                              const CheckCallback& callback) {
-  // TODO(treib): Hack: For now, allow all Google URLs to save search QPS. If we
-  // ever remove this, we should find a way to allow at least the NTP.
+  // TODO(treib): Hack: For now, allow all Google URLs to save QPS. If we ever
+  // remove this, we should find a way to allow at least the NTP.
   if (google_util::IsGoogleDomainUrl(url,
                                      google_util::ALLOW_SUBDOMAIN,
                                      google_util::ALLOW_NON_STANDARD_PORTS)) {
     callback.Run(url, SupervisedUserURLFilter::ALLOW, false);
     return true;
   }
   // TODO(treib): Hack: For now, allow all YouTube URLs since YouTube has its
   // own Safety Mode anyway.
   if (google_util::IsYoutubeDomainUrl(url,
                                       google_util::ALLOW_SUBDOMAIN,
                                       google_util::ALLOW_NON_STANDARD_PORTS)) {
     callback.Run(url, SupervisedUserURLFilter::ALLOW, false);
     return true;
   }
 
   auto cache_it = cache_.Get(url);
   if (cache_it != cache_.end()) {
     const CheckResult& result = cache_it->second;
-    DVLOG(1) << "Cache hit! " << url.spec() << " is "
-             << (result.behavior == SupervisedUserURLFilter::BLOCK ? "NOT" : "")
-             << " safe; certain: " << !result.uncertain;
-    callback.Run(url, result.behavior, result.uncertain);
-    return true;
+    base::TimeDelta age = base::TimeTicks::Now() - result.timestamp;
+    if (age < cache_timeout_) {
+      DVLOG(1) << "Cache hit! " << url.spec() << " is "
+               << (result.behavior == SupervisedUserURLFilter::BLOCK ? "NOT"
+                                                                     : "")
+               << " safe; certain: " << !result.uncertain;
+      callback.Run(url, result.behavior, result.uncertain);
+      return true;
+    }
+    DVLOG(1) << "Outdated cache entry for " << url.spec() << ", purging";
+    cache_.Erase(cache_it);

[Bernhard Bauer, 2015/10/20 08:10:03]

Hm... just as a small possible optimization: We evict entries from the MRU cache
based on the last time they were _accessed_, but we also only consider cache
entries that were _written_ not longer than a certain period of time ago. This
means that there could potentially be values in the cache that we are not
interested in anymore, but which are still competing for space with fresher
entries. Would it make sense to do a full pass over the cache from time to time
and prune all the stale entries to reduce space pressure?

[Marc Treib, 2015/10/20 08:53:55]

I thought about it, and decided it's probably not worth the effort/overhead.
Worst case, we'll evict some still-valid entries, while keeping expired but
more-recently-accessed ones. Oh well.
If you do think it's worth it: What would be a good trigger for the cleanup
pass? On every n-th request (when the cache is full)?

[Bernhard Bauer, 2015/10/20 10:37:15]

Yes, I was thinking something like on every n-th cache eviction (where n is the
number of entries in the cache), so that we have an upper bound on unnecessarily
evicted elements, and the total overhead is amortized to constant.

It might make sense to gather some stats before we make any productive changes
to the code anyway. Do we have something like that already to get our cache hit
rate etc.?

[Marc Treib, 2015/10/20 11:53:50]

No, we don't have any stats on the cache in particular. I'll think about adding
some. (But we don't have enough users yet anyway for those to tell us
anything...)

   }
 
   // See if we already have a check in progress for this URL.
   for (Check* check : checks_in_progress_) {
     if (check->url == url) {
       DVLOG(1) << "Adding to pending check for " << url.spec();
       check->callbacks.push_back(callback);
       return false;
     }
   }
@@ skipping 27 matching lines @@
   }
 
   std::string response_body;
   source->GetResponseAsString(&response_body);
   bool is_porn = false;
   bool uncertain = !ParseResponse(response_body, &is_porn);
   SupervisedUserURLFilter::FilteringBehavior behavior =
       is_porn ? SupervisedUserURLFilter::BLOCK : SupervisedUserURLFilter::ALLOW;
 
   UMA_HISTOGRAM_TIMES("ManagedUsers.SafeSitesDelay",
-                      base::Time::Now() - check->start_time);
+                      base::TimeTicks::Now() - check->start_time);
 
   cache_.Put(check->url, CheckResult(behavior, uncertain));
 
   for (size_t i = 0; i < check->callbacks.size(); i++)
     check->callbacks[i].Run(check->url, behavior, uncertain);
   checks_in_progress_.erase(it);
 }