ambient: Add max retries to refresh token

Current implementation will not return callback until succeed. This
patch adds a limit to the max number of retries. This patch also changes
the backoff logic to use backoff entry.

Bug: b/169250512
Test: added new tests.
Change-Id: Id421a152dc7fe6ce3f365086d16c6e611b4fc001
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2427082
Commit-Queue: Tao Wu <wutao@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810840}

ash/ambient/ambient_access_token_controller.cc

@@ -14,14 +14,22 @@ namespace ash {
 
 namespace {
 
-constexpr base::TimeDelta kMinTokenRefreshDelay =
-    base::TimeDelta::FromMilliseconds(1000);
-constexpr base::TimeDelta kMaxTokenRefreshDelay =
-    base::TimeDelta::FromMilliseconds(60 * 1000);
+constexpr int kMaxRetries = 3;
+
+constexpr net::BackoffEntry::Policy kRetryBackoffPolicy = {
+    0,          // Number of initial errors to ignore.
+    1000,       // Initial delay in ms.
+    2.0,        // Factor by which the waiting time will be multiplied.
+    0.2,        // Fuzzing percentage.
+    60 * 1000,  // Maximum delay in ms.
+    -1,         // Never discard the entry.
+    true,       // Use initial delay.
+};
 
 }  // namespace
 
-AmbientAccessTokenController::AmbientAccessTokenController() = default;
+AmbientAccessTokenController::AmbientAccessTokenController()
+    : refresh_token_retry_backoff_(&kRetryBackoffPolicy) {}
 
 AmbientAccessTokenController::~AmbientAccessTokenController() = default;
 
@@ -75,11 +83,17 @@ void AmbientAccessTokenController::AccessTokenRefreshed(
   has_pending_request_ = false;
 
   if (gaia_id.empty() || access_token.empty()) {
-    RetryRefreshAccessToken();
+    refresh_token_retry_backoff_.InformOfRequest(/*succeeded=*/false);
+    if (refresh_token_retry_backoff_.failure_count() <= kMaxRetries)
+      RetryRefreshAccessToken();
+    else
+      NotifyAccessTokenRefreshed();
+
     return;
   }
 
   VLOG(1) << "Access token fetched.";
+  refresh_token_retry_backoff_.Reset();
   gaia_id_ = gaia_id;
   access_token_ = access_token;
   expiration_time_ = expiration_time;
@@ -88,14 +102,7 @@ void AmbientAccessTokenController::AccessTokenRefreshed(
 
 void AmbientAccessTokenController::RetryRefreshAccessToken() {
   base::TimeDelta backoff_delay =
-      std::min(kMinTokenRefreshDelay *
-                   (1 << (token_refresh_error_backoff_factor - 1)),
-               kMaxTokenRefreshDelay) +
-      base::RandDouble() * kMinTokenRefreshDelay;
-
-  if (backoff_delay < kMaxTokenRefreshDelay)
-    ++token_refresh_error_backoff_factor;
-
+      refresh_token_retry_backoff_.GetTimeUntilRelease();
   token_refresh_timer_.Start(
       FROM_HERE, backoff_delay,
       base::BindOnce(&AmbientAccessTokenController::RefreshAccessToken,
@@ -118,4 +125,8 @@ void AmbientAccessTokenController::SetTokenUsageBufferForTesting(
   token_usage_time_buffer_ = time;
 }
 
+base::TimeDelta AmbientAccessTokenController::GetTimeUntilReleaseForTesting() {
+  return refresh_token_retry_backoff_.GetTimeUntilRelease();
+}
+
 }  // namespace ash

ash/ambient/ambient_access_token_controller.h

@@ -13,6 +13,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
+#include "net/base/backoff_entry.h"
 
 namespace ash {
 
@@ -54,6 +55,8 @@ class ASH_EXPORT AmbientAccessTokenController {
 
   void SetTokenUsageBufferForTesting(base::TimeDelta time);
 
+  base::TimeDelta GetTimeUntilReleaseForTesting();
+
   std::string gaia_id_;
   std::string access_token_;
 
@@ -67,7 +70,8 @@ class ASH_EXPORT AmbientAccessTokenController {
   base::TimeDelta token_usage_time_buffer_ = kTokenUsageTimeBuffer;
 
   base::OneShotTimer token_refresh_timer_;
-  int token_refresh_error_backoff_factor = 1;
+
+  net::BackoffEntry refresh_token_retry_backoff_;
 
   std::vector<AccessTokenCallback> callbacks_;
 

ash/ambient/ambient_controller_unittest.cc

@@ -215,7 +215,7 @@ TEST_F(AmbientControllerTest, ShouldReturnEmptyAccessToken) {
   CloseAmbientScreen();
 }
 
-TEST_F(AmbientControllerTest, ShouldRefreshAccessTokenAfterFailure) {
+TEST_F(AmbientControllerTest, ShouldRetryRefreshAccessTokenAfterFailure) {
   EXPECT_FALSE(IsAccessTokenRequestPending());
 
   // Lock the screen will request a token.
@@ -225,16 +225,72 @@ TEST_F(AmbientControllerTest, ShouldRefreshAccessTokenAfterFailure) {
   EXPECT_FALSE(IsAccessTokenRequestPending());
 
   // Token request automatically retry.
-  // The failure delay has jitter so fast forward a bit more.
-  constexpr base::TimeDelta kMaxTokenRefreshDelay =
-      base::TimeDelta::FromSeconds(60);
-  task_environment()->FastForwardBy(kMaxTokenRefreshDelay * 2);
+  task_environment()->FastForwardBy(GetRefreshTokenDelay() * 1.1);
   EXPECT_TRUE(IsAccessTokenRequestPending());
 
   // Clean up.
   CloseAmbientScreen();
 }
 
+TEST_F(AmbientControllerTest, ShouldRetryRefreshAccessTokenWithBackoffPolicy) {
+  EXPECT_FALSE(IsAccessTokenRequestPending());
+
+  // Lock the screen will request a token.
+  LockScreen();
+  EXPECT_TRUE(IsAccessTokenRequestPending());
+  IssueAccessToken(/*access_token=*/std::string(), /*with_error=*/true);
+  EXPECT_FALSE(IsAccessTokenRequestPending());
+
+  base::TimeDelta delay1 = GetRefreshTokenDelay();
+  task_environment()->FastForwardBy(delay1 * 1.1);
+  EXPECT_TRUE(IsAccessTokenRequestPending());
+  IssueAccessToken(/*access_token=*/std::string(), /*with_error=*/true);
+  EXPECT_FALSE(IsAccessTokenRequestPending());
+
+  base::TimeDelta delay2 = GetRefreshTokenDelay();
+  EXPECT_GT(delay2, delay1);
+
+  task_environment()->FastForwardBy(delay2 * 1.1);
+  EXPECT_TRUE(IsAccessTokenRequestPending());
+
+  // Clean up.
+  CloseAmbientScreen();
+}
+
+TEST_F(AmbientControllerTest, ShouldRetryRefreshAccessTokenOnlyThreeTimes) {
+  EXPECT_FALSE(IsAccessTokenRequestPending());
+
+  // Lock the screen will request a token.
+  LockScreen();
+  EXPECT_TRUE(IsAccessTokenRequestPending());
+  IssueAccessToken(/*access_token=*/std::string(), /*with_error=*/true);
+  EXPECT_FALSE(IsAccessTokenRequestPending());
+
+  // 1st retry.
+  task_environment()->FastForwardBy(GetRefreshTokenDelay() * 1.1);
+  EXPECT_TRUE(IsAccessTokenRequestPending());
+  IssueAccessToken(/*access_token=*/std::string(), /*with_error=*/true);
+  EXPECT_FALSE(IsAccessTokenRequestPending());
+
+  // 2nd retry.
+  task_environment()->FastForwardBy(GetRefreshTokenDelay() * 1.1);
+  EXPECT_TRUE(IsAccessTokenRequestPending());
+  IssueAccessToken(/*access_token=*/std::string(), /*with_error=*/true);
+  EXPECT_FALSE(IsAccessTokenRequestPending());
+
+  // 3rd retry.
+  task_environment()->FastForwardBy(GetRefreshTokenDelay() * 1.1);
+  EXPECT_TRUE(IsAccessTokenRequestPending());
+  IssueAccessToken(/*access_token=*/std::string(), /*with_error=*/true);
+  EXPECT_FALSE(IsAccessTokenRequestPending());
+
+  // Will not retry.
+  task_environment()->FastForwardBy(GetRefreshTokenDelay() * 1.1);
+  EXPECT_FALSE(IsAccessTokenRequestPending());
+
+  CloseAmbientScreen();
+}
+
 TEST_F(AmbientControllerTest,
        CheckAcquireAndReleaseWakeLockWhenBatteryIsCharging) {
   // Simulate a device being connected to a charger initially.

ash/ambient/test/ambient_ash_test_base.cc

@@ -264,6 +264,10 @@ bool AmbientAshTestBase::IsAccessTokenRequestPending() const {
   return ambient_client_->IsAccessTokenRequestPending();
 }
 
+base::TimeDelta AmbientAshTestBase::GetRefreshTokenDelay() {
+  return token_controller()->GetTimeUntilReleaseForTesting();
+}
+
 AmbientController* AmbientAshTestBase::ambient_controller() {
   return Shell::Get()->ambient_controller();
 }

ash/ambient/test/ambient_ash_test_base.h

@@ -96,6 +96,8 @@ class AmbientAshTestBase : public AshTestBase {
 
   bool IsAccessTokenRequestPending() const;
 
+  base::TimeDelta GetRefreshTokenDelay();
+
   AmbientBackgroundImageView* GetAmbientBackgroundImageView();
 
   // Returns the media string view for displaying ongoing media info.