PasswordStore: Expose "unsynced deletions" to observers

An "unsynced deletion" is a password deletion that has been applied
locally, but not committed to the server yet.
Clear Browsing Data is interested in the existence of unsynced
deletions, so it can track when a deletion has actually been committed.

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

components/password_manager/core/browser/login_database.cc

@@ -1764,7 +1764,15 @@ std::unique_ptr<syncer::MetadataBatch> LoginDatabase::GetAllSyncMetadata() {
 
 void LoginDatabase::DeleteAllSyncMetadata() {
   TRACE_EVENT0("passwords", "LoginDatabase::DeleteAllSyncMetadata");
+  bool had_unsynced_deletions = HasUnsyncedDeletions();
   ClearAllSyncMetadata(&db_);
+  if (had_unsynced_deletions && deletions_have_synced_callback_) {
+    // Note: At this point we can't be fully sure whether the deletions actually
+    // reached the server yet. We might have sent a commit, but haven't received
+    // the commit confirmation. Let's be conservative and assume they haven't
+    // been successfully deleted.
+    deletions_have_synced_callback_.Run(/*success=*/false);
+  }
 }
 
 bool LoginDatabase::UpdateSyncMetadata(
@@ -1796,7 +1804,13 @@ bool LoginDatabase::UpdateSyncMetadata(
   s.BindInt(0, storage_key_int);
   s.BindString(1, encrypted_metadata);
 
-  return s.Run();
+  bool had_unsynced_deletions = HasUnsyncedDeletions();
+  bool result = s.Run();
+  if (result && had_unsynced_deletions && !HasUnsyncedDeletions() &&
+      deletions_have_synced_callback_) {
+    deletions_have_synced_callback_.Run(/*success=*/true);
+  }
+  return result;
 }
 
 bool LoginDatabase::ClearSyncMetadata(syncer::ModelType model_type,
@@ -1817,7 +1831,13 @@ bool LoginDatabase::ClearSyncMetadata(syncer::ModelType model_type,
                              "storage_key=?"));
   s.BindInt(0, storage_key_int);
 
-  return s.Run();
+  bool had_unsynced_deletions = HasUnsyncedDeletions();
+  bool result = s.Run();
+  if (result && had_unsynced_deletions && !HasUnsyncedDeletions() &&
+      deletions_have_synced_callback_) {
+    deletions_have_synced_callback_.Run(/*success=*/true);
+  }
+  return result;
 }
 
 bool LoginDatabase::UpdateModelTypeState(
@@ -1847,6 +1867,24 @@ bool LoginDatabase::ClearModelTypeState(syncer::ModelType model_type) {
   return s.Run();
 }
 
+void LoginDatabase::SetDeletionsHaveSyncedCallback(
+    base::RepeatingCallback<void(bool)> callback) {
+  deletions_have_synced_callback_ = std::move(callback);
+}
+
+bool LoginDatabase::HasUnsyncedDeletions() {
+  TRACE_EVENT0("passwords", "LoginDatabase::HasUnsyncedDeletions");
+
+  std::unique_ptr<syncer::MetadataBatch> batch = GetAllSyncEntityMetadata();
+  for (const auto& metadata_entry : batch->GetAllMetadata()) {
+    // Note: No need for an explicit "is unsynced" check: Once the deletion is
+    // committed, the metadata entry is removed.
+    if (metadata_entry.second->is_deleted())
+      return true;
+  }
+  return false;
+}
+
 bool LoginDatabase::BeginTransaction() {
   TRACE_EVENT0("passwords", "LoginDatabase::BeginTransaction");
   return db_.BeginTransaction();

components/password_manager/core/browser/login_database.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
@@ -187,6 +188,9 @@ class LoginDatabase : public PasswordStoreSync::MetadataStore {
       syncer::ModelType model_type,
       const sync_pb::ModelTypeState& model_type_state) override;
   bool ClearModelTypeState(syncer::ModelType model_type) override;
+  void SetDeletionsHaveSyncedCallback(
+      base::RepeatingCallback<void(bool)> callback) override;
+  bool HasUnsyncedDeletions() override;
 
   // Callers that requires transaction support should call these methods to
   // begin, rollback and commit transactions. They delegate to the transaction
@@ -362,6 +366,12 @@ class LoginDatabase : public PasswordStoreSync::MetadataStore {
   bool use_encryption_ = true;
 #endif  // defined(OS_POSIX)
 
+  // A callback to be invoked whenever all pending deletions have been processed
+  // by Sync - see
+  // PasswordStoreSync::MetadataStore::SetDeletionsHaveSyncedCallback for more
+  // details.
+  base::RepeatingCallback<void(bool)> deletions_have_synced_callback_;
+
   DISALLOW_COPY_AND_ASSIGN(LoginDatabase);
 };
 

components/password_manager/core/browser/password_store.cc

@@ -136,9 +136,7 @@ bool PasswordStore::FormDigest::operator==(const FormDigest& other) const {
 }
 
 PasswordStore::PasswordStore()
-    : observers_(new base::ObserverListThreadSafe<Observer>()),
-      shutdown_called_(false),
-      init_status_(InitStatus::kUnknown) {}
+    : observers_(new base::ObserverListThreadSafe<Observer>()) {}
 
 bool PasswordStore::Init(PrefService* prefs,
                          base::RepeatingClosure sync_enabled_or_disabled_cb) {
@@ -195,11 +193,13 @@ void PasswordStore::RemoveLoginsByURLAndTime(
     const base::RepeatingCallback<bool(const GURL&)>& url_filter,
     base::Time delete_begin,
     base::Time delete_end,
-    base::OnceClosure completion) {
+    base::OnceClosure completion,
+    base::OnceCallback<void(bool)> sync_completion) {
   DCHECK(main_task_runner_->RunsTasksInCurrentSequence());
   ScheduleTask(base::BindOnce(&PasswordStore::RemoveLoginsByURLAndTimeInternal,
                               this, url_filter, delete_begin, delete_end,
-                              std::move(completion)));
+                              std::move(completion),
+                              std::move(sync_completion)));
 }
 
 void PasswordStore::RemoveLoginsCreatedBetween(base::Time delete_begin,
@@ -745,6 +745,24 @@ void PasswordStore::NotifyLoginsChanged(
   }
 }
 
+void PasswordStore::NotifyDeletionsHaveSynced(bool success) {
+  DCHECK(background_task_runner_->RunsTasksInCurrentSequence());
+  // Either all deletions have been committed to the Sync server, or Sync is
+  // telling us that it won't commit them (because Sync was turned off
+  // permanently). In either case, run the corresponding callbacks now (on the
+  // main task runner).
+  DCHECK(!GetMetadataStore()->HasUnsyncedDeletions());
+  for (base::OnceCallback<void(bool)>& callback :
+       deletions_have_synced_callbacks_) {
+    main_task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce([](base::OnceCallback<void(bool)> callback,
+                          bool success) { std::move(callback).Run(success); },
+                       std::move(callback), success));
+  }
+  deletions_have_synced_callbacks_.clear();
+}
+
 void PasswordStore::InvokeAndNotifyAboutCompromisedPasswordsChange(
     base::OnceCallback<bool()> callback) {
   DCHECK(background_task_runner_->RunsTasksInCurrentSequence());
@@ -954,7 +972,8 @@ void PasswordStore::RemoveLoginsByURLAndTimeInternal(
     const base::RepeatingCallback<bool(const GURL&)>& url_filter,
     base::Time delete_begin,
     base::Time delete_end,
-    base::OnceClosure completion) {
+    base::OnceClosure completion,
+    base::OnceCallback<void(bool)> sync_completion) {
   DCHECK(background_task_runner_->RunsTasksInCurrentSequence());
   TRACE_EVENT0("passwords", "PasswordStore::RemoveLoginsByURLAndTimeInternal");
   BeginTransaction();
@@ -966,8 +985,18 @@ void PasswordStore::RemoveLoginsByURLAndTimeInternal(
   // sync codebase needs to update metadata atomically together with the login
   // data.
   CommitTransaction();
+
   if (completion)
     main_task_runner_->PostTask(FROM_HERE, std::move(completion));
+
+  if (sync_completion) {
+    deletions_have_synced_callbacks_.push_back(std::move(sync_completion));
+
+    // Do an immediate check for the case where there are already no unsynced
+    // deletions.
+    if (!GetMetadataStore()->HasUnsyncedDeletions())
+      NotifyDeletionsHaveSynced(/*success=*/true);
+  }
 }
 
 void PasswordStore::RemoveLoginsCreatedBetweenInternal(

components/password_manager/core/browser/password_store.h

@@ -172,15 +172,19 @@ class PasswordStore : protected PasswordStoreSync,
   virtual void RemoveLogin(const autofill::PasswordForm& form);
 
   // Remove all logins whose origins match the given filter and that were
-  // created
-  // in the given date range. |completion| will be posted to the
-  // |main_task_runner_| after deletions have been completed and notification
-  // have been sent out.
+  // created in the given date range. |completion| will be posted to the
+  // |main_task_runner_| after deletions have been completed and notifications
+  // have been sent out. |sync_completion| will be posted to
+  // |main_task_runner_| once the deletions have also been propagated to the
+  // server (or, in rare cases, if the user permanently disables Sync). This is
+  // only relevant for Sync users and for account store users - for other users,
+  // |sync_completion| will be run immediately after |completion|.
   void RemoveLoginsByURLAndTime(
       const base::RepeatingCallback<bool(const GURL&)>& url_filter,
       base::Time delete_begin,
       base::Time delete_end,
-      base::OnceClosure completion);
+      base::OnceClosure completion,
+      base::OnceCallback<void(bool)> sync_completion = base::NullCallback());
 
   // Removes all logins created in the given date range. If |completion| is not
   // null, it will be posted to the |main_task_runner_| after deletions have
@@ -573,6 +577,8 @@ class PasswordStore : protected PasswordStoreSync,
   // been changed.
   void NotifyLoginsChanged(const PasswordStoreChangeList& changes) override;
 
+  void NotifyDeletionsHaveSynced(bool success) override;
+
   void NotifyUnsyncedCredentialsWillBeDeleted(
       const std::vector<autofill::PasswordForm>& unsynced_credentials) override;
 
@@ -700,7 +706,8 @@ class PasswordStore : protected PasswordStoreSync,
       const base::RepeatingCallback<bool(const GURL&)>& url_filter,
       base::Time delete_begin,
       base::Time delete_end,
-      base::OnceClosure completion);
+      base::OnceClosure completion,
+      base::OnceCallback<void(bool)> sync_completion);
   void RemoveLoginsCreatedBetweenInternal(base::Time delete_begin,
                                           base::Time delete_end,
                                           base::OnceClosure completion);
@@ -860,9 +867,14 @@ class PasswordStore : protected PasswordStoreSync,
 
   std::unique_ptr<UnsyncedCredentialsDeletionNotifier> deletion_notifier_;
 
-  bool shutdown_called_;
+  // A list of callbacks that should be run once all pending deletions have been
+  // sent to the Sync server. Note that the vector itself lives on the
+  // background thread, but the callbacks must be run on the main thread!
+  std::vector<base::OnceCallback<void(bool)>> deletions_have_synced_callbacks_;
+
+  bool shutdown_called_ = false;
 
-  InitStatus init_status_;
+  InitStatus init_status_ = InitStatus::kUnknown;
 
   DISALLOW_COPY_AND_ASSIGN(PasswordStore);
 };

components/password_manager/core/browser/password_store_default.cc

@@ -40,6 +40,11 @@ bool PasswordStoreDefault::InitOnBackgroundSequence() {
     success = false;
     LOG(ERROR) << "Could not create/open login database.";
   }
+  if (success) {
+    login_db_->SetDeletionsHaveSyncedCallback(
+        base::BindRepeating(&PasswordStoreDefault::NotifyDeletionsHaveSynced,
+                            base::Unretained(this)));
+  }
   return PasswordStore::InitOnBackgroundSequence() && success;
 }
 

components/password_manager/core/browser/password_store_sync.h

@@ -102,6 +102,20 @@ class PasswordStoreSync {
 
     // Deletes all the stored sync metadata for passwords.
     virtual void DeleteAllSyncMetadata() = 0;
+
+    // Registers a callback that will be invoked whenever all pending (unsynced)
+    // deletions are gone. If they were committed to the server (or, rarely, the
+    // entity was undeleted), the |callback| will be run with "true". If the
+    // deletions are gone because Sync was permanently turned off, it'll be run
+    // with "false" instead.
+    // Note that there can be only one such callback; if one was already
+    // registered, it'll be overridden by the new |callback|.
+    virtual void SetDeletionsHaveSyncedCallback(
+        base::RepeatingCallback<void(bool)> callback) = 0;
+
+    // Returns whether there are any pending deletions that have not been sent
+    // to the Sync server yet.
+    virtual bool HasUnsyncedDeletions() = 0;
   };
 
   PasswordStoreSync();
@@ -149,6 +163,11 @@ class PasswordStoreSync {
   // Notifies observers that password store data may have been changed.
   virtual void NotifyLoginsChanged(const PasswordStoreChangeList& changes) = 0;
 
+  // Notifies any waiting callback that all pending deletions have been
+  // committed to the Sync server now, or that Sync definitely won't commit
+  // them (because Sync was turned off permanently).
+  virtual void NotifyDeletionsHaveSynced(bool success) = 0;
+
   // Notifies the UI that some unsynced credentials will be deleted on sign-out
   // in order to offer the user the option of saving them in the profile store.
   // Should only be called for the account store.

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

@@ -185,6 +185,9 @@ class MockSyncMetadataStore : public PasswordStoreSync::MetadataStore {
   MOCK_METHOD2(UpdateModelTypeState,
                bool(syncer::ModelType, const sync_pb::ModelTypeState&));
   MOCK_METHOD1(ClearModelTypeState, bool(syncer::ModelType));
+  MOCK_METHOD1(SetDeletionsHaveSyncedCallback,
+               void(base::RepeatingCallback<void(bool)>));
+  MOCK_METHOD0(HasUnsyncedDeletions, bool());
 };
 
 class MockPasswordStoreSync : public PasswordStoreSync {
@@ -205,6 +208,8 @@ class MockPasswordStoreSync : public PasswordStoreSync {
   MOCK_METHOD1(RemoveLoginSync,
                PasswordStoreChangeList(const autofill::PasswordForm&));
   MOCK_METHOD1(NotifyLoginsChanged, void(const PasswordStoreChangeList&));
+  MOCK_METHOD1(NotifyDeletionsHaveSynced, void(bool));
+
   MOCK_METHOD1(
       NotifyUnsyncedCredentialsWillBeDeleted,
       void(const std::vector<autofill::PasswordForm>& unsynced_credentials));

components/password_manager/core/browser/test_password_store.cc

@@ -42,6 +42,9 @@ class TestPasswordSyncMetadataStore : public PasswordStoreSync::MetadataStore {
   bool ClearModelTypeState(syncer::ModelType model_type) override;
   std::unique_ptr<syncer::MetadataBatch> GetAllSyncMetadata() override;
   void DeleteAllSyncMetadata() override;
+  void SetDeletionsHaveSyncedCallback(
+      base::RepeatingCallback<void(bool)> callback) override;
+  bool HasUnsyncedDeletions() override;
 
  private:
   sync_pb::ModelTypeState sync_model_type_state_;
@@ -60,7 +63,7 @@ bool TestPasswordSyncMetadataStore::UpdateSyncMetadata(
 bool TestPasswordSyncMetadataStore::ClearSyncMetadata(
     syncer::ModelType model_type,
     const std::string& storage_key) {
-  sync_metadata_.clear();
+  sync_metadata_.erase(storage_key);
   return true;
 }
 
@@ -96,6 +99,15 @@ void TestPasswordSyncMetadataStore::DeleteAllSyncMetadata() {
   sync_metadata_.clear();
 }
 
+void TestPasswordSyncMetadataStore::SetDeletionsHaveSyncedCallback(
+    base::RepeatingCallback<void(bool)> callback) {
+  NOTIMPLEMENTED();
+}
+
+bool TestPasswordSyncMetadataStore::HasUnsyncedDeletions() {
+  return false;
+}
+
 }  // namespace
 
 TestPasswordStore::TestPasswordStore(bool is_account_store)