[Password Manager] Determine status of PasswordStore::Init

Chrome doesn't handle failures when initializing PasswordStore.
This causes a bug that user is asked to save passwords when Chrome is
not able to save them.

This CL includes:
- Check if encryption is available on Mac.
- Check if login database was successfully initialized.

Bug: 479725
Change-Id: Ic491e2b04c98270b984a6f08a431b99863419116
Reviewed-on: https://chromium-review.googlesource.com/1243069
Commit-Queue: Tonko Sabolčec <tsabolcec@google.com>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595068}

chrome/browser/password_manager/password_store_mac.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/password_manager/password_store_mac.h"
 #include "base/metrics/histogram_macros.h"
+#include "components/os_crypt/os_crypt.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 
 using password_manager::MigrationStatus;
@@ -33,9 +34,13 @@ void PasswordStoreMac::ShutdownOnUIThread() {
 
 PasswordStoreMac::~PasswordStoreMac() = default;
 
-void PasswordStoreMac::InitOnBackgroundSequence(
+bool PasswordStoreMac::InitOnBackgroundSequence(
     const syncer::SyncableService::StartSyncFlare& flare) {
-  PasswordStoreDefault::InitOnBackgroundSequence(flare);
+  if (!PasswordStoreDefault::InitOnBackgroundSequence(flare))
+    return false;
+
+  if (!OSCrypt::IsEncryptionAvailable())
+    return false;
 
   if (login_db() && (initial_status_ == MigrationStatus::NOT_STARTED ||
                      initial_status_ == MigrationStatus::FAILED_ONCE ||
@@ -53,6 +58,8 @@ void PasswordStoreMac::InitOnBackgroundSequence(
       "PasswordManager.KeychainMigration.Status",
       static_cast<int>(initial_status_),
       static_cast<int>(MigrationStatus::MIGRATION_STATUS_COUNT));
+
+  return true;
 }
 
 void PasswordStoreMac::UpdateStatusPref(MigrationStatus status) {

chrome/browser/password_manager/password_store_mac.h

@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "components/password_manager/core/browser/keychain_migration_status_mac.h"
+#include "components/password_manager/core/browser/password_store.h"
 #include "components/password_manager/core/browser/password_store_default.h"
 #include "components/prefs/pref_member.h"
 
@@ -35,7 +36,7 @@ class PasswordStoreMac : public password_manager::PasswordStoreDefault {
   ~PasswordStoreMac() override;
 
   // PasswordStore:
-  void InitOnBackgroundSequence(
+  bool InitOnBackgroundSequence(
       const syncer::SyncableService::StartSyncFlare& flare) override;
 
   // Writes status to the prefs.

components/password_manager/core/browser/mock_password_store.cc

@@ -18,7 +18,9 @@ MockPasswordStore::CreateBackgroundTaskRunner() const {
   return base::SequencedTaskRunnerHandle::Get();
 }
 
-void MockPasswordStore::InitOnBackgroundSequence(
-    const syncer::SyncableService::StartSyncFlare& flare) {}
+bool MockPasswordStore::InitOnBackgroundSequence(
+    const syncer::SyncableService::StartSyncFlare& flare) {
+  return true;
+}
 
 }  // namespace password_manager

components/password_manager/core/browser/mock_password_store.h

@@ -97,7 +97,7 @@ class MockPasswordStore : public PasswordStore {
   // PasswordStore:
   scoped_refptr<base::SequencedTaskRunner> CreateBackgroundTaskRunner()
       const override;
-  void InitOnBackgroundSequence(
+  bool InitOnBackgroundSequence(
       const syncer::SyncableService::StartSyncFlare& flare) override;
 };
 

components/password_manager/core/browser/password_store.cc

@@ -15,6 +15,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/task/post_task.h"
+#include "base/task_runner_util.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "build/build_config.h"
 #include "components/autofill/core/common/form_data.h"
@@ -122,7 +123,8 @@ bool PasswordStore::FormDigest::operator==(const FormDigest& other) const {
 PasswordStore::PasswordStore()
     : observers_(new base::ObserverListThreadSafe<Observer>()),
       is_propagating_password_changes_to_web_credentials_enabled_(false),
-      shutdown_called_(false) {}
+      shutdown_called_(false),
+      init_status_(InitStatus::kUnknown) {}
 
 bool PasswordStore::Init(const syncer::SyncableService::StartSyncFlare& flare,
                          PrefService* prefs) {
@@ -134,8 +136,12 @@ bool PasswordStore::Init(const syncer::SyncableService::StartSyncFlare& flare,
   prefs_ = prefs;
   hash_password_manager_.set_prefs(prefs);
 #endif
-  ScheduleTask(base::BindRepeating(&PasswordStore::InitOnBackgroundSequence,
-                                   this, flare));
+  if (background_task_runner_) {
+    base::PostTaskAndReplyWithResult(
+        background_task_runner_.get(), FROM_HERE,
+        base::BindOnce(&PasswordStore::InitOnBackgroundSequence, this, flare),
+        base::BindOnce(&PasswordStore::OnInitCompleted, this));
+  }
   return true;
 }
 
@@ -331,6 +337,10 @@ bool PasswordStore::ScheduleTask(const base::Closure& task) {
   return false;
 }
 
+bool PasswordStore::IsAbleToSavePasswords() const {
+  return init_status_ == InitStatus::kSuccess;
+}
+
 void PasswordStore::ShutdownOnUIThread() {
   ScheduleTask(base::Bind(&PasswordStore::DestroyOnBackgroundSequence, this));
   // The AffiliationService must be destroyed from the main sequence.
@@ -468,7 +478,7 @@ PasswordStore::CreateBackgroundTaskRunner() const {
       {base::MayBlock(), base::TaskPriority::USER_VISIBLE});
 }
 
-void PasswordStore::InitOnBackgroundSequence(
+bool PasswordStore::InitOnBackgroundSequence(
     const syncer::SyncableService::StartSyncFlare& flare) {
   DCHECK(background_task_runner_->RunsTasksInCurrentSequence());
   DCHECK(!syncable_service_);
@@ -480,6 +490,7 @@ void PasswordStore::InitOnBackgroundSequence(
   GetAutofillableLoginsImpl(
       std::make_unique<GetLoginsRequest>(reuse_detector_));
 #endif
+  return true;
 }
 
 void PasswordStore::GetLoginsImpl(const FormDigest& form,
@@ -604,6 +615,12 @@ void PasswordStore::ClearAllEnterprisePasswordHashImpl() {
 }
 #endif
 
+void PasswordStore::OnInitCompleted(bool success) {
+  DCHECK(main_task_runner_->RunsTasksInCurrentSequence());
+  init_status_ = success ? InitStatus::kSuccess : InitStatus::kFailure;
+  // TODO(tsabolcec): Add UMA histogram to record the success rate.
+}
+
 void PasswordStore::Schedule(
     void (PasswordStore::*func)(std::unique_ptr<GetLoginsRequest>),
     PasswordStoreConsumer* consumer) {

components/password_manager/core/browser/password_store.h

@@ -252,6 +252,9 @@ class PasswordStore : protected PasswordStoreSync,
   // Schedules the given |task| to be run on the PasswordStore's TaskRunner.
   bool ScheduleTask(const base::Closure& task);
 
+  // Returns true iff initialization was successful.
+  bool IsAbleToSavePasswords() const;
+
   base::WeakPtr<syncer::SyncableService> GetPasswordSyncableService();
 
 // TODO(crbug.com/706392): Fix password reuse detection for Android.
@@ -369,6 +372,19 @@ class PasswordStore : protected PasswordStoreSync,
   };
 #endif
 
+  // Status of PasswordStore::Init().
+  enum class InitStatus {
+    // Initialization status is still not determined (init hasn't started or
+    // finished yet).
+    kUnknown,
+    // Initialization is successfully finished.
+    kSuccess,
+    // There was an error during initialization and PasswordStore is not ready
+    // to save or get passwords.
+    // Removing passwords may still work.
+    kFailure,
+  };
+
   ~PasswordStore() override;
 
   // Create a TaskRunner to be saved in |background_task_runner_|.
@@ -376,8 +392,9 @@ class PasswordStore : protected PasswordStoreSync,
       const;
 
   // Creates PasswordSyncableService and PasswordReuseDetector instances on the
-  // background sequence. Subclasses can add more logic.
-  virtual void InitOnBackgroundSequence(
+  // background sequence. Subclasses can add more logic. Returns true on
+  // success.
+  virtual bool InitOnBackgroundSequence(
       const syncer::SyncableService::StartSyncFlare& flare);
 
   // Methods below will be run in PasswordStore's own sequence.
@@ -521,6 +538,11 @@ class PasswordStore : protected PasswordStoreSync,
   FRIEND_TEST_ALL_PREFIXES(PasswordStoreTest,
                            UpdatePasswordsStoredForAffiliatedWebsites);
 
+  // Called on the main thread after initialization is completed.
+  // |success| is true if initialization was successful. Sets the
+  // |init_status_|.
+  void OnInitCompleted(bool success);
+
   // Schedule the given |func| to be run in the PasswordStore's own sequence
   // with responses delivered to |consumer| on the current sequence.
   void Schedule(void (PasswordStore::*func)(std::unique_ptr<GetLoginsRequest>),
@@ -676,6 +698,8 @@ class PasswordStore : protected PasswordStoreSync,
 
   bool shutdown_called_;
 
+  InitStatus init_status_;
+
   DISALLOW_COPY_AND_ASSIGN(PasswordStore);
 };
 

components/password_manager/core/browser/password_store_default.cc

@@ -28,15 +28,19 @@ void PasswordStoreDefault::ShutdownOnUIThread() {
   ScheduleTask(base::Bind(&PasswordStoreDefault::ResetLoginDB, this));
 }
 
-void PasswordStoreDefault::InitOnBackgroundSequence(
+bool PasswordStoreDefault::InitOnBackgroundSequence(
     const syncer::SyncableService::StartSyncFlare& flare) {
   DCHECK(background_task_runner()->RunsTasksInCurrentSequence());
   DCHECK(login_db_);
+  bool success = true;
   if (!login_db_->Init()) {
     login_db_.reset();
+    // The initialization should be continued, because PasswordSyncableService
+    // has to be initialized even if database initialization failed.
+    success = false;
     LOG(ERROR) << "Could not create/open login database.";
   }
-  PasswordStore::InitOnBackgroundSequence(flare);
+  return PasswordStore::InitOnBackgroundSequence(flare) && success;
 }
 
 void PasswordStoreDefault::ReportMetricsImpl(

components/password_manager/core/browser/password_store_default.h

@@ -41,7 +41,7 @@ class PasswordStoreDefault : public PasswordStore {
   ~PasswordStoreDefault() override;
 
   // Opens |login_db_| on the background sequence.
-  void InitOnBackgroundSequence(
+  bool InitOnBackgroundSequence(
       const syncer::SyncableService::StartSyncFlare& flare) override;
 
   // Implements PasswordStore interface.