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 106177)
+++ 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,13 +23,22 @@
 const char DownloadProtectionService::kDownloadRequestUrl[] =
     "https://sb-ssl.google.com/safebrowsing/clientreport/download";
 
+namespace {
+bool IsBinaryFile(const FilePath& file) {

[mattm, 2011/10/19 18:54:12]

"Binary" doesn't seem like quite the right term.. not coming up with any
brilliant alternatives though

[Brian Ryner, 2011/10/19 19:20:11]

I could use "executable" if you think that's better.

+  FilePath::StringType extension = file.Extension();

[noelutz, 2011/10/19 16:12:49]

Maybe wrap this in StringToLowerASCII to make sure the comparison below does the
right thing?

[Brian Ryner, 2011/10/19 18:06:59]

I ended up replacing this with calls to FilePath::MatchesExtension(), which is
case insensitive.

+  return (EndsWith(extension, FILE_PATH_LITERAL("exe"), false) ||
+          EndsWith(extension, FILE_PATH_LITERAL("cab"), false) ||
+          EndsWith(extension, FILE_PATH_LITERAL("msi"), false));
+}
+}  // namespace
+
 DownloadProtectionService::DownloadInfo::DownloadInfo()
     : total_bytes(0), user_initiated(false) {}
 
 DownloadProtectionService::DownloadInfo::~DownloadInfo() {}
 
 DownloadProtectionService::DownloadProtectionService(
-    SafeBrowsingService* sb_service,
+    const base::WeakPtr<SafeBrowsingService>& sb_service,

[mattm, 2011/10/19 18:54:12]

Hmm, still not sure about these WeakPtr's but I'm not sure this is safe. 
weak_ptr.h says a WeakPtr should only be derefed on the thread that created the
WeakPtr, which I think is UI for this, but it's derefed on IO thread later on.

[Brian Ryner, 2011/10/19 19:20:11]

Hm, good point.  Maybe we need to AddRef the SafeBrowsingService during the time
we're executing on the IO thread?  See my comment below about the rationale for
the weak pointer.

     net::URLRequestContextGetter* request_context_getter)
     : sb_service_(sb_service),
       request_context_getter_(request_context_getter),
@@ -120,6 +131,34 @@
     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,
+                 this, info, callback));
+  return false;
+}
+
+void DownloadProtectionService::ExtractFileFeatures(
+    const DownloadInfo& info,
+    const CheckDownloadCallback& callback) {
+  if (safe_browsing::signature_util::IsSigned(info.local_file)) {
+    VLOG(2) << "Downloaded a signed binary: " << info.local_file.value();
+    UMA_HISTOGRAM_COUNTS("SBClientDownload.SignedBinaryDownload", 1);

[mattm, 2011/10/19 18:54:12]

Could use UMA_HISTOGRAM_BOOLEAN to get them both in the same histogram.

[Brian Ryner, 2011/10/19 19:20:11]

Done.

+  } else {
+    VLOG(2) << "Downloaded an unsigned binary: " << info.local_file.value();
+    UMA_HISTOGRAM_COUNTS("SBClientDownload.UnsignedBinaryDownload", 1);
+  }
+
   // 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.
@@ -128,7 +167,6 @@
       FROM_HERE,
       base::Bind(&DownloadProtectionService::StartCheckClientDownload,
                  this, info, callback));
-  return false;
 }
 
 void DownloadProtectionService::StartCheckClientDownload(