AccountPasswordStoreFactory: Cache in a pref whether the store exists

(as in, whether the actual database files exist on disk)

This lets us delete the store (if the corresponding feature flag got
disabled) only if it actually exists. This is an alternative (and maybe
better) implementation of crrev.com/c/2300439.

Bug: 1079297
Change-Id: I3ae84eb87b53e1e7b1e69a20afe68402a7095326
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2320278Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792166}

chrome/browser/password_manager/account_password_store_factory.cc

@@ -25,6 +25,8 @@
 #include "components/password_manager/core/browser/password_store_default.h"
 #include "components/password_manager/core/browser/password_store_factory_util.h"
 #include "components/password_manager/core/common/password_manager_features.h"
+#include "components/password_manager/core/common/password_manager_pref_names.h"
+#include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
@@ -122,15 +124,18 @@ scoped_refptr<PasswordStore> AccountPasswordStoreFactory::GetForProfile(
     ServiceAccessType access_type) {
   if (!base::FeatureList::IsEnabled(
           password_manager::features::kEnablePasswordsAccountStorage)) {
-    if (!GetInstance()->account_store_deleted_.contains(profile)) {
+    if (profile->GetPrefs()->GetBoolean(
+            password_manager::prefs::kAccountStorageExists)) {
       // TODO(crbug.com/1108738): Remove this logic once
       // kEnablePasswordsAccountStorage is launched.
-      GetInstance()->account_store_deleted_.insert(profile);
+      profile->GetPrefs()->ClearPref(
+          password_manager::prefs::kAccountStorageExists);
       password_manager::DeleteLoginDatabaseForAccountStorageFiles(
           profile->GetPath());
     }
     return nullptr;
   }
+
   // |profile| gets always redirected to a non-Incognito profile below, so
   // Incognito & IMPLICIT_ACCESS means that incognito browsing session would
   // result in traces in the normal profile without the user knowing it.
@@ -138,6 +143,11 @@ scoped_refptr<PasswordStore> AccountPasswordStoreFactory::GetForProfile(
       profile->IsOffTheRecord()) {
     return nullptr;
   }
+
+  // Either the store exists already, or it'll be created now.
+  profile->GetPrefs()->SetBoolean(
+      password_manager::prefs::kAccountStorageExists, true);
+
   return base::WrapRefCounted(static_cast<password_manager::PasswordStore*>(
       GetInstance()->GetServiceForBrowserContext(profile, true).get()));
 }
@@ -154,9 +164,12 @@ AccountPasswordStoreFactory::AccountPasswordStoreFactory()
   DependsOn(WebDataServiceFactory::GetInstance());
 }
 
-AccountPasswordStoreFactory::~AccountPasswordStoreFactory() {
-  // All entries should have been removed in BrowserContextShutdown().
-  DCHECK(account_store_deleted_.empty());
+AccountPasswordStoreFactory::~AccountPasswordStoreFactory() = default;
+
+void AccountPasswordStoreFactory::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterBooleanPref(password_manager::prefs::kAccountStorageExists,
+                                false);
 }
 
 scoped_refptr<RefcountedKeyedService>
@@ -208,9 +221,3 @@ content::BrowserContext* AccountPasswordStoreFactory::GetBrowserContextToUse(
 bool AccountPasswordStoreFactory::ServiceIsNULLWhileTesting() const {
   return true;
 }
-
-void AccountPasswordStoreFactory::BrowserContextShutdown(
-    content::BrowserContext* context) {
-  account_store_deleted_.erase(context);
-  RefcountedBrowserContextKeyedServiceFactory::BrowserContextShutdown(context);
-}

chrome/browser/password_manager/account_password_store_factory.h

@@ -5,7 +5,6 @@
 #ifndef CHROME_BROWSER_PASSWORD_MANAGER_ACCOUNT_PASSWORD_STORE_FACTORY_H_
 #define CHROME_BROWSER_PASSWORD_MANAGER_ACCOUNT_PASSWORD_STORE_FACTORY_H_
 
-#include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "base/memory/singleton.h"
 #include "components/keyed_service/content/refcounted_browser_context_keyed_service_factory.h"
@@ -35,19 +34,13 @@ class AccountPasswordStoreFactory
   ~AccountPasswordStoreFactory() override;
 
   // RefcountedBrowserContextKeyedServiceFactory:
+  void RegisterProfilePrefs(
+      user_prefs::PrefRegistrySyncable* registry) override;
   scoped_refptr<RefcountedKeyedService> BuildServiceInstanceFor(
       content::BrowserContext* context) const override;
   content::BrowserContext* GetBrowserContextToUse(
       content::BrowserContext* context) const override;
   bool ServiceIsNULLWhileTesting() const override;
-  void BrowserContextShutdown(content::BrowserContext* context) override;
-
-  // If the feature flag kEnablePasswordsAccountStorage is disabled, then this
-  // factory will always return nullptr, and any files related to the
-  // account-scoped store should be deleted from disk. This set keeps track of
-  // the contexts (aka profiles) for which the files were already deleted, so
-  // that the (potentially expensive) disk operations don't get run repeatedly.
-  base::flat_set<content::BrowserContext*> account_store_deleted_;
 
   DISALLOW_COPY_AND_ASSIGN(AccountPasswordStoreFactory);
 };

components/password_manager/core/common/password_manager_pref_names.cc

@@ -41,6 +41,8 @@ const char kSignInPasswordPromoRevive[] =
 const char kAccountStoragePerAccountSettings[] =
     "profile.password_account_storage_settings";
 
+const char kAccountStorageExists[] = "profile.password_account_storage_exists";
+
 const char kSyncPasswordHash[] = "profile.sync_password_hash";
 
 const char kSyncPasswordLengthAndHashSalt[] =

components/password_manager/core/common/password_manager_pref_names.h

@@ -68,6 +68,11 @@ extern const char kSignInPasswordPromoRevive[];
 // dictionary of key-value pairs.
 extern const char kAccountStoragePerAccountSettings[];
 
+// A boolean that tracks whether the account-scoped password store exists on
+// disk. When the factory needs to delete the store from disk, it uses this pref
+// to only trigger the deletion if the store actually exists.
+extern const char kAccountStorageExists[];
+
 // String that represents the sync password hash.
 extern const char kSyncPasswordHash[];