Delete passwords account store on startup if the feature is disabled

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

chrome/browser/password_manager/account_storage/account_password_store_factory.cc

@@ -122,6 +122,13 @@ scoped_refptr<PasswordStore> AccountPasswordStoreFactory::GetForProfile(
     ServiceAccessType access_type) {
   if (!base::FeatureList::IsEnabled(
           password_manager::features::kEnablePasswordsAccountStorage)) {
+    if (!GetInstance()->account_store_deleted_.contains(profile)) {
+      // TODO(crbug.com/1108738): Remove this logic once
+      // kEnablePasswordsAccountStorage is launched.
+      GetInstance()->account_store_deleted_.insert(profile);
+      password_manager::DeleteLoginDatabaseForAccountStorageFiles(
+          profile->GetPath());
+    }
     return nullptr;
   }
   // |profile| gets always redirected to a non-Incognito profile below, so
@@ -147,7 +154,10 @@ AccountPasswordStoreFactory::AccountPasswordStoreFactory()
   DependsOn(WebDataServiceFactory::GetInstance());
 }
 
-AccountPasswordStoreFactory::~AccountPasswordStoreFactory() = default;
+AccountPasswordStoreFactory::~AccountPasswordStoreFactory() {
+  // All entries should have been removed in BrowserContextShutdown().
+  DCHECK(account_store_deleted_.empty());
+}
 
 scoped_refptr<RefcountedKeyedService>
 AccountPasswordStoreFactory::BuildServiceInstanceFor(
@@ -198,3 +208,9 @@ 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_storage/account_password_store_factory.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_PASSWORD_MANAGER_ACCOUNT_STORAGE_ACCOUNT_PASSWORD_STORE_FACTORY_H_
 #define CHROME_BROWSER_PASSWORD_MANAGER_ACCOUNT_STORAGE_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"
@@ -39,6 +40,14 @@ class AccountPasswordStoreFactory
   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);
 };

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

@@ -5,10 +5,12 @@
 #include <memory>
 #include <string>
 
+#include "base/files/file_path_watcher.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
 #include "chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h"
 #include "chrome/browser/password_manager/account_storage/account_password_store_factory.h"
@@ -25,6 +27,7 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/password_manager/core/browser/password_manager_features_util.h"
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
+#include "components/password_manager/core/browser/password_store_factory_util.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/sync/driver/profile_sync_service.h"
@@ -34,6 +37,7 @@
 #include "content/public/browser/browsing_data_remover.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browsing_data_remover_test_util.h"
+#include "content/public/test/test_launcher.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/http/http_status_code.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -62,6 +66,41 @@ MATCHER_P3(MatchesLoginAndRealm, username, password, signon_realm, "") {
 void GetNewTab(Browser* browser, content::WebContents** web_contents) {
   PasswordManagerBrowserTestBase::GetNewTab(browser, web_contents);
 }
+
+// Helper class that waits until a given path does not exist on the filesystem
+// anymore.
+class PathDeletionWaiter {
+ public:
+  explicit PathDeletionWaiter(const base::FilePath& path) : path_(path) {}
+
+  // Blocks until the path passed into the constructor does not exist anymore.
+  // Returns true if the path was deleted (or didn't exist in the first place),
+  // or false if some error occurred.
+  bool WaitForPathToBeDeleted() {
+    base::ScopedAllowBlockingForTesting allow_blocking;
+
+    if (!base::PathExists(path_)) {
+      return true;
+    }
+    watcher_.Watch(path_, /*recursive=*/true,
+                   base::BindRepeating(&PathDeletionWaiter::PathChanged,
+                                       base::Unretained(this)));
+    run_loop_.Run();
+    return !base::PathExists(path_);
+  }
+
+ private:
+  void PathChanged(const base::FilePath& path, bool error) {
+    if (error || !base::PathExists(path_)) {
+      run_loop_.Quit();
+    }
+  }
+
+  const base::FilePath path_;
+  base::FilePathWatcher watcher_;
+  base::RunLoop run_loop_;
+};
+
 #endif  // !defined(OS_CHROMEOS)
 
 // This test fixture is similar to SingleClientPasswordsSyncTest, but it also
@@ -714,6 +753,66 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerSyncTest, ClearAccountStoreOnStartup) {
               ElementsAre(MatchesLogin("localuser", "localpass")));
 }
 
+// A variant of PasswordManagerSyncTest where the account storage feature flag
+// is enabled in PRE_ tests, but not in regular tests. This allows testing the
+// behavior when the feature flag gets turned off.
+class PasswordManagerTurnAccountStorageOffSyncTest
+    : public PasswordManagerSyncTest {
+ public:
+  PasswordManagerTurnAccountStorageOffSyncTest() {
+    // kEnablePasswordsAccountStorage is enabled iff this is a PRE_ test.
+    if (content::IsPreTest()) {
+      feature_list_.InitAndEnableFeature(
+          password_manager::features::kEnablePasswordsAccountStorage);
+    } else {
+      feature_list_.InitAndDisableFeature(
+          password_manager::features::kEnablePasswordsAccountStorage);
+    }
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(PasswordManagerTurnAccountStorageOffSyncTest,
+                       PRE_DeletesAccountStoreWhenFeatureTurnedOff) {
+  ASSERT_TRUE(base::FeatureList::IsEnabled(
+      password_manager::features::kEnablePasswordsAccountStorage));
+
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+
+  // Set up the password account storage, and add a credential just so that it's
+  // not empty.
+  AddCredentialToFakeServer(CreateTestPasswordForm("user", "pass"));
+  SetupSyncTransportWithPasswordAccountStorage();
+  ASSERT_THAT(GetAllLoginsFromAccountPasswordStore(),
+              ElementsAre(MatchesLogin("user", "pass")));
+
+  // Make sure that the account store actually exists on disk at the expected
+  // path.
+  {
+    base::ScopedAllowBlockingForTesting allow_blocking;
+    ASSERT_TRUE(base::PathExists(
+        password_manager::GetLoginDatabaseForAccountStoragePathForTesting(
+            GetProfile(0)->GetPath())));
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(PasswordManagerTurnAccountStorageOffSyncTest,
+                       DeletesAccountStoreWhenFeatureTurnedOff) {
+  ASSERT_FALSE(base::FeatureList::IsEnabled(
+      password_manager::features::kEnablePasswordsAccountStorage));
+
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+
+  // Since the feature flag is now disabled, the account-scoped store should get
+  // deleted soon after browser startup.
+  PathDeletionWaiter waiter(
+      password_manager::GetLoginDatabaseForAccountStoragePathForTesting(
+          GetProfile(0)->GetPath()));
+  EXPECT_TRUE(waiter.WaitForPathToBeDeleted());
+}
+
 #endif  // !defined(OS_CHROMEOS)
 
 }  // namespace

components/password_manager/core/browser/login_database.cc

@@ -1669,12 +1669,17 @@ bool LoginDatabase::IsEmpty() {
   return s.Step() && s.ColumnInt(0) == 0;
 }
 
+// static
+void LoginDatabase::DeleteDatabaseFile(const base::FilePath& db_path) {
+  sql::Database::Delete(db_path);
+}
+
 bool LoginDatabase::DeleteAndRecreateDatabaseFile() {
   TRACE_EVENT0("passwords", "LoginDatabase::DeleteAndRecreateDatabaseFile");
   DCHECK(db_.is_open());
   meta_table_.Reset();
   db_.Close();
-  sql::Database::Delete(db_path_);
+  DeleteDatabaseFile(db_path_);
   return Init();
 }
 

components/password_manager/core/browser/login_database.h

@@ -54,6 +54,12 @@ class LoginDatabase : public PasswordStoreSync::MetadataStore {
   LoginDatabase(const base::FilePath& db_path, IsAccountStore is_account_store);
   ~LoginDatabase() override;
 
+  // Deletes any database files for the given |db_path| from the disk. Must not
+  // be called while a LoginDatabase instance for this path exists!
+  // This does blocking I/O, so must only be called from a thread that allows
+  // this (in particular, *not* from the UI thread).
+  static void DeleteDatabaseFile(const base::FilePath& db_path);
+
   // Returns whether this is the profile-scoped or the account-scoped storage:
   // true:  Gaia-account-scoped store, which is used for signed-in but not
   //        syncing users.

components/password_manager/core/browser/password_store_factory_util.cc

@@ -102,4 +102,18 @@ std::unique_ptr<LoginDatabase> CreateLoginDatabaseForAccountStorage(
                                          IsAccountStore(true));
 }
 
+void DeleteLoginDatabaseForAccountStorageFiles(
+    const base::FilePath& profile_path) {
+  base::FilePath login_db_file_path =
+      profile_path.Append(kLoginDataForAccountFileName);
+  base::ThreadPool::PostTask(
+      FROM_HERE, {base::MayBlock()},
+      base::BindOnce(&LoginDatabase::DeleteDatabaseFile, login_db_file_path));
+}
+
+base::FilePath GetLoginDatabaseForAccountStoragePathForTesting(
+    const base::FilePath& profile_path) {
+  return profile_path.Append(kLoginDataForAccountFileName);
+}
+
 }  // namespace password_manager

components/password_manager/core/browser/password_store_factory_util.h

@@ -42,6 +42,14 @@ std::unique_ptr<LoginDatabase> CreateLoginDatabaseForProfileStorage(
 std::unique_ptr<LoginDatabase> CreateLoginDatabaseForAccountStorage(
     const base::FilePath& profile_path);
 
+// Deletes any files associated with the acccount-scoped LoginDatabase. Do *not*
+// call this while a LoginDatabase instance is using these files!
+void DeleteLoginDatabaseForAccountStorageFiles(
+    const base::FilePath& profile_path);
+
+base::FilePath GetLoginDatabaseForAccountStoragePathForTesting(
+    const base::FilePath& profile_path);
+
 }  // namespace password_manager
 
 #endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_STORE_FACTORY_UTIL_H_

components/password_manager/core/browser/sync/password_model_type_controller.cc

@@ -69,12 +69,10 @@ PasswordModelTypeController::PasswordModelTypeController(
               base::Unretained(this))) {
   identity_manager_->AddObserver(this);
 
-  if (!base::FeatureList::IsEnabled(features::kEnablePasswordsAccountStorage)) {
-    DCHECK(!account_password_store_for_cleanup);
-    // TODO(crbug.com/1079297): Delete the database file (if it exists); see
-    // password_store_factory_util.h.
-  } else {
-    DCHECK(account_password_store_for_cleanup);
+  DCHECK_EQ(
+      !!base::FeatureList::IsEnabled(features::kEnablePasswordsAccountStorage),
+      !!account_password_store_for_cleanup);
+  if (base::FeatureList::IsEnabled(features::kEnablePasswordsAccountStorage)) {
     // Note: Right now, we're still in the middle of SyncService initialization,
     // so we can't check IsOptedInForAccountStorage() yet (SyncService might not
     // have determined the syncing account yet). Post a task do to it after the