Include high-fidelity metadata about a download in incident reports.

chrome/browser/safe_browsing/download_protection_service.cc

 // Copyright (c) 2012 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/compiler_specific.h"
 #include "base/format_macros.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 714 matching lines @@
     request.set_file_basename(
         item_->GetTargetFilePath().BaseName().AsUTF8Unsafe());
     request.set_download_type(type_);
     request.mutable_signature()->CopyFrom(signature_info_);
     request.mutable_image_headers()->CopyFrom(image_headers_);
     if (!request.SerializeToString(&client_download_request_data_)) {
       FinishRequest(UNKNOWN, REASON_INVALID_REQUEST_PROTO);
       return;
     }
 
+    service_->client_download_request_callbacks_.Notify(item_, &request);
+
     VLOG(2) << "Sending a request for URL: "
             << item_->GetUrlChain().back();
     fetcher_.reset(net::URLFetcher::Create(0 /* ID used for testing */,
                                            GetDownloadRequestUrl(),
                                            net::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",
@@ skipping 30 matching lines @@
     }
     if (!timeout_start_time_.is_null()) {
       UMA_HISTOGRAM_ENUMERATION("SBClientDownload.DownloadRequestTimeoutStats",
                                 reason,
                                 REASON_MAX);
       if (reason != REASON_REQUEST_CANCELED) {
         UMA_HISTOGRAM_TIMES("SBClientDownload.DownloadRequestTimeoutDuration",
                             base::TimeTicks::Now() - timeout_start_time_);
       }
     }
+    if (result == SAFE && (reason == REASON_WHITELISTED_URL ||
+                           reason == REASON_TRUSTED_EXECUTABLE)) {

[mattm, 2014/10/24 02:11:10]

This seems error prone, I'm not confident this condition is correct, though I'm
not entirely sure what it's trying to do.

[grt (UTC plus 2), 2014/10/30 19:07:35]

I've clarified the comment and added TODOs to the related code in
CheckWhitelists. I hope to address these TODOs in a separate change since
they'll involve changing the CheckClientDownloadRequest state machine.

+      // A ClientDownloadRequest was not generated for this download, so
+      // callbakcs were not run with one. Run them with null so they see that a

[mattm, 2014/10/24 02:11:09]

callbacks

[grt (UTC plus 2), 2014/10/30 19:07:35]

Done.

+      // previously-seen ClientDownloadRequest is now stale.

[mattm, 2014/10/24 02:11:10]

This comment doesn't make sense to me. How would anything have seen a
ClientDownloadRequest if none was ever generated?

[grt (UTC plus 2), 2014/10/30 19:07:35]

From an unrelated previous download. I'll clarify in the comment.

+      // TODO(grt): persist metadata for these downloads as well.
+      service_->client_download_request_callbacks_.Notify(item_, nullptr);
+    }
     if (service_) {
       VLOG(2) << "SafeBrowsing download verdict for: "
               << item_->DebugString(true) << " verdict:" << reason
               << " result:" << result;
       UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckDownloadStats",
                                 reason,
                                 REASON_MAX);
 #if defined(OS_MACOSX)
       // OSX is currently sending pings only for evaluation purposes, ignore
       // the result for now.
@@ skipping 159 matching lines @@
   ClientDownloadRequest::DownloadType type =
       ClientDownloadRequest::WIN_EXECUTABLE;
   return (CheckClientDownloadRequest::IsSupportedDownload(
               item, target_path, &reason, &type) &&
           (ClientDownloadRequest::CHROME_EXTENSION != type));
 #else
   return false;
 #endif
 }
 
+DownloadProtectionService::ClientDownloadRequestSubscription
+DownloadProtectionService::RegisterClientDownloadRequestCallback(
+    const ClientDownloadRequestCallback& callback) {
+  return client_download_request_callbacks_.Add(callback);
+}
+
 void DownloadProtectionService::CancelPendingRequests() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   for (std::set<scoped_refptr<CheckClientDownloadRequest> >::iterator it =
            download_requests_.begin();
        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();
   }
@@ skipping 102 matching lines @@
 GURL DownloadProtectionService::GetDownloadRequestUrl() {
   GURL url(kDownloadRequestUrl);
   std::string api_key = google_apis::GetAPIKey();
   if (!api_key.empty())
     url = url.Resolve("?key=" + net::EscapeQueryParamValue(api_key, true));
 
   return url;
 }
 
 }  // namespace safe_browsing