Collect some histograms about signed binary downloads.

chrome/browser/safe_browsing/download_protection_service.cc

Index: chrome/browser/safe_browsing/download_protection_service.cc
===================================================================
--- chrome/browser/safe_browsing/download_protection_service.cc	(revision 106761)
+++ chrome/browser/safe_browsing/download_protection_service.cc	(working copy)
@@ -8,7 +8,9 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
+#include "base/string_util.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/browser_thread.h"
@@ -21,6 +23,14 @@
 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")));
+}
+}  // namespace
+
 DownloadProtectionService::DownloadInfo::DownloadInfo()
     : total_bytes(0), user_initiated(false) {}
 
@@ -34,6 +44,7 @@
       enabled_(false) {}
 
 DownloadProtectionService::~DownloadProtectionService() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   STLDeleteContainerPairFirstPointers(download_requests_.begin(),
                                       download_requests_.end());
   download_requests_.clear();
@@ -45,7 +56,7 @@
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(&DownloadProtectionService::SetEnabledOnIOThread,
-                 this, enabled));
+                 base::Unretained(this), enabled));

[noelutz, 2011/10/22 00:41:46]

I just want to make sure I follow the code here.  We're assuming that we will
only delete this service object once the IO thread is going away and no-one will
flush the IO message loop?

 }
 
 void DownloadProtectionService::SetEnabledOnIOThread(bool enabled) {
@@ -98,7 +109,7 @@
         BrowserThread::UI,
         FROM_HERE,
         base::Bind(&DownloadProtectionService::EndCheckClientDownload,
-                   this, SAFE, reason, callback));
+                   base::Unretained(this), SAFE, reason, callback));
   } else {
     NOTREACHED();
   }
@@ -120,6 +131,37 @@
     RecordStats(REASON_INVALID_URL);
     return true;  // For now we only support HTTP download URLs.
   }
+
+  if (!IsBinaryFile(info.local_file)) {
+    RecordStats(REASON_NOT_BINARY_FILE);
+    return true;
+  }
+
+  // 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(&DownloadProtectionService::ExtractFileFeatures,
+                 base::Unretained(this), info, enabled_, callback));

[noelutz, 2011/10/22 00:41:46]

I think there is a race condition here.  You access enabled_ on the UI thread
but it's written and set on the IO thread.  Also, you're not really using
pingback_enabled anywhere as far as I can tell.  Could you just read enabled_ in
StartCheckClientDownload which runs on the IO thread?

+  return false;
+}
+
+void DownloadProtectionService::ExtractFileFeatures(
+    const DownloadInfo& info,
+    bool pingback_enabled,
+    const CheckDownloadCallback& callback) {
+  bool is_signed;
+  if (safe_browsing::signature_util::IsSigned(info.local_file)) {
+    VLOG(2) << "Downloaded a signed binary: " << info.local_file.value();
+    is_signed = true;
+  } else {
+    VLOG(2) << "Downloaded an unsigned binary: " << info.local_file.value();
+    is_signed = false;
+  }
+  UMA_HISTOGRAM_BOOLEAN("SBClientDownload.SignedBinaryDownload", is_signed);
+
   // TODO(noelutz): DownloadInfo should also contain the IP address of every
   // URL in the redirect chain.  We also should check whether the download URL
   // is hosted on the internal network.
@@ -127,20 +169,25 @@
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(&DownloadProtectionService::StartCheckClientDownload,
-                 this, info, callback));
-  return false;
+                 base::Unretained(this), info, pingback_enabled, callback));
 }
 
 void DownloadProtectionService::StartCheckClientDownload(
     const DownloadInfo& info,
+    bool pingback_enabled,
     const CheckDownloadCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  if (!enabled_ || !sb_service_.get()) {
-    // This is a hard fail.  We won't even call the callback in this case.
-    RecordStats(REASON_SB_DISABLED);
+  DownloadCheckResultReason reason = REASON_MAX;
+  if (!enabled_ || !sb_service_) {
+    reason = REASON_SB_DISABLED;
+    RecordStats(reason);
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&DownloadProtectionService::EndCheckClientDownload,
+                   base::Unretained(this), SAFE, reason, callback));

[noelutz, 2011/10/22 00:41:46]

Will this always work?  What if the destructor is called before
EndCheckClientDownload?

     return;
   }
-  DownloadCheckResultReason reason = REASON_MAX;
   for (size_t i = 0; i < info.download_url_chain.size(); ++i) {
     if (sb_service_->MatchDownloadWhitelistUrl(info.download_url_chain[i])) {
       reason = REASON_WHITELISTED_URL;
@@ -180,7 +227,7 @@
         BrowserThread::UI,
         FROM_HERE,
         base::Bind(&DownloadProtectionService::EndCheckClientDownload,
-                   this, SAFE, reason, callback));
+                   base::Unretained(this), SAFE, reason, callback));

[noelutz, 2011/10/22 00:41:46]

same question as above.

     return;
   }