Safebrowsing: change gethash caching to match api 2.3 rules, fix some corner cases.

chrome/browser/safe_browsing/database_manager.cc

Index: chrome/browser/safe_browsing/database_manager.cc
diff --git a/chrome/browser/safe_browsing/database_manager.cc b/chrome/browser/safe_browsing/database_manager.cc
index f1b921b48943c5f6e70fb1ecbb58c39f63e1c7bc..985d3bfab87a3764d6349c5f2dad157174fc4107 100644
--- a/chrome/browser/safe_browsing/database_manager.cc
+++ b/chrome/browser/safe_browsing/database_manager.cc
@@ -336,13 +336,11 @@ bool SafeBrowsingDatabaseManager::CheckBrowseUrl(const GURL& url,
     return false;
   }
 
-  std::string list;
   std::vector<SBPrefix> prefix_hits;
-  std::vector<SBFullHashResult> full_hits;
+  std::vector<SBFullHashResult> cache_hits;
 
   bool prefix_match =
-      database_->ContainsBrowseUrl(url, &list, &prefix_hits, &full_hits,
-          sb_service_->protocol_manager()->last_update());

[Scott Hess - ex-Googler, 2014/04/01 22:08:36]

I can't quite tell if last_update() is necessary any longer.  AFAICT it's used
for histograms and for SafeBrowsingServerTest.  The latter case seems pretty
weak to me.

[mattm, 2014/04/03 01:38:12]

Perhaps that could be done another way, yeah. I can think about that later.

+      database_->ContainsBrowseUrl(url, &prefix_hits, &cache_hits);
 
   UMA_HISTOGRAM_TIMES("SB2.FilterCheck", base::TimeTicks::Now() - start);
 
@@ -356,9 +354,9 @@ bool SafeBrowsingDatabaseManager::CheckBrowseUrl(const GURL& url,
                                                    client,
                                                    safe_browsing_util::MALWARE,
                                                    expected_threats);
-  check->need_get_hash = full_hits.empty();
+  check->need_get_hash = cache_hits.empty();
   check->prefix_hits.swap(prefix_hits);
-  check->full_hits.swap(full_hits);
+  check->cache_hits.swap(cache_hits);
   checks_.insert(check);
 
   BrowserThread::PostTask(
@@ -394,7 +392,7 @@ void SafeBrowsingDatabaseManager::CancelCheck(Client* client) {
 void SafeBrowsingDatabaseManager::HandleGetHashResults(
     SafeBrowsingCheck* check,
     const std::vector<SBFullHashResult>& full_hashes,
-    bool can_cache) {
+    const base::TimeDelta& cache_lifetime) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   if (!enabled_)
@@ -412,9 +410,9 @@ void SafeBrowsingDatabaseManager::HandleGetHashResults(
   std::vector<SBPrefix> prefixes = check->prefix_hits;
   OnHandleGetHashResults(check, full_hashes);  // 'check' is deleted here.
 
-  if (can_cache && MakeDatabaseAvailable()) {
+  if (cache_lifetime != base::TimeDelta() && MakeDatabaseAvailable()) {
     // Cache the GetHash results in memory:
-    database_->CacheHashResults(prefixes, full_hashes);
+    database_->CacheHashResults(prefixes, full_hashes, cache_lifetime);
   }
 }
 
@@ -687,7 +685,10 @@ void SafeBrowsingDatabaseManager::OnCheckDone(SafeBrowsingCheck* check) {
   } else {
     // We may have cached results for previous GetHash queries.  Since
     // this data comes from cache, don't histogram hits.
-    HandleOneCheck(check, check->full_hits);
+    bool is_threat = HandleOneCheck(check, check->cache_hits);
+    // cache_hits should only contain hits for a fullhash we searched for, so if
+    // we got to this point it should always result in a threat match.
+    DCHECK(is_threat);
   }
 }
 
@@ -844,11 +845,12 @@ void SafeBrowsingDatabaseManager::OnResetDatabase() {
 }
 
 void SafeBrowsingDatabaseManager::CacheHashResults(
-  const std::vector<SBPrefix>& prefixes,
-  const std::vector<SBFullHashResult>& full_hashes) {
+    const std::vector<SBPrefix>& prefixes,
+    const std::vector<SBFullHashResult>& full_hashes,
+    const base::TimeDelta& cache_lifetime) {
   DCHECK_EQ(base::MessageLoop::current(),
             safe_browsing_thread_->message_loop());
-  GetDatabase()->CacheHashResults(prefixes, full_hashes);
+  GetDatabase()->CacheHashResults(prefixes, full_hashes, cache_lifetime);
 }
 
 void SafeBrowsingDatabaseManager::OnHandleGetHashResults(