[TrustedVault] Initialize StandaloneTrustedVaultBackend in non-lazy way

There might be no events triggerring lazy initialization of
StandaloneTrustedVaultBackend while device registration needs to be
completed. This CL removes lazy initialization logic and backend is now
always initialized in client ctor.

Bug: 1102340
Change-Id: I83a4e4d79bd7dc0f076d9795a814b514e143f99d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2424305
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809772}

components/sync/trusted_vault/DEPS

@@ -3,6 +3,7 @@ include_rules = [
   "+components/signin/public",
   "+components/sync/base",
   "+components/sync/driver",
+  "+components/sync/engine",
   "+components/sync/protocol",
   "+crypto",
   "+services/network/public",

components/sync/trusted_vault/standalone_trusted_vault_client.cc

@@ -14,6 +14,7 @@
 #include "base/task_runner_util.h"
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/sync/base/bind_to_task_runner.h"
+#include "components/sync/engine/sync_engine_switches.h"
 #include "components/sync/trusted_vault/standalone_trusted_vault_backend.h"
 #include "components/sync/trusted_vault/trusted_vault_access_token_fetcher_impl.h"
 #include "components/sync/trusted_vault/trusted_vault_connection_impl.h"
@@ -115,12 +116,28 @@ void PrimaryAccountObserver::UpdatePrimaryAccountIfNeeded() {
 StandaloneTrustedVaultClient::StandaloneTrustedVaultClient(
     const base::FilePath& file_path,
     signin::IdentityManager* identity_manager)
-    : identity_manager_(identity_manager),
-      file_path_(file_path),
-      backend_task_runner_(
+    : backend_task_runner_(
           base::ThreadPool::CreateSequencedTaskRunner(kBackendTaskTraits)),
       access_token_fetcher_frontend_(identity_manager) {
-  DCHECK(identity_manager_);
+  if (!base::FeatureList::IsEnabled(
+          switches::kSyncSupportTrustedVaultPassphrase)) {
+    return;
+  }
+  // TODO(crbug.com/1113598): populate URLLoaderFactory into
+  // TrustedVaultConnectionImpl ctor.
+  // TODO(crbug.com/1102340): allow setting custom TrustedVaultConnection for
+  // testing.
+  backend_ = base::MakeRefCounted<StandaloneTrustedVaultBackend>(
+      file_path, std::make_unique<TrustedVaultConnectionImpl>(
+                     /*url_loader_factory=*/nullptr,
+                     std::make_unique<TrustedVaultAccessTokenFetcherImpl>(
+                         access_token_fetcher_frontend_.GetWeakPtr())));
+  backend_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&StandaloneTrustedVaultBackend::ReadDataFromDisk,
+                     backend_));
+  primary_account_observer_ = std::make_unique<PrimaryAccountObserver>(
+      backend_task_runner_, backend_, identity_manager);
 }
 
 StandaloneTrustedVaultClient::~StandaloneTrustedVaultClient() = default;
@@ -134,7 +151,7 @@ StandaloneTrustedVaultClient::AddKeysChangedObserver(
 void StandaloneTrustedVaultClient::FetchKeys(
     const CoreAccountInfo& account_info,
     base::OnceCallback<void(const std::vector<std::vector<uint8_t>>&)> cb) {
-  TriggerLazyInitializationIfNeeded();
+  DCHECK(backend_);
   backend_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&StandaloneTrustedVaultBackend::FetchKeys, backend_,
@@ -145,7 +162,7 @@ void StandaloneTrustedVaultClient::StoreKeys(
     const std::string& gaia_id,
     const std::vector<std::vector<uint8_t>>& keys,
     int last_key_version) {
-  TriggerLazyInitializationIfNeeded();
+  DCHECK(backend_);
   backend_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&StandaloneTrustedVaultBackend::StoreKeys,
                                 backend_, gaia_id, keys, last_key_version));
@@ -153,7 +170,7 @@ void StandaloneTrustedVaultClient::StoreKeys(
 }
 
 void StandaloneTrustedVaultClient::RemoveAllStoredKeys() {
-  TriggerLazyInitializationIfNeeded();
+  DCHECK(backend_);
   backend_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&StandaloneTrustedVaultBackend::RemoveAllStoredKeys,
@@ -164,7 +181,7 @@ void StandaloneTrustedVaultClient::RemoveAllStoredKeys() {
 void StandaloneTrustedVaultClient::MarkKeysAsStale(
     const CoreAccountInfo& account_info,
     base::OnceCallback<void(bool)> cb) {
-  TriggerLazyInitializationIfNeeded();
+  DCHECK(backend_);
   base::PostTaskAndReplyWithResult(
       backend_task_runner_.get(), FROM_HERE,
       base::BindOnce(&StandaloneTrustedVaultBackend::MarkKeysAsStale, backend_,
@@ -188,6 +205,7 @@ void StandaloneTrustedVaultClient::WaitForFlushForTesting(
 
 void StandaloneTrustedVaultClient::FetchBackendPrimaryAccountForTesting(
     base::OnceCallback<void(const base::Optional<CoreAccountInfo>&)> cb) const {
+  DCHECK(backend_);
   base::PostTaskAndReplyWithResult(
       backend_task_runner_.get(), FROM_HERE,
       base::BindOnce(
@@ -196,36 +214,6 @@ void StandaloneTrustedVaultClient::FetchBackendPrimaryAccountForTesting(
       std::move(cb));
 }
 
-void StandaloneTrustedVaultClient::TriggerLazyInitializationIfNeeded() {
-  if (backend_) {
-    return;
-  }
-
-  // TODO(crbug.com/1113598): populate URLLoaderFactory into
-  // TrustedVaultConnectionImpl ctor.
-  // TODO(crbug.com/1102340): allow setting custom TrustedVaultConnection for
-  // testing.
-  backend_ = base::MakeRefCounted<StandaloneTrustedVaultBackend>(
-      file_path_, std::make_unique<TrustedVaultConnectionImpl>(
-                      /*url_loader_factory=*/nullptr,
-                      std::make_unique<TrustedVaultAccessTokenFetcherImpl>(
-                          access_token_fetcher_frontend_.GetWeakPtr())));
-  backend_task_runner_->PostTask(
-      FROM_HERE,
-      base::BindOnce(&StandaloneTrustedVaultBackend::ReadDataFromDisk,
-                     backend_));
-
-  // |primary_account_observer_| will populate primary account to |backend_|
-  // whenever it changes and upon construction, if there is a primary account
-  // already.
-  primary_account_observer_ = std::make_unique<PrimaryAccountObserver>(
-      backend_task_runner_, backend_, identity_manager_);
-}
-
-bool StandaloneTrustedVaultClient::IsInitializationTriggeredForTesting() const {
-  return backend_ != nullptr;
-}
-
 void StandaloneTrustedVaultClient::SetRecoverabilityDegradedForTesting() {
   is_recoverability_degraded_for_testing_ = true;
 }

components/sync/trusted_vault/standalone_trusted_vault_client.h

@@ -60,33 +60,25 @@ class StandaloneTrustedVaultClient : public TrustedVaultClient {
   void FetchBackendPrimaryAccountForTesting(
       base::OnceCallback<void(const base::Optional<CoreAccountInfo>&)> callback)
       const;
-  bool IsInitializationTriggeredForTesting() const;
   void SetRecoverabilityDegradedForTesting();
 
  private:
-  void TriggerLazyInitializationIfNeeded();
-
-  // Never null and must outlive this object.
-  signin::IdentityManager* identity_manager_;
-
-  const base::FilePath file_path_;
   const scoped_refptr<base::SequencedTaskRunner> backend_task_runner_;
 
   CallbackList observer_list_;
 
-  // |backend_| constructed lazily in the UI thread, used in
-  // |backend_task_runner_| and destroyed (refcounted) on any thread.
+  // Allows access token fetching for primary account on the ui thread. Passed
+  // as WeakPtr to TrustedVaultAccessTokenFetcherImpl.
+  TrustedVaultAccessTokenFetcherFrontend access_token_fetcher_frontend_;
+
+  // |backend_| constructed in the UI thread, used in |backend_task_runner_|
+  // and destroyed (refcounted) on any thread.
   scoped_refptr<StandaloneTrustedVaultBackend> backend_;
 
   // Observes changes of primary account and populates them into |backend_|.
-  // Created lazily together with |backend_| and holds reference to it and
-  // |backend_task_runner_|.
+  // Holds references to |backend_| and |backend_task_runner_|.
   std::unique_ptr<signin::IdentityManager::Observer> primary_account_observer_;
 
-  // Allows access token fetching for primary account on the ui thread. Passed
-  // as WeakPtr to TrustedVaultAccessTokenFetcherImpl.
-  TrustedVaultAccessTokenFetcherFrontend access_token_fetcher_frontend_;
-
   bool is_recoverability_degraded_for_testing_ = false;
 };
 

components/sync/trusted_vault/standalone_trusted_vault_client_unittest.cc

@@ -9,11 +9,13 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "components/os_crypt/os_crypt.h"
 #include "components/os_crypt/os_crypt_mocker.h"
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/signin/public/identity_manager/identity_test_environment.h"
+#include "components/sync/engine/sync_engine_switches.h"
 #include "components/sync/protocol/local_trusted_vault.pb.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -66,7 +68,10 @@ class StandaloneTrustedVaultClientTest : public testing::Test {
   StandaloneTrustedVaultClientTest()
       : file_path_(CreateUniqueTempDir(&temp_dir_)
                        .Append(base::FilePath(FILE_PATH_LITERAL("some_file")))),
-        client_(file_path_, identity_env_.identity_manager()) {}
+        client_(file_path_, identity_env_.identity_manager()) {
+    override_features_.InitAndEnableFeature(
+        switches::kSyncSupportTrustedVaultPassphrase);
+  }
 
   ~StandaloneTrustedVaultClientTest() override = default;
 
@@ -97,6 +102,7 @@ class StandaloneTrustedVaultClientTest : public testing::Test {
     return fetched_primary_account;
   }
 
+  base::test::ScopedFeatureList override_features_;
   base::test::TaskEnvironment task_environment_;
   base::ScopedTempDir temp_dir_;
   const base::FilePath file_path_;
@@ -105,16 +111,9 @@ class StandaloneTrustedVaultClientTest : public testing::Test {
   StandaloneTrustedVaultClient client_;
 };
 
-TEST_F(StandaloneTrustedVaultClientTest, ShouldNotAutoTriggerInitialization) {
-  EXPECT_FALSE(client_.IsInitializationTriggeredForTesting());
-}
-
 TEST_F(StandaloneTrustedVaultClientTest, ShouldFetchEmptyKeys) {
   const std::string kGaiaId = "user1";
-
-  ASSERT_FALSE(client_.IsInitializationTriggeredForTesting());
   EXPECT_THAT(FetchKeysAndWait(kGaiaId), IsEmpty());
-  EXPECT_TRUE(client_.IsInitializationTriggeredForTesting());
 }
 
 TEST_F(StandaloneTrustedVaultClientTest, ShouldFetchNonEmptyKeys) {
@@ -139,9 +138,12 @@ TEST_F(StandaloneTrustedVaultClientTest, ShouldFetchNonEmptyKeys) {
   ASSERT_NE(-1, base::WriteFile(file_path_, encrypted_data.c_str(),
                                 encrypted_data.size()));
 
-  ASSERT_FALSE(client_.IsInitializationTriggeredForTesting());
-  EXPECT_THAT(FetchKeysAndWait(kGaiaId1), ElementsAre(kKey1));
-  EXPECT_THAT(FetchKeysAndWait(kGaiaId2), ElementsAre(kKey2, kKey3));
+  // Initialize new client to read the data from the file.
+  StandaloneTrustedVaultClient client(file_path_,
+                                      identity_env_.identity_manager());
+  EXPECT_THAT(FetchKeysAndWaitForClient(kGaiaId1, &client), ElementsAre(kKey1));
+  EXPECT_THAT(FetchKeysAndWaitForClient(kGaiaId2, &client),
+              ElementsAre(kKey2, kKey3));
 }
 
 TEST_F(StandaloneTrustedVaultClientTest, ShouldStoreKeys) {
@@ -224,16 +226,10 @@ TEST_F(StandaloneTrustedVaultClientTest, ShouldRemoveAllStoredKeys) {
 // ChromeOS doesn't support sign-out.
 #if !defined(OS_CHROMEOS)
 TEST_F(StandaloneTrustedVaultClientTest, ShouldPopulatePrimaryAccountChanges) {
-  ASSERT_FALSE(client_.IsInitializationTriggeredForTesting());
-
   // Set initial primary account before backend initialization.
   const std::string email1 = "user1@gmail.com";
   identity_env_.SetPrimaryAccount(email1);
 
-  // Trigger backend initialization.
-  ASSERT_THAT(FetchKeysAndWait("user1"), IsEmpty());
-  ASSERT_TRUE(client_.IsInitializationTriggeredForTesting());
-
   // Verify that current primary account corresponds to |email1|.
   base::Optional<CoreAccountInfo> current_primary_account =
       FetchBackendPrimaryAccountAndWait();