Don't send download pings if the download is cancelled

There's a small race condition where, if the download is cancelled
before we send a download ping, when we notify the
IncidentReportingService, we'll fail a DCHECK, since the
IncidentReportingService expects us to only send pings for IN_PROGRESS
or COMPLETED downloads.

Change-Id: If9e5647f2e60f19f0469f51709990ae8226cd5ee
Reviewed-on: https://chromium-review.googlesource.com/c/1457672
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#630043}

chrome/browser/safe_browsing/download_protection/check_client_download_request.cc

@@ -593,6 +593,11 @@ std::string CheckClientDownloadRequest::SanitizeUrl(const GURL& url) const {
 void CheckClientDownloadRequest::SendRequest() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
+  if (item_->GetState() == download::DownloadItem::CANCELLED) {
+    FinishRequest(DownloadCheckResult::UNKNOWN, REASON_DOWNLOAD_DESTROYED);
+    return;
+  }
+
   auto request = std::make_unique<ClientDownloadRequest>();
   auto population = is_extended_reporting_
                         ? ChromeUserPopulation::EXTENDED_REPORTING

chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc

@@ -2730,4 +2730,37 @@ TEST_F(DownloadProtectionServiceTest,
   EXPECT_EQ(referrer_chain_data->referrer_chain_length(), 3u);
 }
 
+TEST_F(DownloadProtectionServiceTest, DoesNotSendPingForCancelledDownloads) {
+  PrepareResponse(ClientDownloadResponse::SAFE, net::HTTP_OK,
+                  net::URLRequestStatus::SUCCESS);
+
+  NiceMockDownloadItem item;
+  PrepareBasicDownloadItem(&item, {"http://www.evil.com/a.exe"},  // url_chain
+                           "http://www.google.com/",              // referrer
+                           FILE_PATH_LITERAL("a.tmp"),            // tmp_path
+                           FILE_PATH_LITERAL("a.exe"));           // final_path
+
+  // Mock a cancelled download.
+  EXPECT_CALL(item, GetState())
+      .WillRepeatedly(Return(download::DownloadItem::CANCELLED));
+
+  EXPECT_CALL(*sb_service_->mock_database_manager(),
+              MatchDownloadWhitelistUrl(_))
+      .WillRepeatedly(Return(false));
+  EXPECT_CALL(*binary_feature_extractor_.get(), CheckSignature(tmp_path_, _));
+  EXPECT_CALL(*binary_feature_extractor_.get(),
+              ExtractImageFeatures(
+                  tmp_path_, BinaryFeatureExtractor::kDefaultOptions, _, _));
+
+  RunLoop run_loop;
+  download_service_->CheckClientDownload(
+      &item,
+      base::BindRepeating(&DownloadProtectionServiceTest::CheckDoneCallback,
+                          base::Unretained(this), run_loop.QuitClosure()));
+  run_loop.RunUntilIdle();
+
+  EXPECT_FALSE(has_result_);
+  EXPECT_FALSE(HasClientDownloadRequest());
+}
+
 }  // namespace safe_browsing