Add browser tests for DeepScanningDialogDelegate

These tests use the least possible amount of overrides to ensure
DeepScanningDialogDelegate every requests/callbacks eventually resolves
and returns to the caller.

Also add code to handle the web contents being destroyed and avoid ASAN
issues.

Bug: 1041890
Change-Id: Ibfe232354f0591ae9e854bf5a0163ede68d6faf9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2083598
Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747713}

chrome/browser/safe_browsing/cloud_content_scanning/binary_upload_service.h

@@ -163,7 +163,7 @@ class BinaryUploadService {
   // Upload the given file contents for deep scanning. The results will be
   // returned asynchronously by calling |request|'s |callback|. This must be
   // called on the UI thread.
-  void UploadForDeepScanning(std::unique_ptr<Request> request);
+  virtual void UploadForDeepScanning(std::unique_ptr<Request> request);
 
   void OnGetInstanceID(Request* request, const std::string& token);
 

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_delegate.cc

@@ -744,14 +744,17 @@ void DeepScanningDialogDelegate::FillAllResultsWith(bool status) {
   std::fill(result_.paths_results.begin(), result_.paths_results.end(), status);
 }
 
+BinaryUploadService* DeepScanningDialogDelegate::GetBinaryUploadService() {
+  return g_browser_process->safe_browsing_service()->GetBinaryUploadService(
+      Profile::FromBrowserContext(web_contents_->GetBrowserContext()));
+}
+
 void DeepScanningDialogDelegate::UploadTextForDeepScanning(
     std::unique_ptr<BinaryUploadService::Request> request) {
   DCHECK_EQ(
       DlpDeepScanningClientRequest::WEB_CONTENT_UPLOAD,
       request->deep_scanning_request().dlp_scan_request().content_source());
-  BinaryUploadService* upload_service =
-      g_browser_process->safe_browsing_service()->GetBinaryUploadService(
-          Profile::FromBrowserContext(web_contents_->GetBrowserContext()));
+  BinaryUploadService* upload_service = GetBinaryUploadService();
   if (upload_service)
     upload_service->MaybeUploadForDeepScanning(std::move(request));
 }
@@ -763,9 +766,7 @@ void DeepScanningDialogDelegate::UploadFileForDeepScanning(
       !data_.do_dlp_scan ||
       (DlpDeepScanningClientRequest::FILE_UPLOAD ==
        request->deep_scanning_request().dlp_scan_request().content_source()));
-  BinaryUploadService* upload_service =
-      g_browser_process->safe_browsing_service()->GetBinaryUploadService(
-          Profile::FromBrowserContext(web_contents_->GetBrowserContext()));
+  BinaryUploadService* upload_service = GetBinaryUploadService();
   if (upload_service)
     upload_service->MaybeUploadForDeepScanning(std::move(request));
 }

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_delegate.h

@@ -29,6 +29,7 @@ class Profile;
 
 namespace safe_browsing {
 
+class BinaryUploadService;
 class DeepScanningDialogViews;
 
 // A tab modal dialog delegate that informs the user of a background deep
@@ -306,6 +307,10 @@ class DeepScanningDialogDelegate {
   // DeepScanningFinalResult enum.
   void UpdateFinalResult(DeepScanningFinalResult message);
 
+  // Returns the BinaryUploadService used to upload content for deep scanning.
+  // Virtual to override in tests.
+  virtual BinaryUploadService* GetBinaryUploadService();
+
   // The web contents that is attempting to access the data.
   content::WebContents* web_contents_ = nullptr;
 

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_delegate_browsertest.cc

+// Copyright 2020 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 <memory>
+
+#include "base/test/bind_test_util.h"
+#include "chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_browsertest_base.h"
+#include "chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_delegate.h"
+#include "chrome/browser/safe_browsing/dm_token_utils.h"
+#include "chrome/browser/ui/browser.h"
+
+namespace safe_browsing {
+
+namespace {
+
+class FakeBinaryUploadService : public BinaryUploadService {
+ public:
+  FakeBinaryUploadService() : BinaryUploadService(nullptr, nullptr, nullptr) {}
+
+  // Sets whether the user is authorized to upload data for Deep Scanning.
+  void SetAuthorized(bool authorized) {
+    authorization_result_ = authorized
+                                ? BinaryUploadService::Result::SUCCESS
+                                : BinaryUploadService::Result::UNAUTHORIZED;
+  }
+
+  // Finish the authentication request. Called after ShowForWebContents to
+  // simulate an async callback.
+  void ReturnAuthorizedResponse() {
+    authorization_request_->FinishRequest(authorization_result_,
+                                          DeepScanningClientResponse());
+  }
+
+  void SetResponseForText(BinaryUploadService::Result result,
+                          const DeepScanningClientResponse& response) {
+    prepared_text_result_ = result;
+    prepared_text_response_ = response;
+  }
+
+  void SetResponseForFile(const std::string& path,
+                          BinaryUploadService::Result result,
+                          const DeepScanningClientResponse& response) {
+    prepared_file_results_[path] = result;
+    prepared_file_responses_[path] = response;
+  }
+
+  int requests_count() const { return requests_count_; }
+
+ private:
+  void UploadForDeepScanning(std::unique_ptr<Request> request) override {
+    // The first uploaded request is the authentication one.
+    if (++requests_count_ == 1) {
+      authorization_request_.swap(request);
+    } else {
+      std::string file = request->deep_scanning_request().filename();
+      if (file.empty()) {
+        request->FinishRequest(prepared_text_result_, prepared_text_response_);
+      } else {
+        ASSERT_TRUE(prepared_file_results_.count(file));
+        ASSERT_TRUE(prepared_file_responses_.count(file));
+        request->FinishRequest(prepared_file_results_[file],
+                               prepared_file_responses_[file]);
+      }
+    }
+  }
+
+  BinaryUploadService::Result authorization_result_;
+  std::unique_ptr<Request> authorization_request_;
+  BinaryUploadService::Result prepared_text_result_;
+  DeepScanningClientResponse prepared_text_response_;
+  std::map<std::string, BinaryUploadService::Result> prepared_file_results_;
+  std::map<std::string, DeepScanningClientResponse> prepared_file_responses_;
+  int requests_count_ = 0;
+};
+
+FakeBinaryUploadService* FakeBinaryUploadServiceStorage() {
+  static FakeBinaryUploadService service;
+  return &service;
+}
+
+// A fake delegate with minimal overrides to obtain behavior that's as close to
+// the real one as possible.
+class MinimalFakeDeepScanningDialogDelegate
+    : public DeepScanningDialogDelegate {
+ public:
+  MinimalFakeDeepScanningDialogDelegate(
+      content::WebContents* web_contents,
+      DeepScanningDialogDelegate::Data data,
+      DeepScanningDialogDelegate::CompletionCallback callback)
+      : DeepScanningDialogDelegate(web_contents,
+                                   std::move(data),
+                                   std::move(callback),
+                                   DeepScanAccessPoint::UPLOAD) {}
+
+  static std::unique_ptr<DeepScanningDialogDelegate> Create(
+      content::WebContents* web_contents,
+      DeepScanningDialogDelegate::Data data,
+      DeepScanningDialogDelegate::CompletionCallback callback) {
+    return std::make_unique<MinimalFakeDeepScanningDialogDelegate>(
+        web_contents, std::move(data), std::move(callback));
+  }
+
+ private:
+  BinaryUploadService* GetBinaryUploadService() override {
+    return FakeBinaryUploadServiceStorage();
+  }
+};
+
+constexpr char kDmToken[] = "dm_token";
+
+}  // namespace
+
+// Tests the behavior of the dialog delegate with minimal overriding of methods.
+// Only responses obtained via the BinaryUploadService are faked.
+class DeepScanningDialogDelegateBrowserTest
+    : public DeepScanningBrowserTestBase {
+ public:
+  DeepScanningDialogDelegateBrowserTest() = default;
+
+  void EnableUploadScanning() {
+    SetDMTokenForTesting(policy::DMToken::CreateValidTokenForTesting(kDmToken));
+
+    SetDlpPolicy(CHECK_UPLOADS);
+    SetMalwarePolicy(SEND_UPLOADS);
+    SetWaitPolicy(DELAY_UPLOADS);
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(DeepScanningDialogDelegateBrowserTest, Unauthorized) {
+  EnableUploadScanning();
+
+  DeepScanningDialogDelegate::SetFactoryForTesting(
+      base::BindRepeating(&MinimalFakeDeepScanningDialogDelegate::Create));
+
+  FakeBinaryUploadServiceStorage()->SetAuthorized(false);
+
+  bool called = false;
+  base::RunLoop run_loop;
+  base::RepeatingClosure quit_closure = run_loop.QuitClosure();
+
+  DeepScanningDialogDelegate::Data data;
+  data.do_dlp_scan = true;
+  data.do_malware_scan = true;
+  data.text.emplace_back(base::UTF8ToUTF16("foo"));
+  data.paths.emplace_back(FILE_PATH_LITERAL("/tmp/foo.doc"));
+
+  DeepScanningDialogDelegate::ShowForWebContents(
+      browser()->tab_strip_model()->GetActiveWebContents(), std::move(data),
+      base::BindLambdaForTesting(
+          [&quit_closure, &called](
+              const DeepScanningDialogDelegate::Data& data,
+              const DeepScanningDialogDelegate::Result& result) {
+            ASSERT_EQ(result.text_results.size(), 1u);
+            ASSERT_EQ(result.paths_results.size(), 1u);
+            ASSERT_TRUE(result.text_results[0]);
+            ASSERT_TRUE(result.paths_results[0]);
+            called = true;
+            quit_closure.Run();
+          }),
+      DeepScanAccessPoint::UPLOAD);
+
+  FakeBinaryUploadServiceStorage()->ReturnAuthorizedResponse();
+
+  run_loop.Run();
+  EXPECT_TRUE(called);
+
+  // Only 1 request (the authentication one) should have been uploaded.
+  ASSERT_EQ(FakeBinaryUploadServiceStorage()->requests_count(), 1);
+}
+
+IN_PROC_BROWSER_TEST_F(DeepScanningDialogDelegateBrowserTest, Files) {
+  base::ScopedAllowBlockingForTesting allow_blocking;
+
+  // Create the files to be opened and scanned.
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+
+  base::FilePath ok_path = temp_dir.GetPath().AppendASCII("ok.doc");
+  base::FilePath bad_path = temp_dir.GetPath().AppendASCII("bad.exe");
+
+  base::File ok_file(ok_path, base::File::FLAG_CREATE | base::File::FLAG_WRITE);
+  base::File bad_file(bad_path,
+                      base::File::FLAG_CREATE | base::File::FLAG_WRITE);
+
+  std::string ok_content = "ok file content";
+  std::string bad_content = "bad file content";
+
+  ok_file.WriteAtCurrentPos(ok_content.data(), ok_content.size());
+  bad_file.WriteAtCurrentPos(bad_content.data(), bad_content.size());
+
+  // Set up delegate and upload service.
+  EnableUploadScanning();
+
+  DeepScanningDialogDelegate::SetFactoryForTesting(
+      base::BindRepeating(&MinimalFakeDeepScanningDialogDelegate::Create));
+
+  DeepScanningClientResponse ok_response;
+  ok_response.mutable_dlp_scan_verdict()->set_status(
+      DlpDeepScanningVerdict::SUCCESS);
+  ok_response.mutable_malware_scan_verdict()->set_verdict(
+      MalwareDeepScanningVerdict::CLEAN);
+
+  DeepScanningClientResponse bad_response;
+  bad_response.mutable_dlp_scan_verdict()->set_status(
+      DlpDeepScanningVerdict::SUCCESS);
+  bad_response.mutable_malware_scan_verdict()->set_verdict(
+      MalwareDeepScanningVerdict::MALWARE);
+
+  FakeBinaryUploadServiceStorage()->SetAuthorized(true);
+  FakeBinaryUploadServiceStorage()->SetResponseForFile(
+      "ok.doc", BinaryUploadService::Result::SUCCESS, ok_response);
+  FakeBinaryUploadServiceStorage()->SetResponseForFile(
+      "bad.exe", BinaryUploadService::Result::SUCCESS, bad_response);
+
+  bool called = false;
+  base::RunLoop run_loop;
+  base::RepeatingClosure quit_closure = run_loop.QuitClosure();
+
+  DeepScanningDialogDelegate::Data data;
+  data.do_dlp_scan = true;
+  data.do_malware_scan = true;
+  data.paths.emplace_back(ok_path);
+  data.paths.emplace_back(bad_path);
+
+  // Start test.
+  DeepScanningDialogDelegate::ShowForWebContents(
+      browser()->tab_strip_model()->GetActiveWebContents(), std::move(data),
+      base::BindLambdaForTesting(
+          [&quit_closure, &called](
+              const DeepScanningDialogDelegate::Data& data,
+              const DeepScanningDialogDelegate::Result& result) {
+            ASSERT_TRUE(result.text_results.empty());
+            ASSERT_EQ(result.paths_results.size(), 2u);
+            ASSERT_TRUE(result.paths_results[0]);
+            ASSERT_FALSE(result.paths_results[1]);
+            called = true;
+            quit_closure.Run();
+          }),
+      DeepScanAccessPoint::UPLOAD);
+
+  FakeBinaryUploadServiceStorage()->ReturnAuthorizedResponse();
+
+  run_loop.Run();
+  EXPECT_TRUE(called);
+
+  // There should have been 1 request per file and 1 for authentication.
+  ASSERT_EQ(FakeBinaryUploadServiceStorage()->requests_count(), 3);
+}
+
+IN_PROC_BROWSER_TEST_F(DeepScanningDialogDelegateBrowserTest, Texts) {
+  // Set up delegate and upload service.
+  EnableUploadScanning();
+
+  DeepScanningDialogDelegate::SetFactoryForTesting(
+      base::BindRepeating(&MinimalFakeDeepScanningDialogDelegate::Create));
+
+  FakeBinaryUploadServiceStorage()->SetAuthorized(true);
+  DeepScanningClientResponse response =
+      SimpleDeepScanningClientResponseForTesting(/*dlp=*/false,
+                                                 /*malware=*/base::nullopt);
+  FakeBinaryUploadServiceStorage()->SetResponseForText(
+      BinaryUploadService::Result::SUCCESS, response);
+
+  bool called = false;
+  base::RunLoop run_loop;
+  base::RepeatingClosure quit_closure = run_loop.QuitClosure();
+
+  DeepScanningDialogDelegate::Data data;
+  data.do_dlp_scan = true;
+  data.do_malware_scan = true;
+  data.text.emplace_back(base::UTF8ToUTF16("text1"));
+  data.text.emplace_back(base::UTF8ToUTF16("text2"));
+
+  // Start test.
+  DeepScanningDialogDelegate::ShowForWebContents(
+      browser()->tab_strip_model()->GetActiveWebContents(), std::move(data),
+      base::BindLambdaForTesting(
+          [&quit_closure, &called](
+              const DeepScanningDialogDelegate::Data& data,
+              const DeepScanningDialogDelegate::Result& result) {
+            ASSERT_TRUE(result.paths_results.empty());
+            ASSERT_EQ(result.text_results.size(), 2u);
+            ASSERT_FALSE(result.text_results[0]);
+            ASSERT_FALSE(result.text_results[1]);
+            called = true;
+            quit_closure.Run();
+          }),
+      DeepScanAccessPoint::UPLOAD);
+
+  FakeBinaryUploadServiceStorage()->ReturnAuthorizedResponse();
+
+  run_loop.Run();
+  EXPECT_TRUE(called);
+
+  // There should have been 1 request for all texts and 1 for authentication.
+  ASSERT_EQ(FakeBinaryUploadServiceStorage()->requests_count(), 2);
+}
+
+}  // namespace safe_browsing

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_views.cc

@@ -176,7 +176,8 @@ DeepScanningDialogViews::DeepScanningDialogViews(
     content::WebContents* web_contents,
     DeepScanAccessPoint access_point,
     bool is_file_scan)
-    : delegate_(std::move(delegate)),
+    : content::WebContentsObserver(web_contents),
+      delegate_(std::move(delegate)),
       web_contents_(web_contents),
       access_point_(std::move(access_point)),
       is_file_scan_(is_file_scan) {
@@ -229,6 +230,10 @@ ui::ModalType DeepScanningDialogViews::GetModalType() const {
   return ui::MODAL_TYPE_CHILD;
 }
 
+void DeepScanningDialogViews::WebContentsDestroyed() {
+  delete this;
+}
+
 void DeepScanningDialogViews::ShowResult(
     DeepScanningDialogDelegate::DeepScanningFinalResult result) {
   DCHECK(is_pending());

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_views.h

@@ -11,6 +11,7 @@
 #include "base/time/time.h"
 #include "chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_delegate.h"
 #include "chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_utils.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "ui/views/animation/bounds_animator.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/window/dialog_delegate.h"
@@ -38,7 +39,8 @@ class DeepScanningMessageView;
 
 // Dialog shown for Deep Scanning to offer the possibility of cancelling the
 // upload to the user.
-class DeepScanningDialogViews : public views::DialogDelegate {
+class DeepScanningDialogViews : public views::DialogDelegate,
+                                public content::WebContentsObserver {
  public:
   // Enum used to represent what the dialog is currently showing.
   enum class DeepScanningDialogStatus {
@@ -119,6 +121,9 @@ class DeepScanningDialogViews : public views::DialogDelegate {
   void DeleteDelegate() override;
   ui::ModalType GetModalType() const override;
 
+  // content::WebContentsObserver:
+  void WebContentsDestroyed() override;
+
   // Updates the dialog with the result, and simply delete it from memory if
   // nothing should be shown.
   void ShowResult(DeepScanningDialogDelegate::DeepScanningFinalResult result);

chrome/test/BUILD.gn

@@ -1165,6 +1165,7 @@ if (!is_android) {
       "../browser/safe_browsing/chrome_cleaner/reporter_runner_browsertest_win.cc",
       "../browser/safe_browsing/cloud_content_scanning/deep_scanning_browsertest_base.cc",
       "../browser/safe_browsing/cloud_content_scanning/deep_scanning_browsertest_base.h",
+      "../browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_delegate_browsertest.cc",
       "../browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_views_browsertest.cc",
       "../browser/safe_browsing/download_protection/download_protection_service_browsertest.cc",
       "../browser/safe_browsing/test_safe_browsing_database_helper.cc",