Refactor CheckUrlForHighConfidenceAllowlist to return only true or false

The local allowlist only contains full hashes, so this CL eliminates the
code paths that are currently handling a hash-prefix match, which would
trigger a full hash check ping.

This involves:
 - Returning true where previously we returned MATCH, returning false
otherwise, and never running the full hash check.
 - Removing the OnCheckUrlForHighConfidenceAllowlist function from
SafeBrowsingDatabaseManager::Client, since that is only useful for cases
where the database manager responds to the client asynchronously.
 - Removing ClientCallbackType::CHECK_HIGH_CONFIDENCE_ALLOWLIST and the
|client| parameter from the CheckUrlForHighConfidenceAllowlist function.
Reading from the database still requires a PendingCheck, so just like
HandleUrlSynchronously, we pass through ClientCallbackType::CHECK_OTHER
for the callback type and nullptr for the client.

Separately, this also includes:
 - Removing some lingering high-confidence allowlist code from the
AllowlistCheckerClient, which had already previously had the high-
confidence allowlist checking functionality removed.
 - Changing SafeBrowsing.RT.LocalMatch.Result histogram ownership per
vakh@'s suggestion.

Bug: 1372485
Change-Id: I9f5b651ad9ae43a12c33f16f1bb702618265f45e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4032247
Commit-Queue: thefrog <thefrog@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1073589}

components/safe_browsing/android/remote_database_manager.cc

@@ -25,9 +25,9 @@
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
 using content::BrowserThread;
-
 namespace safe_browsing {
 
+using IsInAllowlistResult = RealTimeUrlChecksAllowlist::IsInAllowlistResult;
 namespace {
 
 // Android field trial for controlling types_to_check.
@@ -240,29 +240,24 @@ bool RemoteSafeBrowsingDatabaseManager::CheckResourceUrl(const GURL& url,
   return true;
 }
 
-AsyncMatch
-RemoteSafeBrowsingDatabaseManager::CheckUrlForHighConfidenceAllowlist(
-    const GURL& url,
-    Client* client) {
+bool RemoteSafeBrowsingDatabaseManager::CheckUrlForHighConfidenceAllowlist(
+    const GURL& url) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (!enabled_ || !CanCheckUrl(url))
-    return AsyncMatch::NO_MATCH;
+    return false;
 
   if (base::FeatureList::IsEnabled(kComponentUpdaterAndroidProtegoAllowlist)) {
     // SafeBrowsingComponentUpdaterAndroidProtegoAllowlist is enabled.
-    RealTimeUrlChecksAllowlist::IsInAllowlistResult is_match =
+    IsInAllowlistResult match_result =
         RealTimeUrlChecksAllowlist::GetInstance()->IsInAllowlist(url);
-    // Note that if the allowlist is unavailable, we return MATCH
-    return is_match == RealTimeUrlChecksAllowlist::IsInAllowlistResult::
-                           kNotInAllowlist
-               ? AsyncMatch::NO_MATCH
-               : AsyncMatch::MATCH;
+    // Note that if the allowlist is unavailable, we say that is a match.
+    return match_result == IsInAllowlistResult::kInAllowlist ||
+           match_result == IsInAllowlistResult::kAllowlistUnavailable;
   }
-  // TODO(crbug.com/1014202): Make this call async.
-  bool is_match = SafeBrowsingApiHandlerBridge::GetInstance()
-                      .StartHighConfidenceAllowlistCheck(url);
-  return is_match ? AsyncMatch::MATCH : AsyncMatch::NO_MATCH;
+
+  return SafeBrowsingApiHandlerBridge::GetInstance()
+      .StartHighConfidenceAllowlistCheck(url);
 }
 
 bool RemoteSafeBrowsingDatabaseManager::CheckUrlForAccuracyTips(

components/safe_browsing/android/remote_database_manager.h

@@ -53,8 +53,7 @@ class RemoteSafeBrowsingDatabaseManager : public SafeBrowsingDatabaseManager {
                          Client* client) override;
   AsyncMatch CheckCsdAllowlistUrl(const GURL& url, Client* client) override;
   bool CheckResourceUrl(const GURL& url, Client* client) override;
-  AsyncMatch CheckUrlForHighConfidenceAllowlist(const GURL& url,
-                                                Client* client) override;
+  bool CheckUrlForHighConfidenceAllowlist(const GURL& url) override;
   bool CheckUrlForAccuracyTips(const GURL& url, Client* client) override;
   bool CheckUrlForSubresourceFilter(const GURL& url, Client* client) override;
   bool MatchDownloadAllowlistUrl(const GURL& url) override;

components/safe_browsing/core/browser/db/allowlist_checker_client.cc

@@ -106,11 +106,6 @@ void AllowlistCheckerClient::OnCheckAllowlistUrlResult(
   OnCheckUrlResult(did_match_allowlist);
 }
 
-void AllowlistCheckerClient::OnCheckUrlForHighConfidenceAllowlist(
-    bool did_match_allowlist) {
-  OnCheckUrlResult(did_match_allowlist);
-}
-
 void AllowlistCheckerClient::OnCheckUrlResult(bool did_match_allowlist) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   timer_.Stop();

components/safe_browsing/core/browser/db/allowlist_checker_client.h

@@ -42,7 +42,6 @@ class AllowlistCheckerClient : public SafeBrowsingDatabaseManager::Client {
 
   // SafeBrowsingDatabaseMananger::Client impl
   void OnCheckAllowlistUrlResult(bool is_allowlisted) override;
-  void OnCheckUrlForHighConfidenceAllowlist(bool did_match_allowlist) override;
 
  private:
   // Helper method to instantiate a AllowlistCheckerClient object.

components/safe_browsing/core/browser/db/allowlist_checker_client_unittest.cc

@@ -44,9 +44,6 @@ class MockSafeBrowsingDatabaseManager : public TestSafeBrowsingDatabaseManager {
   MOCK_METHOD2(CheckCsdAllowlistUrl,
                AsyncMatch(const GURL&, SafeBrowsingDatabaseManager::Client*));
 
-  MOCK_METHOD2(CheckUrlForHighConfidenceAllowlist,
-               AsyncMatch(const GURL&, SafeBrowsingDatabaseManager::Client*));
-
  protected:
   ~MockSafeBrowsingDatabaseManager() override {}
 };

components/safe_browsing/core/browser/db/database_manager.h

@@ -92,11 +92,6 @@ class SafeBrowsingDatabaseManager
     // Currently only used for CSD allowlist.
     virtual void OnCheckAllowlistUrlResult(bool did_match_allowlist) {}
 
-    // Called when the result of checking the high-confidence allowlist is
-    // known.
-    virtual void OnCheckUrlForHighConfidenceAllowlist(
-        bool did_match_allowlist) {}
-
     // Called when the result of checking for accuracy tips is known.
     virtual void OnCheckUrlForAccuracyTip(bool should_show_accuracy_tip) {}
   };
@@ -187,13 +182,12 @@ class SafeBrowsingDatabaseManager
                                             Client* client) = 0;
 
   // Called on the IO thread to check whether |url| is safe by checking if it
-  // appears on a high-confidence allowlist. The 3-state return value indicates
-  // the result or that |client| will get a callback later with the result.
-  // The high confidence allowlist is a list of partial or full hashes of URLs
-  // that are expected to be safe so in the case of a match on this list, the
-  // realtime full URL Safe Browsing lookup isn't performed.
-  virtual AsyncMatch CheckUrlForHighConfidenceAllowlist(const GURL& url,
-                                                        Client* client) = 0;
+  // appears on a high-confidence allowlist. The return value is true if it
+  // matches the allowlist, and is false if it does not. The high confidence
+  // allowlist is a list of full hashes of URLs that are expected to be safe so
+  // in the case of a match on this list, the realtime full URL Safe Browsing
+  // lookup isn't performed.
+  virtual bool CheckUrlForHighConfidenceAllowlist(const GURL& url) = 0;
 
   // Called on the IO thread to check whether |url| should show an accuracy tip.
   // If we can synchronously determine that the url shouldn't trigger an

components/safe_browsing/core/browser/db/test_database_manager.cc

@@ -67,11 +67,10 @@ bool TestSafeBrowsingDatabaseManager::CheckResourceUrl(const GURL& url,
   return true;
 }
 
-AsyncMatch TestSafeBrowsingDatabaseManager::CheckUrlForHighConfidenceAllowlist(
-    const GURL& url,
-    Client* client) {
+bool TestSafeBrowsingDatabaseManager::CheckUrlForHighConfidenceAllowlist(
+    const GURL& url) {
   NOTIMPLEMENTED();
-  return AsyncMatch::NO_MATCH;
+  return false;
 }
 
 bool TestSafeBrowsingDatabaseManager::CheckUrlForSubresourceFilter(

components/safe_browsing/core/browser/db/test_database_manager.h

@@ -40,8 +40,7 @@ class TestSafeBrowsingDatabaseManager : public SafeBrowsingDatabaseManager {
   bool CheckExtensionIDs(const std::set<std::string>& extension_ids,
                          Client* client) override;
   bool CheckResourceUrl(const GURL& url, Client* client) override;
-  AsyncMatch CheckUrlForHighConfidenceAllowlist(const GURL& url,
-                                                Client* client) override;
+  bool CheckUrlForHighConfidenceAllowlist(const GURL& url) override;
   bool CheckUrlForSubresourceFilter(const GURL& url, Client* client) override;
   bool CheckUrlForAccuracyTips(const GURL& url, Client* client) override;
   bool MatchDownloadAllowlistUrl(const GURL& url) override;

components/safe_browsing/core/browser/db/v4_local_database_manager.cc

@@ -10,6 +10,7 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
+#include "base/check_op.h"
 #include "base/command_line.h"
 #include "base/containers/contains.h"
 #include "base/containers/fixed_flat_map.h"
@@ -504,9 +505,8 @@ bool V4LocalDatabaseManager::CheckResourceUrl(const GURL& url, Client* client) {
   return HandleCheck(std::move(check));
 }
 
-AsyncMatch V4LocalDatabaseManager::CheckUrlForHighConfidenceAllowlist(
-    const GURL& url,
-    Client* client) {
+bool V4LocalDatabaseManager::CheckUrlForHighConfidenceAllowlist(
+    const GURL& url) {
   DCHECK(io_task_runner()->RunsTasksInCurrentSequence());
 
   StoresToCheck stores_to_check({GetUrlHighConfidenceAllowlistId()});
@@ -525,17 +525,20 @@ AsyncMatch V4LocalDatabaseManager::CheckUrlForHighConfidenceAllowlist(
       (!all_stores_available && is_artificial_prefix_empty)) {
     // NOTE(vakh): If Safe Browsing isn't enabled yet, or if the URL isn't a
     // navigation URL, or if the allowlist isn't ready yet, or if the allowlist
-    // is too small, return MATCH. The full URL check won't be performed, but
-    // hash-based check will still be done. If any artificial matches are
-    // present, consider the allowlist as ready.
-    return AsyncMatch::MATCH;
+    // is too small, return that there is a match. The full URL check won't be
+    // performed, but hash-based check will still be done. If any artificial
+    // matches are present, consider the allowlist as ready.
+    return true;
   }
 
   std::unique_ptr<PendingCheck> check = std::make_unique<PendingCheck>(
-      client, ClientCallbackType::CHECK_HIGH_CONFIDENCE_ALLOWLIST,
-      stores_to_check, std::vector<GURL>(1, url));
+      nullptr, ClientCallbackType::CHECK_OTHER, stores_to_check,
+      std::vector<GURL>(1, url));
 
-  return HandleAllowlistCheck(std::move(check));
+  AsyncMatch result =
+      HandleAllowlistCheck(std::move(check), /*allow_async_check=*/false);
+  DCHECK_NE(AsyncMatch::ASYNC, result);
+  return result == AsyncMatch::MATCH;
 }
 
 bool V4LocalDatabaseManager::CheckUrlForAccuracyTips(const GURL& url,
@@ -595,7 +598,7 @@ AsyncMatch V4LocalDatabaseManager::CheckCsdAllowlistUrl(const GURL& url,
       client, ClientCallbackType::CHECK_CSD_ALLOWLIST, stores_to_check,
       std::vector<GURL>(1, url));
 
-  return HandleAllowlistCheck(std::move(check));
+  return HandleAllowlistCheck(std::move(check), /*allow_async_check=*/true);
 }
 
 bool V4LocalDatabaseManager::MatchDownloadAllowlistUrl(const GURL& url) {
@@ -828,7 +831,8 @@ SBThreatType V4LocalDatabaseManager::GetSBThreatTypeForList(
 }
 
 AsyncMatch V4LocalDatabaseManager::HandleAllowlistCheck(
-    std::unique_ptr<PendingCheck> check) {
+    std::unique_ptr<PendingCheck> check,
+    bool allow_async_check) {
   // We don't bother queuing allowlist checks since the DB will
   // normally be available already -- allowlists are used after page load,
   // and navigations are blocked until the DB is ready and dequeues checks.
@@ -854,6 +858,9 @@ AsyncMatch V4LocalDatabaseManager::HandleAllowlistCheck(
     }
   }
 
+  if (!allow_async_check) {
+    return AsyncMatch::NO_MATCH;
+  }
   ScheduleFullHashCheck(std::move(check));
   return AsyncMatch::ASYNC;
 }
@@ -1053,16 +1060,6 @@ void V4LocalDatabaseManager::RespondToClientWithoutPendingCheckCleanup(
                                               check->most_severe_threat_type);
       break;
 
-    case ClientCallbackType::CHECK_HIGH_CONFIDENCE_ALLOWLIST: {
-      DCHECK_EQ(1u, check->urls.size());
-      bool did_match_allowlist = check->most_severe_threat_type ==
-                                 SB_THREAT_TYPE_HIGH_CONFIDENCE_ALLOWLIST;
-      DCHECK(did_match_allowlist ||
-             check->most_severe_threat_type == SB_THREAT_TYPE_SAFE);
-      check->client->OnCheckUrlForHighConfidenceAllowlist(did_match_allowlist);
-      break;
-    }
-
     case ClientCallbackType::CHECK_RESOURCE_URL:
       DCHECK_EQ(1u, check->urls.size());
       check->client->OnCheckResourceUrlResult(check->urls[0],

components/safe_browsing/core/browser/db/v4_local_database_manager.h

@@ -77,8 +77,7 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
   bool CheckExtensionIDs(const std::set<FullHash>& extension_ids,
                          Client* client) override;
   bool CheckResourceUrl(const GURL& url, Client* client) override;
-  AsyncMatch CheckUrlForHighConfidenceAllowlist(const GURL& url,
-                                                Client* client) override;
+  bool CheckUrlForHighConfidenceAllowlist(const GURL& url) override;
   bool CheckUrlForAccuracyTips(const GURL& url, Client* client) override;
   bool CheckUrlForSubresourceFilter(const GURL& url, Client* client) override;
   bool MatchDownloadAllowlistUrl(const GURL& url) override;
@@ -140,9 +139,6 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
     // part of the CSD allowlist.
     CHECK_CSD_ALLOWLIST,
 
-    // TODO(vakh): Explain this.
-    CHECK_HIGH_CONFIDENCE_ALLOWLIST,
-
     // Checks whether the URL should shown an accuracy tip.
     CHECK_ACCURACY_TIPS,
 
@@ -281,8 +277,12 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
   bool HandleCheck(std::unique_ptr<PendingCheck> check);
 
   // Like HandleCheck, but for allowlists that have both full-hashes and
-  // partial hashes in the DB. Returns MATCH, NO_MATCH, or ASYNC.
-  AsyncMatch HandleAllowlistCheck(std::unique_ptr<PendingCheck> check);
+  // partial hashes in the DB. If |allow_async_check| is false, it will only
+  // return either MATCH or NO_MATCH. If |allow_async_check| is true, it returns
+  // MATCH, NO_MATCH, or ASYNC. In the ASYNC case, it will schedule performing
+  // the full hash check.
+  AsyncMatch HandleAllowlistCheck(std::unique_ptr<PendingCheck> check,
+                                  bool allow_async_check);
 
   // Computes the hashes of URLs that have artificially been marked as unsafe
   // using any of the following command line flags: "mark_as_phishing",