Sample 1% url whitelisted PPAPI downloads to ping SB server

Similar to regular downloads, we want SB pings for requestors whose
URLs match the SB whitelist in flash downloads. To achieve that, we
sample 1% of whitelisted PPAPI downloads based on the
SafeBrowsingService::whitelist_sample_rate() if the user is in
extended reporting group and not in incognito mode.

Also refactored download_protection_service_unittest to elliminate
the use of MessageLoop (deprecated).

BUG=610924

Review-Url: https://codereview.chromium.org/2146703002
Cr-Commit-Position: refs/heads/master@{#406101}

chrome/browser/file_select_helper.cc

@@ -532,6 +532,7 @@ void FileSelectHelper::CheckDownloadRequestWithSafeBrowsing(
   GURL requestor_url = params->requestor;
   sb_service->download_protection_service()->CheckPPAPIDownloadRequest(
       requestor_url, default_file_path, alternate_extensions,
+      Profile::FromBrowserContext(web_contents_->GetBrowserContext()),
       base::Bind(&InterpretSafeBrowsingVerdict,
                  base::Bind(&FileSelectHelper::ProceedWithSafeBrowsingVerdict,
                             this, default_file_path, base::Passed(&params))));

chrome/browser/safe_browsing/download_protection_service.cc

@@ -73,9 +73,23 @@
 using content::BrowserThread;
 
 namespace {
-static const int64_t kDownloadRequestTimeoutMs = 7000;
+
+const int64_t kDownloadRequestTimeoutMs = 7000;
 // We sample 1% of whitelisted downloads to still send out download pings.
-static const double kWhitelistDownloadSampleRate = 0.01;
+const double kWhitelistDownloadSampleRate = 0.01;
+
+enum WhitelistType {
+  NO_WHITELIST_MATCH,
+  URL_WHITELIST,
+  SIGNATURE_WHITELIST,
+  WHITELIST_TYPE_MAX
+};
+
+void RecordCountOfWhitelistedDownload(WhitelistType type) {
+  UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckWhitelistResult", type,
+                            WHITELIST_TYPE_MAX);
+}
+
 }  // namespace
 
 namespace safe_browsing {
@@ -760,20 +774,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
   }
 #endif  // defined(OS_MACOSX)
 
-  enum WhitelistType {
-    NO_WHITELIST_MATCH,
-    URL_WHITELIST,
-    SIGNATURE_WHITELIST,
-    WHITELIST_TYPE_MAX
-  };
-
-  static void RecordCountOfWhitelistedDownload(WhitelistType type) {
-    UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckWhitelistResult",
-                              type,
-                              WHITELIST_TYPE_MAX);
-  }
-
-  virtual bool ShouldSampleWhitelistedDownload() {
+  bool ShouldSampleWhitelistedDownload() {
     // We currently sample 1% whitelisted downloads from users who opted
     // in extended reporting and are not in incognito mode.
     return service_ && is_extended_reporting_ && !is_incognito_ &&
@@ -1200,6 +1201,7 @@ class DownloadProtectionService::PPAPIDownloadRequest
       const GURL& requestor_url,
       const base::FilePath& default_file_path,
       const std::vector<base::FilePath::StringType>& alternate_extensions,
+      Profile* profile,
       const CheckDownloadCallback& callback,
       DownloadProtectionService* service,
       scoped_refptr<SafeBrowsingDatabaseManager> database_manager)
@@ -1212,7 +1214,13 @@ class DownloadProtectionService::PPAPIDownloadRequest
         start_time_(base::TimeTicks::Now()),
         supported_path_(
             GetSupportedFilePath(default_file_path, alternate_extensions)),
-        weakptr_factory_(this) {}
+        sample_url_whitelist_(false),
+        weakptr_factory_(this) {
+    DCHECK(profile);
+    is_extended_reporting_ = profile->GetPrefs()->GetBoolean(
+        prefs::kSafeBrowsingExtendedReportingEnabled);
+    is_incognito_ = profile->IsOffTheRecord();
+  }
 
   ~PPAPIDownloadRequest() override {
     if (fetcher_ && !callback_.is_null())
@@ -1262,6 +1270,13 @@ class DownloadProtectionService::PPAPIDownloadRequest
   }
 
  private:
+  bool ShouldSampleWhitelistedDownload() {
+    // We currently sample 1% whitelisted downloads from users who opted
+    // in extended reporting and are not in incognito mode.
+    return service_ && !is_incognito_ && is_extended_reporting_ &&
+           base::RandDouble() < service_->whitelist_sample_rate();
+  }
+
   // Whitelist checking needs to the done on the IO thread.
   static void CheckWhitelistsOnIOThread(
       const GURL& requestor_url,
@@ -1282,10 +1297,14 @@ class DownloadProtectionService::PPAPIDownloadRequest
   void WhitelistCheckComplete(bool was_on_whitelist) {
     DVLOG(2) << __FUNCTION__ << " was_on_whitelist:" << was_on_whitelist;
     if (was_on_whitelist) {
-      // TODO(asanka): Should sample whitelisted downloads based on
-      // service_->whitelist_sample_rate(). http://crbug.com/610924
-      Finish(RequestOutcome::WHITELIST_HIT, SAFE);
-      return;
+      RecordCountOfWhitelistedDownload(URL_WHITELIST);
+      if (!ShouldSampleWhitelistedDownload()) {
+        Finish(RequestOutcome::WHITELIST_HIT, SAFE);
+        return;
+      }
+      sample_url_whitelist_ = true;
+    } else {
+      RecordCountOfWhitelistedDownload(NO_WHITELIST_MATCH);
     }
 
     // Not on whitelist, so we are going to check with the SafeBrowsing
@@ -1306,6 +1325,11 @@ class DownloadProtectionService::PPAPIDownloadRequest
     request.set_file_basename(supported_path_.BaseName().AsUTF8Unsafe());
     request.set_length(0);
     request.mutable_digests()->set_md5(std::string());
+    request.set_skipped_url_whitelist(sample_url_whitelist_);
+    // Download protection does not check certificate whitelist for
+    // base::RunLoop()
+    // downloads.
+    request.set_skipped_certificate_whitelist(false);
     for (const auto& alternate_extension : alternate_extensions_) {
       if (alternate_extension.empty())
         continue;
@@ -1368,12 +1392,13 @@ class DownloadProtectionService::PPAPIDownloadRequest
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     DVLOG(2) << __FUNCTION__ << " response: " << response;
     UMA_HISTOGRAM_SPARSE_SLOWLY(
-        "SBClientDownload.PPAPIDownloadRequest.RequestOutcome",
+        "SBClientDownload.base::RunLoop()DownloadRequest.RequestOutcome",
         static_cast<int>(reason));
-    UMA_HISTOGRAM_SPARSE_SLOWLY("SBClientDownload.PPAPIDownloadRequest.Result",
-                                response);
-    UMA_HISTOGRAM_TIMES("SBClientDownload.PPAPIDownloadRequest.RequestDuration",
-                        start_time_ - base::TimeTicks::Now());
+    UMA_HISTOGRAM_SPARSE_SLOWLY(
+        "SBClientDownload.base::RunLoop()DownloadRequest.Result", response);
+    UMA_HISTOGRAM_TIMES(
+        "SBClientDownload.base::RunLoop()DownloadRequest.RequestDuration",
+        start_time_ - base::TimeTicks::Now());
     if (!callback_.is_null())
       base::ResetAndReturn(&callback_).Run(response);
     fetcher_.reset();
@@ -1458,6 +1483,12 @@ class DownloadProtectionService::PPAPIDownloadRequest
   // ping.
   const base::FilePath supported_path_;
 
+  bool sample_url_whitelist_;
+
+  bool is_extended_reporting_;
+
+  bool is_incognito_;
+
   base::WeakPtrFactory<PPAPIDownloadRequest> weakptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(PPAPIDownloadRequest);
@@ -1563,12 +1594,13 @@ void DownloadProtectionService::CheckPPAPIDownloadRequest(
     const GURL& requestor_url,
     const base::FilePath& default_file_path,
     const std::vector<base::FilePath::StringType>& alternate_extensions,
+    Profile* profile,
     const CheckDownloadCallback& callback) {
   DVLOG(1) << __FUNCTION__ << " url:" << requestor_url
            << " default_file_path:" << default_file_path.value();
   std::unique_ptr<PPAPIDownloadRequest> request(new PPAPIDownloadRequest(
-      requestor_url, default_file_path, alternate_extensions, callback, this,
-      database_manager_));
+      requestor_url, default_file_path, alternate_extensions, profile, callback,
+      this, database_manager_));
   PPAPIDownloadRequest* request_copy = request.get();
   auto insertion_result = ppapi_download_requests_.insert(
       std::make_pair(request_copy, std::move(request)));

chrome/browser/safe_browsing/download_protection_service.h

@@ -28,7 +28,6 @@
 #include "net/url_request/url_request_context_getter.h"
 #include "url/gurl.h"
 
-
 namespace content {
 class DownloadItem;
 class PageNavigator;
@@ -38,6 +37,8 @@ namespace net {
 class X509Certificate;
 }  // namespace net
 
+class Profile;
+
 namespace safe_browsing {
 class BinaryFeatureExtractor;
 class ClientDownloadRequest;
@@ -115,6 +116,7 @@ class DownloadProtectionService {
       const GURL& requestor_url,
       const base::FilePath& default_file_path,
       const std::vector<base::FilePath::StringType>& alternate_extensions,
+      Profile* profile,
       const CheckDownloadCallback& callback);
 
   // Display more information to the user regarding the download specified by

chrome/test/ppapi/ppapi_filechooser_browsertest.cc

@@ -166,6 +166,7 @@ class FakeDownloadProtectionService : public DownloadProtectionService {
       const GURL& requestor_url,
       const base::FilePath& default_file_path,
       const std::vector<base::FilePath::StringType>& alternate_extensions,
+      Profile* profile_unused,
       const CheckDownloadCallback& callback) override {
     const auto iter =
         test_configuration_->result_map.find(default_file_path.Extension());