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

chrome/browser/safe_browsing/protocol_manager.cc

Index: chrome/browser/safe_browsing/protocol_manager.cc
diff --git a/chrome/browser/safe_browsing/protocol_manager.cc b/chrome/browser/safe_browsing/protocol_manager.cc
index 6c1377906cc8257a06e0ad1c647ac77dd019c12a..ae83a9bade6c2c084e1dada0fc620f9412b5543c 100644
--- a/chrome/browser/safe_browsing/protocol_manager.cc
+++ b/chrome/browser/safe_browsing/protocol_manager.cc
@@ -54,6 +54,10 @@ void RecordUpdateResult(UpdateResult result) {
 
 }  // namespace
 
+// The maximum staleness for a cached entry.
+// TODO(mattm): remove this when switching to Safebrowsing API 2.3.
+static const int kMaxStalenessMinutes = 45;
+
 // Minimum time, in seconds, from start up before we must issue an update query.
 static const int kSbTimerStartIntervalSecMin = 60;
 
@@ -172,7 +176,7 @@ void SafeBrowsingProtocolManager::GetFullHash(
   // required to return empty results (i.e. treat the page as safe).
   if (gethash_error_count_ && Time::Now() <= next_gethash_time_) {
     std::vector<SBFullHashResult> full_hashes;
-    callback.Run(full_hashes, false);
+    callback.Run(full_hashes, base::TimeDelta());
     return;
   }
   GURL gethash_url = GetHashUrl();
@@ -218,7 +222,7 @@ void SafeBrowsingProtocolManager::OnURLFetchComplete(
     fetcher.reset(it->first);
     const FullHashDetails& details = it->second;
     std::vector<SBFullHashResult> full_hashes;
-    bool can_cache = false;
+    base::TimeDelta cache_lifetime;
     if (source->GetStatus().is_success() &&
         (source->GetResponseCode() == 200 ||
          source->GetResponseCode() == 204)) {
@@ -228,7 +232,9 @@ void SafeBrowsingProtocolManager::OnURLFetchComplete(
         RecordGetHashResult(details.is_download, GET_HASH_STATUS_200);
       else
         RecordGetHashResult(details.is_download, GET_HASH_STATUS_204);
-      can_cache = true;
+      // In SB 2.3, cache lifetime will be part of the protocol message. For
+      // now, use the old value of 45 minutes.
+      cache_lifetime = base::TimeDelta::FromMinutes(kMaxStalenessMinutes);
       gethash_error_count_ = 0;
       gethash_back_off_mult_ = 1;
       SafeBrowsingProtocolParser parser;
@@ -240,7 +246,7 @@ void SafeBrowsingProtocolManager::OnURLFetchComplete(
           &full_hashes);
       if (!parsed_ok) {
         full_hashes.clear();
-        // TODO(cbentzel): Should can_cache be set to false here?
+        // TODO(cbentzel): Should cache_lifetime be set to 0 here?

[Scott Hess - ex-Googler, 2014/04/03 21:44:03]

Is there a tracking thing for this?  It seems like a form of negative caching -
if the response is invalid in the service for structural reasons, we wouldn't
want to pound the server fetching additional invalid responses, so instead we
cache a negative hit for the lifetime.  BUT, if the response is invalid, we also
don't want to trust the parsed lifetime info.  So maybe there should be a fixed
term for caching such things.

[mattm, 2014/04/04 23:58:49]

Didn't find one, so I filed http://crbug.com/360232

Also added a histogram for it (and for the urlrequest errors too)

       }
     } else {
       HandleGetHashError(Time::Now());
@@ -256,7 +262,7 @@ void SafeBrowsingProtocolManager::OnURLFetchComplete(
     // Invoke the callback with full_hashes, even if there was a parse error or
     // an error response code (in which case full_hashes will be empty). The
     // caller can't be blocked indefinitely.
-    details.callback.Run(full_hashes, can_cache);
+    details.callback.Run(full_hashes, cache_lifetime);
 
     hash_requests_.erase(it);
   } else {