Revert "Implement TrustedVaultRequest retries"

This reverts commit 45d537c. Reason for revert: Appears to be causing SyncAuthTest.TokenExpiry to flakily fail. For example on https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20(1)/49896/overview sync_auth_test.cc(330): Value of: UpdatedProgressMarkerChecker(GetSyncService(0)).Wait() Actual: false Expected: true And similar failures on a variety of other bots starting with the first build including this CL. Original change's description: > Implement TrustedVaultRequest retries > > This CL adds retry logic with exponential backoff for > TrustedVaultRequest, currently only for device/authentication factor > registration. This may help to get more clients into the healthy state: > device registration is mostly attempted during the startup and there > might be factors that doesn't let it succeed immediately (for example, > there is no internet connection yet or just a transient issue). > > Bug: 1413179 > Change-Id: Icc2cb9bef398ed27d94e6add58b9312b7fc028a5 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4307474 > Reviewed-by: Mikel Astiz <mastiz@chromium.org> > Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com> > Cr-Commit-Position: refs/heads/main@{#1118076} Bug: 1413179 Change-Id: Idc35f547f14a59940d623bcbf087f6728d68f6c5 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4346853 Auto-Submit: Marijn Kruisselbrink <mek@chromium.org> Owners-Override: Marijn Kruisselbrink <mek@chromium.org> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Commit-Queue: Marijn Kruisselbrink <mek@chromium.org> Cr-Commit-Position: refs/heads/main@{#1118194}
chromium · Mar 16, 2023 · 2040c4a · 2040c4a
1 parent 80fcfb9
commit 2040c4a
Show file tree

Hide file tree

Showing 10 changed files with 53 additions and 251 deletions.
diff --git a/components/sync/trusted_vault/trusted_vault_access_token_fetcher.h b/components/sync/trusted_vault/trusted_vault_access_token_fetcher.h
@@ -5,8 +5,6 @@
 #ifndef COMPONENTS_SYNC_TRUSTED_VAULT_TRUSTED_VAULT_ACCESS_TOKEN_FETCHER_H_
 #define COMPONENTS_SYNC_TRUSTED_VAULT_TRUSTED_VAULT_ACCESS_TOKEN_FETCHER_H_
 
-#include <memory>
-
 #include "base/functional/callback.h"
 #include "base/types/expected.h"
 
@@ -48,9 +46,6 @@ class TrustedVaultAccessTokenFetcher {
   // on the caller sequence.
   virtual void FetchAccessToken(const CoreAccountId& account_id,
                                 TokenCallback callback) = 0;
-
-  // May be called on any sequence.
-  virtual std::unique_ptr<TrustedVaultAccessTokenFetcher> Clone() = 0;
 };
 
 }  // namespace syncer

diff --git a/components/sync/trusted_vault/trusted_vault_access_token_fetcher_impl.cc b/components/sync/trusted_vault/trusted_vault_access_token_fetcher_impl.cc
@@ -4,11 +4,9 @@
 
 #include "components/sync/trusted_vault/trusted_vault_access_token_fetcher_impl.h"
 
-#include <memory>
 #include <utility>
 
 #include "base/location.h"
-#include "base/memory/ptr_util.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/types/expected.h"
 #include "components/sync/base/bind_to_task_runner.h"
@@ -42,11 +40,6 @@ TrustedVaultAccessTokenFetcherImpl::TrustedVaultAccessTokenFetcherImpl(
   ui_thread_task_runner_ = base::SequencedTaskRunner::GetCurrentDefault();
 }
 
-TrustedVaultAccessTokenFetcherImpl::TrustedVaultAccessTokenFetcherImpl(
-    base::WeakPtr<TrustedVaultAccessTokenFetcherFrontend> frontend,
-    scoped_refptr<base::SequencedTaskRunner> ui_thread_task_runner)
-    : frontend_(frontend), ui_thread_task_runner_(ui_thread_task_runner) {}
-
 TrustedVaultAccessTokenFetcherImpl::~TrustedVaultAccessTokenFetcherImpl() =
     default;
 
@@ -59,10 +52,4 @@ void TrustedVaultAccessTokenFetcherImpl::FetchAccessToken(
                      BindToCurrentSequence(std::move(callback))));
 }
 
-std::unique_ptr<TrustedVaultAccessTokenFetcher>
-TrustedVaultAccessTokenFetcherImpl::Clone() {
-  return base::WrapUnique(new TrustedVaultAccessTokenFetcherImpl(
-      frontend_, ui_thread_task_runner_));
-}
-
 }  // namespace syncer
diff --git a/components/sync/trusted_vault/trusted_vault_access_token_fetcher_impl.h b/components/sync/trusted_vault/trusted_vault_access_token_fetcher_impl.h
@@ -5,8 +5,6 @@
 #ifndef COMPONENTS_SYNC_TRUSTED_VAULT_TRUSTED_VAULT_ACCESS_TOKEN_FETCHER_IMPL_H_
 #define COMPONENTS_SYNC_TRUSTED_VAULT_TRUSTED_VAULT_ACCESS_TOKEN_FETCHER_IMPL_H_
 
-#include <memory>
-
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "components/sync/trusted_vault/trusted_vault_access_token_fetcher.h"
@@ -19,8 +17,7 @@ namespace syncer {
 
 class TrustedVaultAccessTokenFetcherFrontend;
 
-// Must be created on the UI thread, but can be used (and cloned) on any
-// sequence.
+// Must be created on the UI thread, but can be used on any sequence.
 class TrustedVaultAccessTokenFetcherImpl
     : public TrustedVaultAccessTokenFetcher {
  public:
@@ -35,13 +32,8 @@ class TrustedVaultAccessTokenFetcherImpl
   // TrustedVaultAccessTokenFetcher implementation.
   void FetchAccessToken(const CoreAccountId& account_id,
                         TokenCallback callback) override;
-  std::unique_ptr<TrustedVaultAccessTokenFetcher> Clone() override;
 
  private:
-  TrustedVaultAccessTokenFetcherImpl(
-      base::WeakPtr<TrustedVaultAccessTokenFetcherFrontend> frontend,
-      scoped_refptr<base::SequencedTaskRunner> ui_thread_task_runner);
-
   base::WeakPtr<TrustedVaultAccessTokenFetcherFrontend> frontend_;
   scoped_refptr<base::SequencedTaskRunner> ui_thread_task_runner_;
 };

diff --git a/components/sync/trusted_vault/trusted_vault_connection_impl.cc b/components/sync/trusted_vault/trusted_vault_connection_impl.cc
@@ -10,7 +10,6 @@
 #include "base/base64url.h"
 #include "base/containers/span.h"
 #include "base/files/important_file_writer.h"
-#include "base/time/time.h"
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/sync/protocol/vault.pb.h"
 #include "components/sync/trusted_vault/download_keys_response_handler.h"
@@ -329,24 +328,22 @@ TrustedVaultConnectionImpl::DownloadNewKeys(
     const TrustedVaultKeyAndVersion& last_trusted_vault_key_and_version,
     std::unique_ptr<SecureBoxKeyPair> device_key_pair,
     DownloadNewKeysCallback callback) {
-  // TODO(crbug.com/1413179): consider retries for keys downloading after
-  // initial failure returned to the upper layers.
   auto request = std::make_unique<TrustedVaultRequest>(
-      account_info.account_id, TrustedVaultRequest::HttpMethod::kGet,
+      TrustedVaultRequest::HttpMethod::kGet,
       GURL(trusted_vault_service_url_.spec() +
            GetGetSecurityDomainMemberURLPathAndQuery(
                device_key_pair->public_key().ExportToBytes())),
-      /*serialized_request_proto=*/absl::nullopt,
-      /*max_retry_duration=*/base::Seconds(0), GetOrCreateURLLoaderFactory(),
-      access_token_fetcher_->Clone(),
+      /*serialized_request_proto=*/absl::nullopt, GetOrCreateURLLoaderFactory(),
       TrustedVaultURLFetchReasonForUMA::kDownloadKeys);
 
-  request->FetchAccessTokenAndSendRequest(base::BindOnce(
-      &ProcessDownloadKeysResponse,
-      /*response_processor=*/
-      std::make_unique<DownloadKeysResponseHandler>(
-          last_trusted_vault_key_and_version, std::move(device_key_pair)),
-      std::move(callback)));
+  request->FetchAccessTokenAndSendRequest(
+      account_info.account_id, access_token_fetcher_.get(),
+      base::BindOnce(
+          &ProcessDownloadKeysResponse,
+          /*response_processor=*/
+          std::make_unique<DownloadKeysResponseHandler>(
+              last_trusted_vault_key_and_version, std::move(device_key_pair)),
+          std::move(callback)));
 
   return request;
 }
@@ -356,16 +353,16 @@ TrustedVaultConnectionImpl::DownloadIsRecoverabilityDegraded(
     const CoreAccountInfo& account_info,
     IsRecoverabilityDegradedCallback callback) {
   auto request = std::make_unique<TrustedVaultRequest>(
-      account_info.account_id, TrustedVaultRequest::HttpMethod::kGet,
+      TrustedVaultRequest::HttpMethod::kGet,
       GURL(trusted_vault_service_url_.spec() +
            kGetSecurityDomainURLPathAndQuery),
-      /*serialized_request_proto=*/absl::nullopt,
-      /*max_retry_duration=*/base::Seconds(0), GetOrCreateURLLoaderFactory(),
-      access_token_fetcher_->Clone(),
+      /*serialized_request_proto=*/absl::nullopt, GetOrCreateURLLoaderFactory(),
       TrustedVaultURLFetchReasonForUMA::kDownloadIsRecoverabilityDegraded);
 
-  request->FetchAccessTokenAndSendRequest(base::BindOnce(
-      &ProcessDownloadIsRecoverabilityDegradedResponse, std::move(callback)));
+  request->FetchAccessTokenAndSendRequest(
+      account_info.account_id, access_token_fetcher_.get(),
+      base::BindOnce(&ProcessDownloadIsRecoverabilityDegradedResponse,
+                     std::move(callback)));
 
   return request;
 }
@@ -380,20 +377,20 @@ TrustedVaultConnectionImpl::SendJoinSecurityDomainsRequest(
     absl::optional<int> authentication_factor_type_hint,
     JoinSecurityDomainsCallback callback) {
   auto request = std::make_unique<TrustedVaultRequest>(
-      account_info.account_id, TrustedVaultRequest::HttpMethod::kPost,
+      TrustedVaultRequest::HttpMethod::kPost,
       GURL(trusted_vault_service_url_.spec() + kJoinSecurityDomainsURLPath),
       /*serialized_request_proto=*/
       CreateJoinSecurityDomainsRequest(
           trusted_vault_keys, last_trusted_vault_key_version,
           authentication_factor_public_key, authentication_factor_type,
           authentication_factor_type_hint)
           .SerializeAsString(),
-      kMaxJoinSecurityDomainRetryDuration, GetOrCreateURLLoaderFactory(),
-      access_token_fetcher_->Clone(),
+      GetOrCreateURLLoaderFactory(),
       GetURLFetchReasonForUMAForJoinSecurityDomainsRequest(
           authentication_factor_type));
 
   request->FetchAccessTokenAndSendRequest(
+      account_info.account_id, access_token_fetcher_.get(),
       base::BindOnce(&ProcessJoinSecurityDomainsResponse, std::move(callback)));
   return request;
 }

diff --git a/components/sync/trusted_vault/trusted_vault_connection_impl.h b/components/sync/trusted_vault/trusted_vault_connection_impl.h
@@ -9,7 +9,6 @@
 #include <vector>
 
 #include "base/memory/scoped_refptr.h"
-#include "base/time/time.h"
 #include "components/sync/trusted_vault/trusted_vault_access_token_fetcher.h"
 #include "components/sync/trusted_vault/trusted_vault_connection.h"
 #include "url/gurl.h"
@@ -29,11 +28,6 @@ class TrustedVaultConnectionImpl : public TrustedVaultConnection {
       base::OnceCallback<void(TrustedVaultRegistrationStatus,
                               int /*last_key_version=*/)>;
 
-  // Specifies how long JoinSecurityDomainRequest could be delayed due to
-  // retries in case of transient errors. Exposed for testing.
-  static constexpr base::TimeDelta kMaxJoinSecurityDomainRetryDuration =
-      base::Hours(1);
-
   TrustedVaultConnectionImpl(
       const GURL& trusted_vault_service_url,
       std::unique_ptr<network::PendingSharedURLLoaderFactory>

diff --git a/components/sync/trusted_vault/trusted_vault_connection_impl_unittest.cc b/components/sync/trusted_vault/trusted_vault_connection_impl_unittest.cc
@@ -4,7 +4,6 @@
 
 #include "components/sync/trusted_vault/trusted_vault_connection_impl.h"
 
-#include <memory>
 #include <string>
 #include <utility>
 
@@ -98,11 +97,6 @@ class FakeTrustedVaultAccessTokenFetcher
     std::move(callback).Run(access_token_info_or_error_);
   }
 
-  std::unique_ptr<TrustedVaultAccessTokenFetcher> Clone() override {
-    return std::make_unique<FakeTrustedVaultAccessTokenFetcher>(
-        access_token_info_or_error_);
-  }
-
  private:
   const AccessTokenInfoOrError access_token_info_or_error_;
 };
@@ -185,17 +179,12 @@ class TrustedVaultConnectionImplTest : public testing::Test {
         response_http_code);
   }
 
-  base::test::SingleThreadTaskEnvironment& task_environment() {
-    return task_environment_;
-  }
-
   const std::vector<std::vector<uint8_t>> kTrustedVaultKeys = {{1, 2},
                                                                {1, 2, 3, 4}};
   const GURL kTestURL = GURL("https://test.com/test");
 
  private:
-  base::test::SingleThreadTaskEnvironment task_environment_{
-      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  base::test::SingleThreadTaskEnvironment task_environment_;
 
   network::TestURLLoaderFactory test_url_loader_factory_;
 
@@ -531,9 +520,6 @@ TEST_F(TrustedVaultConnectionImplTest,
           /*authentication_factor_type_hint=*/absl::nullopt, callback.Get());
   ASSERT_THAT(request, NotNull());
 
-  // Advance time to bypass retry logic.
-  task_environment().FastForwardBy(
-      TrustedVaultConnectionImpl::kMaxJoinSecurityDomainRetryDuration);
   EXPECT_CALL(callback, Run(Eq(TrustedVaultRegistrationStatus::kNetworkError)));
   EXPECT_TRUE(RespondToJoinSecurityDomainsRequestWithNetworkError());
 }

diff --git a/components/sync/trusted_vault/trusted_vault_registration_verifier.cc b/components/sync/trusted_vault/trusted_vault_registration_verifier.cc
@@ -83,15 +83,14 @@ void TrustedVaultRegistrationVerifier::VerifyMembership(
       ExtractTrustedVaultServiceURLFromCommandLine();
 
   auto request = std::make_unique<TrustedVaultRequest>(
-      primary_account.account_id, TrustedVaultRequest::HttpMethod::kGet,
+      TrustedVaultRequest::HttpMethod::kGet,
       GURL(trusted_vault_service_url.spec() +
            GetGetSecurityDomainMemberURLPathAndQuery(public_key)),
-      /*serialized_request_proto=*/absl::nullopt,
-      /*max_retry_duration=*/base::Seconds(0), url_loader_factory_,
-      access_token_fetcher_.Clone(),
+      /*serialized_request_proto=*/absl::nullopt, url_loader_factory_,
       TrustedVaultURLFetchReasonForUMA::kDownloadKeys);
 
   request->FetchAccessTokenAndSendRequest(
+      primary_account.account_id, &access_token_fetcher_,
       base::BindOnce([](TrustedVaultRequest::HttpStatus http_status,
                         const std::string& response_body) {
         absl::optional<TrustedVaultDownloadKeysStatus> status =