[SHv2] Run PasswordStatusCheckService on browser startup

This CL makes PasswordStatusCheckService be created at browser startup.
There were couple of different issues:

1) To handle ScopedObserver destruction, OnBulkCheckServiceShutDown is
added to both PasswordCheckDelegate and PasswordStatusCheckService.
As A result, whenever bulk leak service shut down, both stop observing
it.

2) Replaced is_update_credential_count_pending_ with
running_update_credential_count_ to control killing infra based on
the number of running checks.

3) Marked ServiceIsNULLWhileTesting is true to adapt the behavior of
AccountPasswordStore. AccountPasswordStoreFactory class also set
ServiceIsNULLWhileTesting to true.

4) Updated SafetyHubHandlerTest by creating a testing factory for
PasswordStatusCheckService.

5) Added RunUntilIdle to SingleClientNigoriWithWebApiTests to provide
enough time to PasswordStatusCheckService to shutdown infra. Otherwise,
there were a few dangling pointers in BulkLeakCheckServiceAdapter.

6) Handled profile_store being empty case in PasswordStatusCheckService
to prevent crashes when password store is not enabled.

Bug: 1443466
Change-Id: Ie2ef3397163118b96b7e86a97c2991f379bf3ee1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4926468
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Commit-Queue: Side YILMAZ <sideyilmaz@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1216875}

chrome/browser/extensions/api/passwords_private/password_check_delegate.cc

@@ -401,6 +401,13 @@ PasswordCheckDelegate::GetInsecureCredentialsManager() {
   return &insecure_credentials_manager_;
 }
 
+void PasswordCheckDelegate::OnBulkCheckServiceShutDown() {
+  // Stop observing BulkLeakCheckService when the service shut down.
+  CHECK(observed_bulk_leak_check_service_.IsObservingSource(
+      BulkLeakCheckServiceFactory::GetForProfile(profile_)));
+  observed_bulk_leak_check_service_.Reset();
+}
+
 void PasswordCheckDelegate::OnSavedPasswordsChanged(
     const password_manager::PasswordStoreChangeList& changes) {
   // Getting the first notification about a change in saved passwords implies

chrome/browser/extensions/api/passwords_private/password_check_delegate.h

@@ -106,6 +106,7 @@ class PasswordCheckDelegate
       password_manager::BulkLeakCheckService::State state) override;
   void OnCredentialDone(const password_manager::LeakCheckCredential& credential,
                         password_manager::IsLeaked is_leaked) override;
+  void OnBulkCheckServiceShutDown() override;
 
   // Starts the analyses of whether credentials are compromised and/or weak.
   // Assumes that `StartPasswordCheck()` was called prior.

chrome/browser/sync/test/integration/single_client_custom_passphrase_sync_test.cc

@@ -294,6 +294,9 @@ IN_PROC_BROWSER_TEST_F(SingleClientCustomPassphraseSyncTest,
   EXPECT_TRUE(
       PasswordFormsChecker(/*index=*/0, /*expected_forms=*/{password_form})
           .Wait());
+  // TODO(crbug.com/1443466): Clean up after solving dangling ptrs when
+  // PasswordStatusCheckService::MaybeResetInfrastructureAsync is called.
+  base::RunLoop().RunUntilIdle();
 }
 #endif  // !BUILDFLAG(IS_ANDROID)
 

chrome/browser/sync/test/integration/single_client_incoming_password_sharing_invitation_test.cc

@@ -444,6 +444,9 @@ IN_PROC_BROWSER_TEST_F(SingleClientIncomingPasswordSharingInvitationTest,
                               base::UTF8ToUTF16(std::string(kPasswordValue))),
                         Field(&PasswordForm::type,
                               PasswordForm::Type::kReceivedViaSharing)))));
+  // TODO(crbug.com/1443466): Clean up after solving dangling ptrs when
+  // PasswordStatusCheckService::MaybeResetInfrastructureAsync is called.
+  base::RunLoop().RunUntilIdle();
 }
 
 // This test verifies that Incoming Password Sharing Invitation data type is

chrome/browser/sync/test/integration/single_client_nigori_sync_test.cc

@@ -409,6 +409,9 @@ IN_PROC_BROWSER_TEST_F(SingleClientNigoriSyncTest,
   EXPECT_TRUE(GetSyncService(0)->GetUserSettings()->SetDecryptionPassphrase(
       kKeyParams.password));
   EXPECT_TRUE(WaitForPasswordForms({password_form}));
+  // TODO(crbug.com/1443466): Clean up after solving dangling ptrs when
+  // PasswordStatusCheckService::MaybeResetInfrastructureAsync is called.
+  base::RunLoop().RunUntilIdle();
 }
 
 // Tests that client can decrypt passwords, encrypted with keystore key in case
@@ -1532,6 +1535,9 @@ IN_PROC_BROWSER_TEST_F(
       kTrustedVaultKeyParams.derivation_params, GetFakeServer());
 
   EXPECT_TRUE(PasswordFormsChecker(0, {password_form1, password_form2}).Wait());
+  // TODO(crbug.com/1443466): Clean up after solving dangling ptrs when
+  // PasswordStatusCheckService::MaybeResetInfrastructureAsync is called.
+  base::RunLoop().RunUntilIdle();
 }
 
 IN_PROC_BROWSER_TEST_F(
@@ -1595,6 +1601,9 @@ IN_PROC_BROWSER_TEST_F(
       kTrustedVaultKeyParams.derivation_params, GetFakeServer());
 
   EXPECT_TRUE(PasswordFormsChecker(0, {password_form1, password_form2}).Wait());
+  // TODO(crbug.com/1443466): Clean up after solving dangling ptrs when
+  // PasswordStatusCheckService::MaybeResetInfrastructureAsync is called.
+  base::RunLoop().RunUntilIdle();
 }
 
 IN_PROC_BROWSER_TEST_F(
@@ -2004,6 +2013,9 @@ IN_PROC_BROWSER_TEST_F(SingleClientNigoriWithWebApiTest,
       "Sync.TrustedVaultURLFetchResponse.DownloadKeys",
       /*sample=*/200,
       /*expected_bucket_count=*/1);
+  // TODO(crbug.com/1443466): Clean up after solving dangling ptrs when
+  // PasswordStatusCheckService::MaybeResetInfrastructureAsync is called.
+  base::RunLoop().RunUntilIdle();
 }
 
 // Regression test for crbug.com/1267391: after following key rotation the
@@ -2130,6 +2142,9 @@ IN_PROC_BROWSER_TEST_F(SingleClientNigoriWithWebApiTest,
   // forms now.
   EXPECT_TRUE(PasswordFormsChecker(0, {password_form1, password_form2}).Wait());
   EXPECT_FALSE(GetSecurityDomainsServer()->ReceivedInvalidRequest());
+  // TODO(crbug.com/1443466): Clean up after solving dangling ptrs when
+  // PasswordStatusCheckService::MaybeResetInfrastructureAsync is called.
+  base::RunLoop().RunUntilIdle();
 }
 
 // ChromeOS doesn't have unconsented primary accounts.

chrome/browser/ui/safety_hub/password_status_check_service.cc

@@ -182,7 +182,9 @@ PasswordStatusCheckService::PasswordStatusCheckService(Profile* profile)
       AccountPasswordStoreFactory::GetForProfile(
           profile_, ServiceAccessType::IMPLICIT_ACCESS);
 
-  profile_password_store_observation_.Observe(profile_store.get());
+  if (profile_store) {
+    profile_password_store_observation_.Observe(profile_store.get());
+  }
   if (account_store) {
     account_password_store_observation_.Observe(account_store.get());
   }
@@ -231,39 +233,28 @@ void PasswordStatusCheckService::StartRepeatedUpdates() {
 
 void PasswordStatusCheckService::UpdateInsecureCredentialCountAsync() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  if (is_update_credential_count_pending_) {
+  // In case there is a running check, do not start checks again.
+  if (running_update_credential_count_ > 0) {
     return;
   }
 
-  is_update_credential_count_pending_ = true;
-
+  // Initializing the infra will cause the presenter will check for all saved
+  // passwords. running_update_credential_count_ will be increased in
+  // OnSavedPasswordsChanged where the weak and reused checks will be
+  // initialized.
   InitializePasswordCheckInfrastructure();
-
-  CHECK(saved_passwords_presenter_);
-  if (!saved_passwords_presenter_observation_.IsObserving()) {
-    saved_passwords_presenter_observation_.Observe(
-        saved_passwords_presenter_.get());
-  }
 }
 
 void PasswordStatusCheckService::RunPasswordCheckAsync() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
+  // In case there is a running check, do not start checks again.
   if (is_password_check_running_) {
     return;
   }
 
-  is_password_check_running_ = true;
-
   InitializePasswordCheckInfrastructure();
 
-  CHECK(password_check_delegate_);
-  if (!bulk_leak_check_observation_.IsObserving()) {
-    bulk_leak_check_observation_.Observe(
-        BulkLeakCheckServiceFactory::GetForProfile(profile_));
-  }
-
+  is_password_check_running_ = true;
   password_check_delegate_->StartPasswordCheck();
   base::RecordAction(base::UserMetricsAction("SafetyHub_PasswordCheckRun"));
 }
@@ -273,8 +264,11 @@ void PasswordStatusCheckService::OnSavedPasswordsChanged(
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   CHECK(IsInfrastructureReady());
 
+  // Increase the counter for update credential count calls.
+  running_update_credential_count_++;
+
   no_passwords_saved_ =
-      saved_passwords_presenter_->GetSavedCredentials().size() == 0;
+      saved_passwords_presenter_->GetSavedCredentials().empty();
 
   base::RepeatingClosure on_done = base::BarrierClosure(
       /*num_closures=*/2,
@@ -291,7 +285,10 @@ void PasswordStatusCheckService::OnSavedPasswordsChanged(
 }
 
 void PasswordStatusCheckService::OnWeakAndReuseChecksDone() {
-  is_update_credential_count_pending_ = false;
+  // Decrease the counter as one check is completed.
+  CHECK_GT(running_update_credential_count_, 0);
+  running_update_credential_count_--;
+
   UpdateInsecureCredentialCount();
   MaybeResetInfrastructureAsync();
 }
@@ -314,6 +311,7 @@ void PasswordStatusCheckService::OnStateChanged(
         kHashingFailure:
     case password_manager::BulkLeakCheckServiceInterface::State::kNetworkError:
     case password_manager::BulkLeakCheckServiceInterface::State::kQuotaLimit:
+      // Reset password check flag since the check is completed.
       is_password_check_running_ = false;
 
       // Set time for next password check and schedule the next run.
@@ -361,19 +359,39 @@ void PasswordStatusCheckService::InitializePasswordCheckInfrastructure() {
     return;
   }
 
+  // ProfilePasswordStore may not exist for some cases like non-user profiles on
+  // Ash. The infra should not be initialized if ProfilePasswordStore does not
+  // exist as is required to initialize the infra successfully.
+  // TODO(crbug.com/1443466): Add CHECK for profile_store after making this
+  // service null if the store does not exist.
+  scoped_refptr<password_manager::PasswordStoreInterface> profile_store =
+      ProfilePasswordStoreFactory::GetForProfile(
+          profile_, ServiceAccessType::IMPLICIT_ACCESS);
+  if (!profile_store) {
+    return;
+  }
+
   credential_id_generator_ = std::make_unique<extensions::IdGenerator>();
   saved_passwords_presenter_ =
       std::make_unique<password_manager::SavedPasswordsPresenter>(
-          AffiliationServiceFactory::GetForProfile(profile_),
-          ProfilePasswordStoreFactory::GetForProfile(
-              profile_, ServiceAccessType::IMPLICIT_ACCESS),
+          AffiliationServiceFactory::GetForProfile(profile_), profile_store,
           AccountPasswordStoreFactory::GetForProfile(
               profile_, ServiceAccessType::IMPLICIT_ACCESS));
   saved_passwords_presenter_->Init();
   password_check_delegate_ =
       std::make_unique<extensions::PasswordCheckDelegate>(
           profile_, saved_passwords_presenter_.get(),
           credential_id_generator_.get());
+
+  // Observe new saved_passwords_presenter_.
+  saved_passwords_presenter_observation_.Reset();
+  saved_passwords_presenter_observation_.Observe(
+      saved_passwords_presenter_.get());
+
+  // Observe new BulkLeakCheckService.
+  bulk_leak_check_observation_.Reset();
+  bulk_leak_check_observation_.Observe(
+      BulkLeakCheckServiceFactory::GetForProfile(profile_));
 }
 
 void PasswordStatusCheckService::UpdateInsecureCredentialCount() {
@@ -406,22 +424,26 @@ void PasswordStatusCheckService::UpdateInsecureCredentialCount() {
 
 void PasswordStatusCheckService::MaybeResetInfrastructureAsync() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  if (!is_update_credential_count_pending_ && !is_password_check_running_) {
-    saved_passwords_presenter_observation_.Reset();
-    bulk_leak_check_observation_.Reset();
-
-    // The reset is done as a task rather than directly because when observers
-    // are notified that e.g. the password check is done, it may be too early to
-    // reset the infrastructure immediately. Synchronous operations may still be
-    // ongoing in `SavedPasswordsPresenter`.
-    base::SingleThreadTaskRunner::GetCurrentDefault()->DeleteSoon(
-        FROM_HERE, std::move(password_check_delegate_));
-    base::SingleThreadTaskRunner::GetCurrentDefault()->DeleteSoon(
-        FROM_HERE, std::move(saved_passwords_presenter_));
-    base::SingleThreadTaskRunner::GetCurrentDefault()->DeleteSoon(
-        FROM_HERE, std::move(credential_id_generator_));
+  // DCHECK to detect if the counters are decreased more than they are
+  // increased.
+  CHECK_GE(running_update_credential_count_, 0);
+  if (running_update_credential_count_ > 0 || is_password_check_running_) {
+    return;
   }
+
+  saved_passwords_presenter_observation_.Reset();
+  bulk_leak_check_observation_.Reset();
+
+  // The reset is done as a task rather than directly because when observers
+  // are notified that e.g. the password check is done, it may be too early to
+  // reset the infrastructure immediately. Synchronous operations may still be
+  // ongoing in `SavedPasswordsPresenter`.
+  base::SingleThreadTaskRunner::GetCurrentDefault()->DeleteSoon(
+      FROM_HERE, std::move(password_check_delegate_));
+  base::SingleThreadTaskRunner::GetCurrentDefault()->DeleteSoon(
+      FROM_HERE, std::move(saved_passwords_presenter_));
+  base::SingleThreadTaskRunner::GetCurrentDefault()->DeleteSoon(
+      FROM_HERE, std::move(credential_id_generator_));
 }
 
 bool PasswordStatusCheckService::IsInfrastructureReady() const {
@@ -511,3 +533,10 @@ const PasswordStatusCheckResult& PasswordStatusCheckService::GetCachedResult()
     const {
   return *latest_result_;
 }
+
+void PasswordStatusCheckService::OnBulkCheckServiceShutDown() {
+  // Stop observing BulkLeakCheckService when the service shut down.
+  CHECK(bulk_leak_check_observation_.IsObservingSource(
+      BulkLeakCheckServiceFactory::GetForProfile(profile_)));
+  bulk_leak_check_observation_.Reset();
+}

chrome/browser/ui/safety_hub/password_status_check_service.h

@@ -44,7 +44,7 @@ class PasswordStatusCheckService
   size_t reused_credential_count() const { return reused_credential_count_; }
 
   bool is_update_credential_count_pending() const {
-    return is_update_credential_count_pending_;
+    return running_update_credential_count_ > 0;
   }
 
   bool is_password_check_running() const { return is_password_check_running_; }
@@ -111,6 +111,7 @@ class PasswordStatusCheckService
       password_manager::BulkLeakCheckService::State state) override;
   void OnCredentialDone(const password_manager::LeakCheckCredential& credential,
                         password_manager::IsLeaked is_leaked) override;
+  void OnBulkCheckServiceShutDown() override;
 
   // PasswordStoreInterface::Observer implementation.
   // Used to trigger an update of the password issue counts when passwords
@@ -194,9 +195,14 @@ class PasswordStatusCheckService
   // True when password stores are empty and there are no saved passwords.
   bool no_passwords_saved_ = true;
 
-  // Flags to indicate which async operations are currently ongoing. Memory
-  // intensive objects will be reset after all have finished.
-  bool is_update_credential_count_pending_ = false;
+  // Counter of running sync update credential count checks. Memory intensive
+  // objects will be reset after all have finished and
+  // is_password_check_running_ is false.
+  int running_update_credential_count_ = 0;
+
+  // Flag to indicate if password check is running. Memory intensive objects
+  // will be reset after the check is completed and
+  // running_update_credential_count_ is zero.
   bool is_password_check_running_ = false;
 
   // Timer to schedule the run of the password check after some time has passed.

chrome/browser/ui/safety_hub/password_status_check_service_factory.cc

@@ -55,3 +55,12 @@ PasswordStatusCheckServiceFactory::BuildServiceInstanceForBrowserContext(
   return std::make_unique<PasswordStatusCheckService>(
       Profile::FromBrowserContext(context));
 }
+
+bool PasswordStatusCheckServiceFactory::ServiceIsCreatedWithBrowserContext()
+    const {
+  return base::FeatureList::IsEnabled(features::kSafetyHub);
+}
+
+bool PasswordStatusCheckServiceFactory::ServiceIsNULLWhileTesting() const {
+  return true;
+}

chrome/browser/ui/safety_hub/password_status_check_service_factory.h

@@ -34,8 +34,12 @@ class PasswordStatusCheckServiceFactory : public ProfileKeyedServiceFactory {
   ~PasswordStatusCheckServiceFactory() override;
 
   // BrowserContextKeyedServiceFactory:
+  bool ServiceIsCreatedWithBrowserContext() const override;
+
   std::unique_ptr<KeyedService> BuildServiceInstanceForBrowserContext(
       content::BrowserContext* context) const override;
+
+  bool ServiceIsNULLWhileTesting() const override;
 };
 
 #endif  // CHROME_BROWSER_UI_SAFETY_HUB_PASSWORD_STATUS_CHECK_SERVICE_FACTORY_H_

chrome/browser/ui/webui/settings/safety_hub_handler_unittest.cc

@@ -5,6 +5,7 @@
 #include <ctime>
 #include <memory>
 
+#include "base/functional/bind.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/gtest_util.h"
@@ -59,6 +60,17 @@ constexpr char16_t kCompromisedPassword[] = u"fnlsr4@cm^mdls@fkspnsg3d";
 constexpr ContentSettingsType kUnusedPermission =
     ContentSettingsType::GEOLOCATION;
 
+namespace {
+
+// Creates a new PassworsStatusCheckService for the given `context`.
+std::unique_ptr<KeyedService> BuildPasswordService(
+    content::BrowserContext* context) {
+  return std::make_unique<PasswordStatusCheckService>(
+      Profile::FromBrowserContext(context));
+}
+
+}  // namespace
+
 class SafetyHubHandlerTest : public testing::Test {
  public:
   SafetyHubHandlerTest() {
@@ -103,6 +115,10 @@ class SafetyHubHandlerTest : public testing::Test {
     EXPECT_EQ(GURL(kUnusedTestSite),
               GURL(*revoked_permissions[0].GetDict().FindString(
                   site_settings::kOrigin)));
+
+    // Create password status check service.
+    PasswordStatusCheckServiceFactory::GetInstance()->SetTestingFactory(
+        profile(), base::BindRepeating(&BuildPasswordService));
   }
 
   void TearDown() override {