Reland "Add browser tests for DeepScanningDialogDelegate"

This is a reland of 28640451

I was able to reproduce the issue on my Mac machine.
The reason for the failure was that the implementation of
WebContentsDestroyed was too naive, the intent of calling "delete this"
was to make the dialog go away without calling the "Cancel" callback.
This is now achieved by deleting the delegate first and updating
CancelButtonCallback so it becomes a no-op in this case (check diff
between Patchset 1 and 2 to see this fix).

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

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

chrome/browser/safe_browsing/cloud_content_scanning/binary_upload_service.h

@@ -164,7 +164,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

@@ -726,14 +726,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));
 }
@@ -745,9 +748,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
@@ -326,6 +327,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/cloud_content_scanning/deep_scanning_dialog_views.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 DeepScanningDialogViews::TestObserver {
+ public:
+  DeepScanningDialogDelegateBrowserTest() {
+    DeepScanningDialogViews::SetObserverForTesting(this);
+  }
+
+  void EnableUploadScanning() {
+    SetDMTokenForTesting(policy::DMToken::CreateValidTokenForTesting(kDmToken));
+
+    SetDlpPolicy(CHECK_UPLOADS);
+    SetMalwarePolicy(SEND_UPLOADS);
+    SetWaitPolicy(DELAY_UPLOADS);
+  }
+
+  void DestructorCalled(DeepScanningDialogViews* views) override {
+    // The test is over once the views are destroyed.
+    CallQuitClosure();
+  }
+};
+
+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;
+  SetQuitClosure(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(
+          [&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;
+          }),
+      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;
+  SetQuitClosure(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(
+          [&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;
+          }),
+      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) {
@@ -201,7 +202,7 @@ void DeepScanningDialogViews::AcceptButtonCallback() {
 }
 
 void DeepScanningDialogViews::CancelButtonCallback() {
-  DCHECK(delegate_);
+  if (delegate_)
     delegate_->Cancel(is_warning());
 }
 
@@ -229,6 +230,13 @@ ui::ModalType DeepScanningDialogViews::GetModalType() const {
   return ui::MODAL_TYPE_CHILD;
 }
 
+void DeepScanningDialogViews::WebContentsDestroyed() {
+  // If |web_contents_| is destroyed, then the scan results don't matter so the
+  // delegate can be destroyed as well.
+  delegate_.reset(nullptr);
+  CancelDialog();
+}
+
 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

@@ -1140,6 +1140,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",