Sample 1% url whitelisted PPAPI downloads to ping safe browsing server

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 2381394d23b89fff2dd471b930f8ddf92471a992..6ed3f5110c7588b7f69b2028eb497bbf150fb8c2 100644
--- a/chrome/browser/safe_browsing/download_protection_service.cc
+++ b/chrome/browser/safe_browsing/download_protection_service.cc
@@ -76,6 +76,18 @@ namespace {
 static const int64_t kDownloadRequestTimeoutMs = 7000;
 // We sample 1% of whitelisted downloads to still send out download pings.
 static const double kWhitelistDownloadSampleRate = 0.01;
+
+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);
+}
 }  // namespace
 
 namespace safe_browsing {
@@ -753,20 +765,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_ &&
@@ -1193,18 +1192,21 @@ 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)
       : requestor_url_(requestor_url),
         default_file_path_(default_file_path),
         alternate_extensions_(alternate_extensions),
+        profile_(profile),

[asanka, 2016/07/14 20:44:32]

Note that DownloadProtectionService has not lifetime tie-in with Profile. As
such, storing a pointer to a Profile can be dangerous. Since we are now in
/safe_browsing/ you can now call ShouldSampleWhitelistedDownload() and store the
resulting boolean rather than keep Profile* around.

[Jialiu Lin, 2016/07/14 21:35:40]

Make sense. Thanks!

         callback_(callback),
         service_(service),
         database_manager_(database_manager),
         start_time_(base::TimeTicks::Now()),
         supported_path_(
             GetSupportedFilePath(default_file_path, alternate_extensions)),
+        sample_url_whitelist_(false),
         weakptr_factory_(this) {}
 
   ~PPAPIDownloadRequest() override {
@@ -1255,6 +1257,18 @@ 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.
+    bool is_extended_reporting =
+        profile_ &&
+        profile_->GetPrefs()->GetBoolean(
+            prefs::kSafeBrowsingExtendedReportingEnabled);
+    bool is_incognito = profile_ && profile_->IsOffTheRecord();
+    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,
@@ -1275,10 +1289,13 @@ 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;
+      if (ShouldSampleWhitelistedDownload()) {
+        RecordCountOfWhitelistedDownload(URL_WHITELIST);
+        sample_url_whitelist_ = true;
+      } else {
+        Finish(RequestOutcome::WHITELIST_HIT, SAFE);
+        return;
+      }
     }
 
     // Not on whitelist, so we are going to check with the SafeBrowsing
@@ -1299,6 +1316,10 @@ 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 PPAPI
+    // downloads.
+    request.set_skipped_certificate_whitelist(false);
     for (const auto& alternate_extension : alternate_extensions_) {
       if (alternate_extension.empty())
         continue;
@@ -1433,6 +1454,8 @@ class DownloadProtectionService::PPAPIDownloadRequest
   // must begin with a leading extension separator.
   const std::vector<base::FilePath::StringType> alternate_extensions_;
 
+  Profile* profile_;
+
   // Callback to invoke with the result of the PPAPI download request check.
   CheckDownloadCallback callback_;
 
@@ -1449,6 +1472,8 @@ class DownloadProtectionService::PPAPIDownloadRequest
   // ping.
   const base::FilePath supported_path_;
 
+  bool sample_url_whitelist_;
+
   base::WeakPtrFactory<PPAPIDownloadRequest> weakptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(PPAPIDownloadRequest);
@@ -1554,12 +1579,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)));