Update PasswordFormManagers when account storage becomes (un)available

Bug: 1004777
Change-Id: I6d0afbdac381e3d1b02535a98205e740860af89d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1865171Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707351}

chrome/browser/password_manager/account_storage/account_password_store_factory.cc

@@ -28,8 +28,50 @@
 #include "content/public/browser/network_service_instance.h"
 #include "content/public/browser/storage_partition.h"
 
+#if !defined(OS_ANDROID)
+#include "base/task/post_task.h"
+#include "base/task/task_traits.h"
+#include "chrome/browser/password_manager/chrome_password_manager_client.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "content/public/browser/browser_task_traits.h"
+#endif  // !defined(OS_ANDROID)
+
 using password_manager::PasswordStore;
 
+#if !defined(OS_ANDROID)
+
+namespace {
+
+void UpdateAllFormManagers(Profile* profile) {
+  for (Browser* browser : *BrowserList::GetInstance()) {
+    if (browser->profile() != profile)
+      continue;
+    TabStripModel* tabs = browser->tab_strip_model();
+    for (int index = 0; index < tabs->count(); index++) {
+      content::WebContents* web_contents = tabs->GetWebContentsAt(index);
+      ChromePasswordManagerClient* client =
+          ChromePasswordManagerClient::FromWebContents(web_contents);
+      if (client)
+        client->UpdateFormManagers();
+    }
+  }
+}
+
+}  // namespace
+
+#endif  // !defined(OS_ANDROID)
+
+void SyncEnabledOrDisabled(Profile* profile) {
+#if defined(OS_ANDROID)
+  NOTREACHED();
+#else
+  base::PostTask(FROM_HERE, {content::BrowserThread::UI},
+                 base::BindOnce(&UpdateAllFormManagers, profile));
+#endif  // defined(OS_ANDROID)
+}
+
 // static
 scoped_refptr<PasswordStore> AccountPasswordStoreFactory::GetForProfile(
     Profile* profile,
@@ -77,7 +119,8 @@ AccountPasswordStoreFactory::BuildServiceInstanceFor(
 
   scoped_refptr<PasswordStore> ps =
       new password_manager::PasswordStoreDefault(std::move(login_db));
-  if (!ps->Init(/*flare=*/base::DoNothing(), profile->GetPrefs())) {
+  if (!ps->Init(/*flare=*/base::DoNothing(), profile->GetPrefs(),
+                base::BindRepeating(&SyncEnabledOrDisabled, profile))) {
     // TODO(crbug.com/479725): Remove the LOG once this error is visible in the
     // UI.
     LOG(WARNING) << "Could not initialize password store.";

components/password_manager/core/browser/password_store.cc

@@ -124,11 +124,13 @@ PasswordStore::PasswordStore()
       init_status_(InitStatus::kUnknown) {}
 
 bool PasswordStore::Init(const syncer::SyncableService::StartSyncFlare& flare,
-                         PrefService* prefs) {
+                         PrefService* prefs,
+                         base::RepeatingClosure sync_enabled_or_disabled_cb) {
   main_task_runner_ = base::SequencedTaskRunnerHandle::Get();
   DCHECK(main_task_runner_);
   background_task_runner_ = CreateBackgroundTaskRunner();
   DCHECK(background_task_runner_);
+  sync_enabled_or_disabled_cb_ = std::move(sync_enabled_or_disabled_cb);
 #if defined(SYNC_PASSWORD_REUSE_DETECTION_ENABLED)
   prefs_ = prefs;
   hash_password_manager_.set_prefs(prefs);
@@ -553,7 +555,7 @@ bool PasswordStore::InitOnBackgroundSequence(
     sync_bridge_.reset(new PasswordSyncBridge(
         std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(
             syncer::PASSWORDS, base::DoNothing()),
-        /*password_store_sync=*/this));
+        /*password_store_sync=*/this, sync_enabled_or_disabled_cb_));
   } else {
     DCHECK(!syncable_service_);
     syncable_service_.reset(new PasswordSyncableService(this));

components/password_manager/core/browser/password_store.h

@@ -112,10 +112,11 @@ class PasswordStore : protected PasswordStoreSync,
 
   PasswordStore();
 
-  // Reimplement this to add custom initialization. Always call this too on the
-  // UI thread.
-  virtual bool Init(const syncer::SyncableService::StartSyncFlare& flare,
-                    PrefService* prefs);
+  // Always call this too on the UI thread.
+  bool Init(
+      const syncer::SyncableService::StartSyncFlare& flare,
+      PrefService* prefs,
+      base::RepeatingClosure sync_enabled_or_disabled_cb = base::DoNothing());
 
   // RefcountedKeyedService:
   void ShutdownOnUIThread() override;
@@ -716,6 +717,8 @@ class PasswordStore : protected PasswordStoreSync,
   std::unique_ptr<PasswordSyncableService> syncable_service_;
   std::unique_ptr<PasswordSyncBridge> sync_bridge_;
 
+  base::RepeatingClosure sync_enabled_or_disabled_cb_;
+
   std::unique_ptr<AffiliatedMatchHelper> affiliated_match_helper_;
 
 #if defined(SYNC_PASSWORD_REUSE_DETECTION_ENABLED)

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

@@ -7,6 +7,7 @@
 #include <unordered_set>
 
 #include "base/auto_reset.h"
+#include "base/callback.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/strings/string_number_conversions.h"
@@ -209,10 +210,13 @@ class ScopedStoreTransaction {
 
 PasswordSyncBridge::PasswordSyncBridge(
     std::unique_ptr<syncer::ModelTypeChangeProcessor> change_processor,
-    PasswordStoreSync* password_store_sync)
+    PasswordStoreSync* password_store_sync,
+    const base::RepeatingClosure& sync_enabled_or_disabled_cb)
     : ModelTypeSyncBridge(std::move(change_processor)),
-      password_store_sync_(password_store_sync) {
+      password_store_sync_(password_store_sync),
+      sync_enabled_or_disabled_cb_(sync_enabled_or_disabled_cb) {
   DCHECK(password_store_sync_);
+  DCHECK(sync_enabled_or_disabled_cb_);
   // The metadata store could be null if the login database initialization
   // fails.
   if (!password_store_sync_->GetMetadataStore()) {
@@ -286,6 +290,8 @@ base::Optional<syncer::ModelError> PasswordSyncBridge::MergeSyncData(
     base::UmaHistogramCounts10000(
         "Sync.DownloadedPasswordsCountWhenInitialMergeFails",
         entity_data.size());
+  } else {
+    sync_enabled_or_disabled_cb_.Run();
   }
   return error;
 }
@@ -768,6 +774,8 @@ void PasswordSyncBridge::ApplyStopSyncChanges(
       }
       password_store_sync_->DeleteAndRecreateDatabaseFile();
       password_store_sync_->NotifyLoginsChanged(password_store_changes);
+
+      sync_enabled_or_disabled_cb_.Run();
     }
   }
 }

components/password_manager/core/browser/sync/password_sync_bridge.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_SYNC_PASSWORD_SYNC_BRIDGE_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_SYNC_PASSWORD_SYNC_BRIDGE_H_
 
+#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/sequence_checker.h"
 #include "components/password_manager/core/browser/password_store_change.h"
@@ -33,7 +34,8 @@ class PasswordSyncBridge : public syncer::ModelTypeSyncBridge {
   // |password_store_sync| must not be null and must outlive this object.
   PasswordSyncBridge(
       std::unique_ptr<syncer::ModelTypeChangeProcessor> change_processor,
-      PasswordStoreSync* password_store_sync);
+      PasswordStoreSync* password_store_sync,
+      const base::RepeatingClosure& sync_enabled_or_disabled_cb);
   ~PasswordSyncBridge() override;
 
   // Notifies the bridge of changes to the password database. Callers are
@@ -76,6 +78,8 @@ class PasswordSyncBridge : public syncer::ModelTypeSyncBridge {
   // Password store responsible for persistence.
   PasswordStoreSync* const password_store_sync_;
 
+  base::RepeatingClosure sync_enabled_or_disabled_cb_;
+
   // True if processing remote sync changes is in progress. Used to ignore
   // password store changes notifications while processing remote sync changes.
   bool is_processing_remote_sync_changes_ = false;

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

@@ -11,6 +11,7 @@
 #include "base/bind.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind_test_util.h"
+#include "base/test/mock_callback.h"
 #include "components/password_manager/core/browser/password_store_sync.h"
 #include "components/sync/base/client_tag_hash.h"
 #include "components/sync/model/data_batch.h"
@@ -233,8 +234,8 @@ class PasswordSyncBridgeTest : public testing::Test {
         .WillByDefault(Invoke(&fake_db_, &FakeDatabase::RemoveLogin));
 
     bridge_ = std::make_unique<PasswordSyncBridge>(
-        mock_processor_.CreateForwardingProcessor(),
-        &mock_password_store_sync_);
+        mock_processor_.CreateForwardingProcessor(), &mock_password_store_sync_,
+        sync_enabled_or_disabled_cb_.Get());
 
     // It's the responsibility of the PasswordStoreSync to inform the bridge
     // about changes in the password store. The bridge notifies the
@@ -303,11 +304,16 @@ class PasswordSyncBridgeTest : public testing::Test {
     return &mock_password_store_sync_;
   }
 
+  base::MockRepeatingClosure* mock_sync_enabled_or_disabled_cb() {
+    return &sync_enabled_or_disabled_cb_;
+  }
+
  private:
   FakeDatabase fake_db_;
   testing::NiceMock<syncer::MockModelTypeChangeProcessor> mock_processor_;
   testing::NiceMock<MockSyncMetadataStore> mock_sync_metadata_store_sync_;
   testing::NiceMock<MockPasswordStoreSync> mock_password_store_sync_;
+  testing::NiceMock<base::MockRepeatingClosure> sync_enabled_or_disabled_cb_;
   std::unique_ptr<PasswordSyncBridge> bridge_;
 };
 
@@ -780,7 +786,7 @@ TEST_F(PasswordSyncBridgeTest,
                                     /*entities=*/testing::SizeIs(1))));
 
   PasswordSyncBridge bridge(mock_processor().CreateForwardingProcessor(),
-                            mock_password_store_sync());
+                            mock_password_store_sync(), base::DoNothing());
 }
 
 // Tests that in case ReadAllLogins() during initial merge returns encryption
@@ -807,4 +813,64 @@ TEST_F(PasswordSyncBridgeTest,
   bridge()->ApplyStopSyncChanges(bridge()->CreateMetadataChangeList());
 }
 
+TEST_F(PasswordSyncBridgeTest, ShouldNotifyOnSyncEnable) {
+  ON_CALL(*mock_password_store_sync(), IsAccountStore())
+      .WillByDefault(Return(true));
+
+  // New password data becoming available because sync was newly enabled should
+  // trigger the callback.
+  EXPECT_CALL(*mock_sync_enabled_or_disabled_cb(), Run());
+
+  syncer::EntityChangeList initial_entity_data;
+  initial_entity_data.push_back(syncer::EntityChange::CreateAdd(
+      /*storage_key=*/"",
+      SpecificsToEntity(CreateSpecificsWithSignonRealm(kSignonRealm1))));
+
+  base::Optional<syncer::ModelError> error = bridge()->MergeSyncData(
+      bridge()->CreateMetadataChangeList(), std::move(initial_entity_data));
+  ASSERT_FALSE(error);
+}
+
+TEST_F(PasswordSyncBridgeTest, ShouldNotNotifyOnSyncChange) {
+  ON_CALL(*mock_password_store_sync(), IsAccountStore())
+      .WillByDefault(Return(true));
+
+  // New password data becoming available due to an incoming sync change should
+  // *not* trigger the callback. This is mainly for performance reasons: In
+  // practice, this callback will cause all PasswordFormManagers to re-query
+  // from the password store, which can be expensive.
+  EXPECT_CALL(*mock_sync_enabled_or_disabled_cb(), Run()).Times(0);
+
+  syncer::EntityChangeList entity_changes;
+  entity_changes.push_back(syncer::EntityChange::CreateAdd(
+      /*storage_key=*/"",
+      SpecificsToEntity(CreateSpecificsWithSignonRealm(kSignonRealm1))));
+
+  base::Optional<syncer::ModelError> error = bridge()->ApplySyncChanges(
+      bridge()->CreateMetadataChangeList(), std::move(entity_changes));
+  ASSERT_FALSE(error);
+}
+
+TEST_F(PasswordSyncBridgeTest, ShouldNotifyOnSyncDisableIfAccountStore) {
+  ON_CALL(*mock_password_store_sync(), IsAccountStore())
+      .WillByDefault(Return(true));
+
+  // The account password store gets cleared when sync is disabled, so this
+  // should trigger the callback.
+  EXPECT_CALL(*mock_sync_enabled_or_disabled_cb(), Run());
+
+  bridge()->ApplyStopSyncChanges(bridge()->CreateMetadataChangeList());
+}
+
+TEST_F(PasswordSyncBridgeTest, ShouldNotNotifyOnSyncDisableIfProfileStore) {
+  ON_CALL(*mock_password_store_sync(), IsAccountStore())
+      .WillByDefault(Return(false));
+
+  // The profile password store does *not* get cleared when sync is disabled, so
+  // this should *not* trigger the callback.
+  EXPECT_CALL(*mock_sync_enabled_or_disabled_cb(), Run()).Times(0);
+
+  bridge()->ApplyStopSyncChanges(bridge()->CreateMetadataChangeList());
+}
+
 }  // namespace password_manager