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

chrome/browser/safe_browsing/protocol_manager.cc

 // Copyright (c) 2012 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/protocol_manager.h"
 
 #ifndef NDEBUG
 #include "base/base64.h"
 #endif
 #include "base/environment.h"
@@ skipping 36 matching lines @@
   UPDATE_RESULT_BACKUP_START = UPDATE_RESULT_BACKUP_CONNECT_FAIL,
 };
 
 void RecordUpdateResult(UpdateResult result) {
   DCHECK(result >= 0 && result < UPDATE_RESULT_MAX);
   UMA_HISTOGRAM_ENUMERATION("SB2.UpdateResult", result, UPDATE_RESULT_MAX);
 }
 
 }  // 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;
 
 // Maximum time, in seconds, from start up before we must issue an update query.
 static const int kSbTimerStartIntervalSecMax = 300;
 
 // The maximum time, in seconds, to wait for a response to an update request.
 static const int kSbMaxUpdateWaitSec = 30;
 
 // Maximum back off multiplier.
@@ skipping 98 matching lines @@
 void SafeBrowsingProtocolManager::GetFullHash(
     const std::vector<SBPrefix>& prefixes,
     FullHashCallback callback,
     bool is_download) {
   DCHECK(CalledOnValidThread());
   // If we are in GetHash backoff, we need to check if we're past the next
   // allowed time. If we are, we can proceed with the request. If not, we are
   // 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();
   net::URLFetcher* fetcher = net::URLFetcher::Create(
       url_fetcher_id_++, gethash_url, net::URLFetcher::POST, this);
   hash_requests_[fetcher] = FullHashDetails(callback, is_download);
 
   std::string get_hash;
   SafeBrowsingProtocolParser parser;
   parser.FormatGetHash(prefixes, &get_hash);
@@ skipping 25 matching lines @@
   DCHECK(CalledOnValidThread());
   scoped_ptr<const net::URLFetcher> fetcher;
   bool parsed_ok = true;
 
   HashRequests::iterator it = hash_requests_.find(source);
   if (it != hash_requests_.end()) {
     // GetHash response.
     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)) {
       // For tracking our GetHash false positive (204) rate, compared to real
       // (200) responses.
       if (source->GetResponseCode() == 200)
         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;
       std::string data;
       source->GetResponseAsString(&data);
       parsed_ok = parser.ParseGetHash(
           data.data(),
           static_cast<int>(data.length()),
           &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());
       if (source->GetStatus().status() == net::URLRequestStatus::FAILED) {
         VLOG(1) << "SafeBrowsing GetHash request for: " << source->GetURL()
                 << " failed with error: " << source->GetStatus().error();
       } else {
         VLOG(1) << "SafeBrowsing GetHash request for: " << source->GetURL()
                 << " failed with error: " << source->GetResponseCode();
       }
     }
 
     // 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 {
     // Update or chunk response.
     fetcher.reset(request_.release());
 
     if (request_type_ == UPDATE_REQUEST ||
         request_type_ == BACKUP_UPDATE_REQUEST) {
       if (!fetcher.get()) {
         // We've timed out waiting for an update response, so we've cancelled
@@ skipping 502 matching lines @@
     FullHashCallback callback, bool is_download)
     : callback(callback),
       is_download(is_download) {
 }
 
 SafeBrowsingProtocolManager::FullHashDetails::~FullHashDetails() {
 }
 
 SafeBrowsingProtocolManagerDelegate::~SafeBrowsingProtocolManagerDelegate() {
 }