Fix V4LocalDatabaseManager::CheckUrlForHighConfidenceAllowlist in asy…

…nc mode on shutdown.

The callback was not being run if SafeBrowsingService::Stop was called, and a NOTREACHED was being hit.

(cherry picked from commit 7263afa)

Bug: 1449786
Change-Id: Ibc4fde9579c5e0ff5ee100551a724c2bbd474b0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4574999
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Auto-Submit: John Abd-El-Malek <jam@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1151291}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4579421
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/branch-heads/5790@{#229}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}

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

@@ -33,6 +33,7 @@
 #include "components/safe_browsing/core/browser/db/v4_protocol_manager_util.h"
 #include "components/safe_browsing/core/common/features.h"
 #include "crypto/sha2.h"
+#include "mojo/public/cpp/bindings/callback_helpers.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace safe_browsing {
@@ -822,6 +823,15 @@ AsyncMatch V4LocalDatabaseManager::HandleAllowlistCheck(
   PendingCheck* check_ptr = check.get();
   AsyncMatch match;
 
+  if (GetPrefixMatchesIsAsync() && !callback.is_null()) {
+    // If StopOnSBThread is called weak_factory_ will get invalidated and
+    // HandleAllowlistCheckContinuation won't be called. We still want to run
+    // the callback though. See comment in CheckUrlForHighConfidenceAllowlist
+    // on why this returns true.
+    callback =
+        mojo::WrapCallbackWithDefaultInvokeIfNotRun(std::move(callback), true);
+  }
+
   GetPrefixMatches(
       check_ptr,
       base::BindOnce(&V4LocalDatabaseManager::HandleAllowlistCheckContinuation,
@@ -1150,6 +1160,12 @@ void V4LocalDatabaseManager::RespondSafeToQueuedAndPendingChecks() {
   // possibility of concurrent modifications while iterating.
   PendingChecks pending_checks = CopyAndRemoveAllPendingChecks();
   for (PendingCheck* it : pending_checks) {
+    if (it->client_callback_type == ClientCallbackType::CHECK_OTHER &&
+        GetPrefixMatchesIsAsync()) {
+      // In this case there's a callback that will run when weak_factory_ is
+      // invalidated.
+      continue;
+    }
     // We don't own the unique pointer for the pending check, so we do not
     // perform cleanup on it while responding to the client.
     RespondToClientWithoutPendingCheckCleanup(it);

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

@@ -19,6 +19,7 @@
 #include "base/task/sequenced_task_runner.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_command_line.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "base/test/test_future.h"
 #include "base/test/test_simple_task_runner.h"
@@ -876,6 +877,32 @@ TEST_F(V4LocalDatabaseManagerTest, TestCheckUrlForHCAllowlistUnavailable) {
       v4_local_database_manager_));
 }
 
+TEST_F(V4LocalDatabaseManagerTest, TestCheckUrlForHCAllowlistAfterStopping) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeatureWithParameters(
+      kMmapSafeBrowsingDatabase, {{"MmapSafeBrowsingDatabaseAsync", "true"}});
+
+  SetupFakeManager();
+  std::string url_safe_no_scheme("example.com/safe/");
+  FullHashStr safe_full_hash(crypto::SHA256HashString(url_safe_no_scheme));
+
+  // Setup to match full hash in the local database.
+  StoreAndHashPrefixes store_and_hash_prefixes;
+  store_and_hash_prefixes.emplace_back(GetUrlHighConfidenceAllowlistId(),
+                                       safe_full_hash);
+  ReplaceV4Database(store_and_hash_prefixes, /* stores_available= */ true);
+
+  const GURL url_check("https://" + url_safe_no_scheme);
+  base::test::TestFuture<bool> future;
+  v4_local_database_manager_->CheckUrlForHighConfidenceAllowlist(
+      url_check, "HPRT", future.GetCallback());
+  EXPECT_EQ(1ul, GetPendingChecks().size());
+  StopLocalDatabaseManager();
+  EXPECT_TRUE(GetPendingChecks().empty());
+
+  EXPECT_TRUE(future.Get());
+}
+
 // When allowlist is available but the size is too small, all URLs should be
 // considered as matches.
 TEST_F(V4LocalDatabaseManagerTest, TestCheckUrlForHCAllowlistSmallSize) {