- Flip the flag for improved SafeBrowsing downoad protection.

chrome/browser/safe_browsing/download_protection_service.cc

Index: chrome/browser/safe_browsing/download_protection_service.cc
diff --git a/chrome/browser/safe_browsing/download_protection_service.cc b/chrome/browser/safe_browsing/download_protection_service.cc
index 71fa9c3e71d3638eec0041145ec6af4bdc6f01f0..b35d864e305083026f985f53e9a45f43adafb35b 100644
--- a/chrome/browser/safe_browsing/download_protection_service.cc
+++ b/chrome/browser/safe_browsing/download_protection_service.cc
@@ -27,6 +27,10 @@
 
 using content::BrowserThread;
 
+namespace {
+static const int64 kDownloadRequestTimeoutMs = 3000;
+}  // namespace
+
 namespace safe_browsing {
 
 const char DownloadProtectionService::kDownloadRequestUrl[] =
@@ -144,7 +148,7 @@ DownloadProtectionService::DownloadInfo::FromDownloadItem(
   download_info.target_file = item.GetTargetFilePath();
   download_info.download_url_chain = item.url_chain();
   download_info.referrer_url = item.referrer_url();
-  // TODO(bryner): Fill in the hash (we shouldn't compute it again)
+  download_info.sha256_hash = item.hash();
   download_info.total_bytes = item.total_bytes();
   // TODO(bryner): Populate user_initiated
   return download_info;
@@ -332,7 +336,8 @@ class DownloadProtectionService::CheckClientDownloadRequest
         service_(service),
         signature_util_(signature_util),
         sb_service_(sb_service),
-        pingback_enabled_(service_->enabled()) {
+        pingback_enabled_(service_->enabled()),
+        finished_(false) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   }
 
@@ -366,6 +371,13 @@ class DownloadProtectionService::CheckClientDownloadRequest
       return;
     }
 
+    // If the request takes too long we cancel it.
+    BrowserThread::PostDelayedTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&CheckClientDownloadRequest::Cancel, this),
+        service_->download_request_timeout_ms());
+
     // Compute features from the file contents. Note that we record histograms
     // based on the result, so this runs regardless of whether the pingbacks
     // are enabled.  Since we do blocking I/O, this happens on the file thread.
@@ -375,12 +387,10 @@ class DownloadProtectionService::CheckClientDownloadRequest
         base::Bind(&CheckClientDownloadRequest::ExtractFileFeatures, this));
   }
 
-  // Canceling a request will cause us to always report the result as SAFE.
-  // In addition, the DownloadProtectionService will not be notified when the
-  // request finishes, so it must drop its reference after calling Cancel.
+  // Canceling a request will cause us to always report the result as SAFE
+  // unless a pending request is about to call FinishRequest.
   void Cancel() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-    service_ = NULL;
     if (fetcher_.get()) {
       // The DownloadProtectionService is going to release its reference, so we
       // might be destroyed before the URLFetcher completes.  Cancel the
@@ -391,6 +401,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
     // Note: If there is no fetcher, then some callback is still holding a
     // reference to this object.  We'll eventually wind up in some method on
     // the UI thread that will call FinishRequest() and run the callback.
+    service_ = NULL;
   }
 
   // From the content::URLFetcherDelegate interface.
@@ -552,6 +563,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
                                                content::URLFetcher::POST,
                                                this));
     fetcher_->SetLoadFlags(net::LOAD_DISABLE_CACHE);
+    fetcher_->SetAutomaticallyRetryOn5xx(false);  // Don't retry on error.
     fetcher_->SetRequestContext(service_->request_context_getter_.get());
     fetcher_->SetUploadData("application/octet-stream", request_data);
     fetcher_->Start();
@@ -566,6 +578,10 @@ class DownloadProtectionService::CheckClientDownloadRequest
 
   void FinishRequest(DownloadCheckResult result) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (finished_) {
+      return;
+    }
+    finished_ = true;
     if (service_) {
       callback_.Run(result);
       service_->RequestFinished(this);
@@ -589,6 +605,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
   scoped_refptr<SafeBrowsingService> sb_service_;
   const bool pingback_enabled_;
   scoped_ptr<content::URLFetcher> fetcher_;
+  bool finished_;
 
   DISALLOW_COPY_AND_ASSIGN(CheckClientDownloadRequest);
 };
@@ -599,7 +616,8 @@ DownloadProtectionService::DownloadProtectionService(
     : sb_service_(sb_service),
       request_context_getter_(request_context_getter),
       enabled_(false),
-      signature_util_(new SignatureUtil()) {}
+      signature_util_(new SignatureUtil()),
+      download_request_timeout_ms_(kDownloadRequestTimeoutMs) {}
 
 DownloadProtectionService::~DownloadProtectionService() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -644,10 +662,13 @@ void DownloadProtectionService::CancelPendingRequests() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   for (std::set<scoped_refptr<CheckClientDownloadRequest> >::iterator it =
            download_requests_.begin();
-       it != download_requests_.end(); ++it) {
-    (*it)->Cancel();
+       it != download_requests_.end();) {
+    // We need to advance the iterator before we cancel because canceling
+    // the request will invalidate it when RequestFinished is called below.
+    scoped_refptr<CheckClientDownloadRequest> tmp = *it++;
+    tmp->Cancel();
   }
-  download_requests_.clear();
+  DCHECK(download_requests_.empty());
 }
 
 void DownloadProtectionService::RequestFinished(