Add unittest for CrowdDenySafeBrowsingRequest.

The production code has also been updated in two regards:

 -- CrowdDenySafeBrowsingRequest::SafeBrowsingClient no longer assumes
    that CheckApiBlacklistUrl never invokes the callback synchronously
    if it returns false, as this can actually happen if the full hashes
    are already available on the client.

 -- DeleteOnIOThread is replaced with a manual call to DeleteSoon, as
    the former made testing difficult.

Bug: 1028642
Change-Id: Id939dd25e7ddabf6ed3529209eaa4ec2a2d7609e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1942429Reviewed-by: Andy Paicu <andypaicu@chromium.org>
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720192}

chrome/browser/permissions/crowd_deny_safe_browsing_request.cc

@@ -48,12 +48,16 @@ class CrowdDenySafeBrowsingRequest::SafeBrowsingClient
 
   void CheckOrigin(const url::Origin& origin) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+    // Start the timer before the call to CheckApiBlacklistUrl(), as it may
+    // call back into OnCheckApiBlacklistUrlResult() synchronously.
+    timeout_.Start(FROM_HERE, kSafeBrowsingCheckTimeout, this,
+                   &SafeBrowsingClient::OnTimeout);
+
     if (!database_manager_->IsSupported() ||
         database_manager_->CheckApiBlacklistUrl(origin.GetURL(), this)) {
+      timeout_.AbandonAndStop();
       SendResultToHandler(Verdict::kAcceptable);
-    } else {
-      timeout_.Start(FROM_HERE, kSafeBrowsingCheckTimeout, this,
-                     &SafeBrowsingClient::OnTimeout);
     }
   }
 
@@ -102,15 +106,18 @@ CrowdDenySafeBrowsingRequest::CrowdDenySafeBrowsingRequest(
     const url::Origin& origin,
     VerdictCallback callback)
     : callback_(std::move(callback)) {
-  client_.reset(new SafeBrowsingClient(database_manager,
-                                       weak_factory_.GetWeakPtr(),
-                                       base::SequencedTaskRunnerHandle::Get()));
+  client_ = std::make_unique<SafeBrowsingClient>(
+      database_manager, weak_factory_.GetWeakPtr(),
+      base::SequencedTaskRunnerHandle::Get());
   base::PostTask(FROM_HERE, {content::BrowserThread::IO},
                  base::BindOnce(&SafeBrowsingClient::CheckOrigin,
                                 base::Unretained(client_.get()), origin));
 }
 
-CrowdDenySafeBrowsingRequest::~CrowdDenySafeBrowsingRequest() = default;
+CrowdDenySafeBrowsingRequest::~CrowdDenySafeBrowsingRequest() {
+  content::BrowserThread::DeleteSoon(content::BrowserThread::IO, FROM_HERE,
+                                     client_.release());
+}
 
 void CrowdDenySafeBrowsingRequest::OnReceivedResult(Verdict verdict) {
   DCHECK(callback_);

chrome/browser/permissions/crowd_deny_safe_browsing_request.h

@@ -57,9 +57,8 @@ class CrowdDenySafeBrowsingRequest {
   void OnReceivedResult(Verdict verdict);
 
   // The client interfacing with Safe Browsing. Created on |this| thread, but
-  // used on the IO thread for the rest of its life.
-  std::unique_ptr<SafeBrowsingClient, content::BrowserThread::DeleteOnIOThread>
-      client_;
+  // used on the IO thread for the rest of its life and destroyed there.
+  std::unique_ptr<SafeBrowsingClient> client_;
 
   VerdictCallback callback_;
   base::WeakPtrFactory<CrowdDenySafeBrowsingRequest> weak_factory_{this};

chrome/browser/permissions/crowd_deny_safe_browsing_request_unittest.cc

+// Copyright 2017 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 "chrome/browser/permissions/crowd_deny_safe_browsing_request.h"
+
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/task/post_task.h"
+#include "base/test/mock_callback.h"
+#include "components/safe_browsing/db/database_manager.h"
+#include "components/safe_browsing/db/test_database_manager.h"
+#include "content/public/test/browser_task_environment.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+#include "url/origin.h"
+
+namespace {
+
+constexpr char kTestOriginFoo[] = "https://foo.com";
+constexpr char kTestOriginBar[] = "https://bar.com";
+
+class FakeSafeBrowsingDatabaseManager
+    : public safe_browsing::TestSafeBrowsingDatabaseManager {
+ public:
+  FakeSafeBrowsingDatabaseManager() = default;
+
+  void SetSimulatedMetadataForUrl(
+      const GURL& url,
+      const safe_browsing::ThreatMetadata& metadata) {
+    url_to_simulated_threat_metadata_.emplace(url, metadata);
+  }
+
+  void RemoveAllBlacklistedUrls() { url_to_simulated_threat_metadata_.clear(); }
+
+  void set_simulate_timeout(bool simulate_timeout) {
+    simulate_timeout_ = simulate_timeout;
+  }
+
+  void set_simulate_synchronous_result(bool simulate_synchronous_result) {
+    simulate_synchronous_result_ = simulate_synchronous_result;
+  }
+
+ protected:
+  ~FakeSafeBrowsingDatabaseManager() override {
+    EXPECT_THAT(pending_clients_, testing::IsEmpty());
+  }
+
+  safe_browsing::ThreatMetadata GetSimulatedMetadataOrSafe(const GURL& url) {
+    auto it = url_to_simulated_threat_metadata_.find(url);
+    return it != url_to_simulated_threat_metadata_.end()
+               ? it->second
+               : safe_browsing::ThreatMetadata();
+  }
+
+  // safe_browsing::TestSafeBrowsingDatabaseManager:
+  bool CheckApiBlacklistUrl(const GURL& url, Client* client) override {
+    if (simulate_synchronous_result_)
+      return true;
+
+    if (simulate_timeout_) {
+      EXPECT_THAT(pending_clients_, testing::Not(testing::Contains(client)));
+      pending_clients_.insert(client);
+    } else {
+      auto result = GetSimulatedMetadataOrSafe(url);
+      client->OnCheckApiBlacklistUrlResult(url, std::move(result));
+    }
+    return false;
+  }
+
+  bool CancelApiCheck(Client* client) override {
+    EXPECT_THAT(pending_clients_, testing::Contains(client));
+    pending_clients_.erase(client);
+    return true;
+  }
+
+  bool IsSupported() const override { return true; }
+  bool ChecksAreAlwaysAsync() const override { return false; }
+  bool CanCheckResourceType(
+      content::ResourceType /* resource_type */) const override {
+    NOTREACHED();
+    return true;
+  }
+
+  safe_browsing::ThreatSource GetThreatSource() const override {
+    NOTREACHED();
+    return safe_browsing::ThreatSource::LOCAL_PVER4;
+  }
+
+  bool CheckExtensionIDs(const std::set<std::string>& extension_ids,
+                         Client* client) override {
+    NOTREACHED();
+    return true;
+  }
+
+ private:
+  void OnCheckUrlForSubresourceFilterComplete(Client* client, const GURL& url);
+
+  std::set<Client*> pending_clients_;
+  std::map<GURL, safe_browsing::ThreatMetadata>
+      url_to_simulated_threat_metadata_;
+  bool simulate_timeout_ = false;
+  bool simulate_synchronous_result_ = false;
+
+  base::WeakPtrFactory<FakeSafeBrowsingDatabaseManager> weak_factory_{this};
+
+  DISALLOW_COPY_AND_ASSIGN(FakeSafeBrowsingDatabaseManager);
+};
+
+}  // namespace
+
+class CrowdDenySafeBrowsingRequestTest : public testing::Test {
+ public:
+  using Verdict = CrowdDenySafeBrowsingRequest::Verdict;
+
+  CrowdDenySafeBrowsingRequestTest()
+      : fake_database_manager_(
+            base::MakeRefCounted<FakeSafeBrowsingDatabaseManager>()) {}
+  ~CrowdDenySafeBrowsingRequestTest() override = default;
+
+ protected:
+  content::BrowserTaskEnvironment* task_environment() {
+    return &task_environment_;
+  }
+
+  FakeSafeBrowsingDatabaseManager* fake_database_manager() {
+    return fake_database_manager_.get();
+  }
+
+  void StartRequestForOriginAndExpectVerdict(const url::Origin& origin,
+                                             Verdict expected_verdict) {
+    base::MockOnceCallback<void(Verdict)> mock_callback_receiver;
+    CrowdDenySafeBrowsingRequest request(fake_database_manager(), origin,
+                                         mock_callback_receiver.Get());
+    EXPECT_CALL(mock_callback_receiver, Run(expected_verdict));
+    task_environment()->RunUntilIdle();
+  }
+
+ private:
+  content::BrowserTaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+
+  scoped_refptr<FakeSafeBrowsingDatabaseManager> fake_database_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(CrowdDenySafeBrowsingRequestTest);
+};
+
+TEST_F(CrowdDenySafeBrowsingRequestTest, Acceptable_SynchronousCompletion) {
+  fake_database_manager()->set_simulate_synchronous_result(true);
+  StartRequestForOriginAndExpectVerdict(
+      url::Origin::Create(GURL(kTestOriginFoo)), Verdict::kAcceptable);
+}
+
+TEST_F(CrowdDenySafeBrowsingRequestTest, Acceptable_UnavailableMetaData) {
+  StartRequestForOriginAndExpectVerdict(
+      url::Origin::Create(GURL(kTestOriginFoo)), Verdict::kAcceptable);
+}
+
+TEST_F(CrowdDenySafeBrowsingRequestTest, Acceptable_UnknownAPIName) {
+  const GURL kTestURL(kTestOriginFoo);
+
+  safe_browsing::ThreatMetadata test_metadata;
+  test_metadata.api_permissions.emplace("");
+  test_metadata.api_permissions.emplace("Stuff");
+  test_metadata.api_permissions.emplace("NOTIFICATION");   // Singular.
+  test_metadata.api_permissions.emplace("notifications");  // Lowercase.
+  fake_database_manager()->SetSimulatedMetadataForUrl(kTestURL, test_metadata);
+
+  StartRequestForOriginAndExpectVerdict(url::Origin::Create(kTestURL),
+                                        Verdict::kAcceptable);
+}
+
+TEST_F(CrowdDenySafeBrowsingRequestTest, Spammy) {
+  const GURL kTestURL(kTestOriginFoo);
+
+  safe_browsing::ThreatMetadata test_metadata;
+  test_metadata.api_permissions.emplace("BANANAS");
+  test_metadata.api_permissions.emplace("NOTIFICATIONS");
+  test_metadata.api_permissions.emplace("ORANGES");
+  fake_database_manager()->SetSimulatedMetadataForUrl(kTestURL, test_metadata);
+
+  StartRequestForOriginAndExpectVerdict(
+      url::Origin::Create(kTestURL),
+      Verdict::kKnownToShowUnsolicitedNotificationPermissionRequests);
+  StartRequestForOriginAndExpectVerdict(
+      url::Origin::Create(GURL(kTestOriginBar)), Verdict::kAcceptable);
+}
+
+TEST_F(CrowdDenySafeBrowsingRequestTest, Timeout) {
+  fake_database_manager()->set_simulate_timeout(true);
+
+  base::MockOnceCallback<void(Verdict)> mock_callback_receiver;
+  CrowdDenySafeBrowsingRequest request(
+      fake_database_manager(), url::Origin::Create(GURL(kTestOriginFoo)),
+      mock_callback_receiver.Get());
+
+  // Verify the request doesn't time out unreasonably fast.
+  EXPECT_CALL(mock_callback_receiver, Run(testing::_)).Times(0);
+  task_environment()->FastForwardBy(base::TimeDelta::FromMilliseconds(100));
+  testing::Mock::VerifyAndClearExpectations(&mock_callback_receiver);
+
+  // But that it eventually does.
+  EXPECT_CALL(mock_callback_receiver, Run(Verdict::kAcceptable));
+  task_environment()->FastForwardUntilNoTasksRemain();
+}
+
+TEST_F(CrowdDenySafeBrowsingRequestTest, AbandonedImmediately) {
+  fake_database_manager()->set_simulate_synchronous_result(true);
+
+  base::MockOnceCallback<void(Verdict)> mock_callback_receiver;
+  EXPECT_CALL(mock_callback_receiver, Run(testing::_)).Times(0);
+
+  {
+    CrowdDenySafeBrowsingRequest request(
+        fake_database_manager(), url::Origin::Create(GURL(kTestOriginFoo)),
+        mock_callback_receiver.Get());
+  }
+
+  task_environment()->RunUntilIdle();
+}
+
+TEST_F(CrowdDenySafeBrowsingRequestTest, AbandonedWhileCheckPending) {
+  fake_database_manager()->set_simulate_timeout(true);
+
+  base::MockOnceCallback<void(Verdict)> mock_callback_receiver;
+  EXPECT_CALL(mock_callback_receiver, Run(testing::_)).Times(0);
+
+  CrowdDenySafeBrowsingRequest request(
+      fake_database_manager(), url::Origin::Create(GURL(kTestOriginFoo)),
+      mock_callback_receiver.Get());
+
+  task_environment()->FastForwardBy(base::TimeDelta::FromMilliseconds(100));
+}

chrome/test/BUILD.gn

@@ -3164,6 +3164,7 @@ test("unit_tests") {
     "../browser/permissions/chooser_context_base_mock_permission_observer.cc",
     "../browser/permissions/chooser_context_base_mock_permission_observer.h",
     "../browser/permissions/chooser_context_base_unittest.cc",
+    "../browser/permissions/crowd_deny_safe_browsing_request_unittest.cc",
     "../browser/permissions/permission_context_base_feature_policy_unittest.cc",
     "../browser/permissions/permission_context_base_unittest.cc",
     "../browser/permissions/permission_decision_auto_blocker_unittest.cc",