Handle CheckClientDownloadRequest different when download is destroyed to avoid crash

When a download is being destroyed while a CheckClientDownloadRequest
is in-flight, we ignore the callback to CheckClientDownloadDone.
This prevents crash that caused by DownloadManagerImpl being destructed
before CheckClientDownloadDone during browser shutdown.

A new UMA enum is added to track this special case.

Bug: 830303
Change-Id: Iff5146a163e357ebd9b97b97cf48c6d7af9a7783
Reviewed-on: https://chromium-review.googlesource.com/1024864
Commit-Queue: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554201}

chrome/browser/safe_browsing/download_protection/check_client_download_request.cc

@@ -155,7 +155,7 @@ void CheckClientDownloadRequest::StartTimeout() {
   BrowserThread::PostDelayedTask(
       BrowserThread::UI, FROM_HERE,
       base::BindOnce(&CheckClientDownloadRequest::Cancel,
-                     weakptr_factory_.GetWeakPtr()),
+                     weakptr_factory_.GetWeakPtr(), false),
       base::TimeDelta::FromMilliseconds(
           service_->download_request_timeout_ms()));
 }
@@ -163,7 +163,7 @@ void CheckClientDownloadRequest::StartTimeout() {
 // Canceling a request will cause us to always report the result as
 // DownloadCheckResult::UNKNOWN unless a pending request is about to call
 // FinishRequest.
-void CheckClientDownloadRequest::Cancel() {
+void CheckClientDownloadRequest::Cancel(bool download_destroyed) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   cancelable_task_tracker_.TryCancelAll();
   if (loader_.get()) {
@@ -176,7 +176,9 @@ void CheckClientDownloadRequest::Cancel() {
   // reference to this object.  We'll eventually wind up in some method on
   // the UI thread that will call FinishRequest() again.  If FinishRequest()
   // is called a second time, it will be a no-op.
-  FinishRequest(DownloadCheckResult::UNKNOWN, REASON_REQUEST_CANCELED);
+  FinishRequest(DownloadCheckResult::UNKNOWN, download_destroyed
+                                                  ? REASON_DOWNLOAD_DESTROYED
+                                                  : REASON_REQUEST_CANCELED);
   // Calling FinishRequest might delete this object, we may be deleted by
   // this point.
 }
@@ -184,7 +186,7 @@ void CheckClientDownloadRequest::Cancel() {
 // download::DownloadItem::Observer implementation.
 void CheckClientDownloadRequest::OnDownloadDestroyed(
     download::DownloadItem* download) {
-  Cancel();
+  Cancel(/*download_destroyed=*/true);
   DCHECK(item_ == NULL);
 }
 
@@ -1061,7 +1063,8 @@ void CheckClientDownloadRequest::FinishRequest(
              << " result:" << static_cast<int>(result);
     UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckDownloadStats", reason,
                               REASON_MAX);
-    callback_.Run(result);
+    if (reason != REASON_DOWNLOAD_DESTROYED)
+      callback_.Run(result);
     item_->RemoveObserver(this);
     item_ = NULL;
     DownloadProtectionService* service = service_;

chrome/browser/safe_browsing/download_protection/check_client_download_request.h

@@ -57,7 +57,10 @@ class CheckClientDownloadRequest
   bool ShouldSampleUnsupportedFile(const base::FilePath& filename);
   void Start();
   void StartTimeout();
-  void Cancel();
+
+  // |download_destroyed| indicates if cancellation is due to the destruction of
+  // the download item.
+  void Cancel(bool download_destroyed);
   void OnDownloadDestroyed(download::DownloadItem* download) override;
   void OnURLLoaderComplete(std::unique_ptr<std::string> response_body);
   static bool IsSupportedDownload(const download::DownloadItem& item,

chrome/browser/safe_browsing/download_protection/download_protection_service.cc

@@ -200,7 +200,7 @@ void DownloadProtectionService::CancelPendingRequests() {
     // 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();
+    tmp->Cancel(/*download_destropyed=*/false);
   }
   DCHECK(download_requests_.empty());
 

chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc

@@ -2026,7 +2026,9 @@ TEST_F(DownloadProtectionServiceTest, TestDownloadItemDestroyed) {
     // notification.
   }
 
-  EXPECT_TRUE(IsResult(DownloadCheckResult::UNKNOWN));
+  // When download is destroyed, no need to check for client download request
+  // result.
+  EXPECT_FALSE(has_result_);
   EXPECT_FALSE(HasClientDownloadRequest());
 }
 
@@ -2054,13 +2056,13 @@ TEST_F(DownloadProtectionServiceTest,
                   tmp_path_, BinaryFeatureExtractor::kDefaultOptions, _, _))
       .Times(0);
 
-  RunLoop run_loop;
   download_service_->CheckClientDownload(
-      item.get(), base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
-                             base::Unretained(this), run_loop.QuitClosure()));
+      item.get(),
+      base::BindRepeating(&DownloadProtectionServiceTest::SyncCheckDoneCallback,
+                          base::Unretained(this)));
+  base::RunLoop().RunUntilIdle();
 
-  run_loop.Run();
-  EXPECT_TRUE(IsResult(DownloadCheckResult::UNKNOWN));
+  EXPECT_FALSE(has_result_);
   EXPECT_FALSE(HasClientDownloadRequest());
 }
 

chrome/browser/safe_browsing/download_protection/download_protection_util.h

@@ -53,6 +53,7 @@ enum DownloadCheckResultReason {
   REASON_REMOTE_FILE = 25,
   REASON_SAMPLED_UNSUPPORTED_FILE = 26,
   REASON_VERDICT_UNKNOWN = 27,
+  REASON_DOWNLOAD_DESTROYED = 28,
   REASON_MAX  // Always add new values before this one.
 };
 

tools/metrics/histograms/enums.xml

@@ -39614,6 +39614,7 @@ Called by update_net_trust_anchors.py.-->
   <int value="25" label="REMOTE_FILE"/>
   <int value="26" label="SAMPLED_UNSUPPORTED_FILE"/>
   <int value="27" label="VERDICT_UNKNOWN"/>
+  <int value="28" label="DOWNLOAD_DESTROYED"/>
 </enum>
 
 <enum name="SBClientDownloadCheckResult">