- Flip the flag for improved SafeBrowsing downoad protection.

chrome/browser/safe_browsing/download_protection_service.cc

 // Copyright (c) 2011 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/download_protection_service.h"
 
 #include "base/bind.h"
 #include "base/format_macros.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/time.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/signature_util.h"
 #include "chrome/common/net/http_return.h"
 #include "chrome/common/safe_browsing/csd.pb.h"
 #include "content/browser/download/download_item.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/url_fetcher.h"
 #include "content/public/common/url_fetcher_delegate.h"
 #include "net/base/load_flags.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_status.h"
 
 using content::BrowserThread;
 
+namespace {
+static const int64 kDownloadRequestTimeoutMs = 3000;
+}  // namespace
+
 namespace safe_browsing {
 
 const char DownloadProtectionService::kDownloadRequestUrl[] =
     "https://sb-ssl.google.com/safebrowsing/clientreport/download";
 
 namespace {
 bool IsBinaryFile(const FilePath& file) {
   return (file.MatchesExtension(FILE_PATH_LITERAL(".exe")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".cab")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".msi")));
@@ skipping 97 matching lines @@
 
 // static
 DownloadProtectionService::DownloadInfo
 DownloadProtectionService::DownloadInfo::FromDownloadItem(
     const DownloadItem& item) {
   DownloadInfo download_info;
   download_info.local_file = item.full_path();
   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;
 }
 
 // Parent SafeBrowsing::Client class used to lookup the bad binary
 // URL and digest list.  There are two sub-classes (one for each list).
 class DownloadSBClient
     : public SafeBrowsingService::Client,
       public base::RefCountedThreadSafe<DownloadSBClient> {
@@ skipping 167 matching lines @@
   CheckClientDownloadRequest(const DownloadInfo& info,
                              const CheckDownloadCallback& callback,
                              DownloadProtectionService* service,
                              SafeBrowsingService* sb_service,
                              SignatureUtil* signature_util)
       : info_(info),
         callback_(callback),
         service_(service),
         signature_util_(signature_util),
         sb_service_(sb_service),
-        pingback_enabled_(service_->enabled()) {
+        pingback_enabled_(service_->enabled()),
+        finished_(false),
+        ALLOW_THIS_IN_INITIALIZER_LIST(timeout_weakptr_factory_(this)) {

[Brian Ryner, 2011/11/17 20:25:30]

#include "base/compiler_specific.h" for this macro.

     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   }
 
   void Start() {
     VLOG(2) << "Starting SafeBrowsing download check for: "
             << info_.DebugString();
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     // TODO(noelutz): implement some cache to make sure we don't issue the same
     // request over and over again if a user downloads the same binary multiple
     // times.
@@ skipping 20 matching lines @@
       return;
     }
 
     // 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.
     BrowserThread::PostTask(
         BrowserThread::FILE,
         FROM_HERE,
         base::Bind(&CheckClientDownloadRequest::ExtractFileFeatures, this));
+
+    // If the request takes too long we cancel it.
+    BrowserThread::PostDelayedTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&CheckClientDownloadRequest::Cancel,
+                   timeout_weakptr_factory_.GetWeakPtr()),
+        service_->download_request_timeout_ms());
   }
 
-  // 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() {
+    // Calling FinishRequest might delete this object if we don't keep a
+    // reference around until Cancel() is finished running.
+    scoped_refptr<CheckClientDownloadRequest> request(this);
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-    service_ = NULL;
+    FinishRequest(SAFE);
     if (fetcher_.get()) {
       // The DownloadProtectionService is going to release its reference, so we
       // might be destroyed before the URLFetcher completes.  Cancel the
       // fetcher so it does not try to invoke OnURLFetchComplete.
-      FinishRequest(SAFE);
       fetcher_.reset();
     }
     // 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.

[Brian Ryner, 2011/11/17 20:25:30]

nit: this comment is now a little out of date.  Since we've just called
FinishRequest, the callback has already been run.  If FinishRequest is called
again later, it will be a no-op.

[noelutz, 2011/11/17 21:28:40]

Done.

+    service_ = NULL;
   }
 
   // From the content::URLFetcherDelegate interface.
   virtual void OnURLFetchComplete(const content::URLFetcher* source) OVERRIDE {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     DCHECK_EQ(source, fetcher_.get());
     VLOG(2) << "Received a response for URL: "
             << info_.download_url_chain.back() << ": success="
             << source->GetStatus().is_success() << " response_code="
             << source->GetResponseCode();
@@ skipping 19 matching lines @@
     RecordImprovedProtectionStats(reason);
     FinishRequest(result);
   }
 
  private:
   friend struct BrowserThread::DeleteOnThread<BrowserThread::UI>;
   friend class DeleteTask<CheckClientDownloadRequest>;
 
   virtual ~CheckClientDownloadRequest() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    timeout_weakptr_factory_.InvalidateWeakPtrs();

[mattm, 2011/11/17 21:43:34]

the WeakPtrFactory destructor should do this already

[noelutz, 2011/11/17 21:46:06]

Done.

   }
 
   void ExtractFileFeatures() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
     signature_util_->CheckSignature(info_.local_file, &signature_info_);
     bool is_signed = (signature_info_.certificate_chain_size() > 0);
     if (is_signed) {
       VLOG(2) << "Downloaded a signed binary: " << info_.local_file.value();
     } else {
       VLOG(2) << "Downloaded an unsigned binary: " << info_.local_file.value();
@@ skipping 102 matching lines @@
       return;
     }
 
     VLOG(2) << "Sending a request for URL: "
             << info_.download_url_chain.back();
     fetcher_.reset(content::URLFetcher::Create(0 /* ID used for testing */,
                                                GURL(kDownloadRequestUrl),
                                                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();
   }
 
   void PostFinishTask(DownloadCheckResult result) {
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
         base::Bind(&CheckClientDownloadRequest::FinishRequest, this, result));
   }
 
   void FinishRequest(DownloadCheckResult result) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (finished_) {
+      return;
+    }
+    finished_ = true;
     if (service_) {
       callback_.Run(result);
       service_->RequestFinished(this);
     } else {
       callback_.Run(SAFE);
     }
   }
 
   void RecordImprovedProtectionStats(DownloadCheckResultReason reason) {
     UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckDownloadStats",
                               reason,
                               REASON_MAX);
   }
 
   DownloadInfo info_;
   ClientDownloadRequest_SignatureInfo signature_info_;
   CheckDownloadCallback callback_;
   // Will be NULL if the request has been canceled.
   DownloadProtectionService* service_;
   scoped_refptr<SignatureUtil> signature_util_;
   scoped_refptr<SafeBrowsingService> sb_service_;
   const bool pingback_enabled_;
   scoped_ptr<content::URLFetcher> fetcher_;
+  bool finished_;
+  base::WeakPtrFactory<CheckClientDownloadRequest> timeout_weakptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CheckClientDownloadRequest);
 };
 
 DownloadProtectionService::DownloadProtectionService(
     SafeBrowsingService* sb_service,
     net::URLRequestContextGetter* request_context_getter)
     : 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));
   CancelPendingRequests();
 }
 
 void DownloadProtectionService::SetEnabled(bool enabled) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (enabled == enabled_) {
     return;
@@ skipping 24 matching lines @@
   BrowserThread::PostTask(
         BrowserThread::IO,
         FROM_HERE,
         base::Bind(&DownloadUrlSBClient::StartCheck, client));
 }
 
 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(
     CheckClientDownloadRequest* request) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   std::set<scoped_refptr<CheckClientDownloadRequest> >::iterator it =
       download_requests_.find(request);
   DCHECK(it != download_requests_.end());
   download_requests_.erase(*it);
 }
 }  // namespace safe_browsing