Commit bb4a3767 authored by Findit's avatar Findit Committed by Daniel Rubery

Revert "Don't send download pings if the download is cancelled"

This reverts commit 053ca6cc.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 630043 as the
culprit for flakes in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vMDUzY2E2Y2M2ZjJhZTZhNGE3M2E5YzczZDgxNDNlYjlmMWYxNzFiMww

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.mac/Mac10.11%20Tests/33780

Sample Failed Step: unit_tests on (none) GPU on Mac on Mac-10.11

Sample Flaky Test: DownloadProtectionServiceTest.DoesNotSendPingForCancelledDownloads

Original change's description:
> 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}

Change-Id: I6c21ce34cdebe86bbf7c877f774e9d3f2ff0f59e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/1459794
Cr-Commit-Position: refs/heads/master@{#630090}
parent b9e04ac4
...@@ -593,11 +593,6 @@ std::string CheckClientDownloadRequest::SanitizeUrl(const GURL& url) const { ...@@ -593,11 +593,6 @@ std::string CheckClientDownloadRequest::SanitizeUrl(const GURL& url) const {
void CheckClientDownloadRequest::SendRequest() { void CheckClientDownloadRequest::SendRequest() {
DCHECK_CURRENTLY_ON(BrowserThread::UI); 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 request = std::make_unique<ClientDownloadRequest>();
auto population = is_extended_reporting_ auto population = is_extended_reporting_
? ChromeUserPopulation::EXTENDED_REPORTING ? ChromeUserPopulation::EXTENDED_REPORTING
......
...@@ -2730,37 +2730,4 @@ TEST_F(DownloadProtectionServiceTest, ...@@ -2730,37 +2730,4 @@ TEST_F(DownloadProtectionServiceTest,
EXPECT_EQ(referrer_chain_data->referrer_chain_length(), 3u); 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 } // namespace safe_browsing
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment