Updater: Introduce sub-second and sub-minute jitter in the client.

Bug: 1144151
Change-Id: I4890a1cfb7a02b038683620418e6032e907b6561
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2601281
Commit-Queue: Mila Green <milagreen@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839670}

chrome/updater/BUILD.gn

@@ -309,6 +309,7 @@ if (is_win || is_mac) {
       "test/integration_tests.cc",
       "test/integration_tests.h",
       "unittest_util_unittest.cc",
+      "update_service_internal_impl_unittest.cc",
       "update_service_unittest.cc",
       "updater_unittest.cc",
     ]

chrome/updater/constants.h

@@ -212,6 +212,12 @@ constexpr int kUninstallPingReasonUserNotAnOwner = 1;
 // The file downloaded to a temporary location could not be moved.
 constexpr int kErrorFailedToMoveDownloadedFile = 5;
 
+// TODO(crbug.com/1144151): This constant should be `60` for production code and
+// `0.1` for test code. Due to test timeout limitation, the full jitter interval
+// (0 to 60 seconds) cannot be tested. A jitter in the interval of 0 to 0.1
+// seconds should be used for testing.
+constexpr double kUpdateCheckJitterMultiplier = 0.1;
+
 }  // namespace updater
 
 #endif  // CHROME_UPDATER_CONSTANTS_H_

chrome/updater/update_service_internal_impl.cc

@@ -14,6 +14,7 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
+#include "base/rand_util.h"
 #include "base/task/post_task.h"
 #include "base/task/thread_pool.h"
 #include "base/threading/sequenced_task_runner_handle.h"
@@ -160,22 +161,26 @@ void CheckForUpdatesTask::MaybeCheckForUpdates() {
     return;
   }
 
-  update_service->UpdateAll(
-      base::DoNothing(),
+  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
       base::BindOnce(
-          [](base::OnceClosure closure,
-             scoped_refptr<updater::Configurator> config,
-             UpdateService::Result result) {
-            const int exit_code = static_cast<int>(result);
-            VLOG(0) << "UpdateAll complete: exit_code = " << exit_code;
-            if (result == UpdateService::Result::kSuccess) {
-              config->GetPrefService()->SetTime(
-                  kPrefUpdateTime, base::Time::NowFromSystemTime());
-            }
-            std::move(closure).Run();
-          },
-          base::BindOnce(&CheckForUpdatesTask::MaybeCheckForUpdatesDone, this),
-          config_));
+          &updater::UpdateService::UpdateAll, update_service, base::DoNothing(),
+          base::BindOnce(
+              [](base::OnceClosure closure,
+                 scoped_refptr<updater::Configurator> config,
+                 UpdateService::Result result) {
+                const int exit_code = static_cast<int>(result);
+                VLOG(0) << "UpdateAll complete: exit_code = " << exit_code;
+                if (result == UpdateService::Result::kSuccess) {
+                  config->GetPrefService()->SetTime(
+                      kPrefUpdateTime, base::Time::NowFromSystemTime());
+                }
+                std::move(closure).Run();
+              },
+              base::BindOnce(&CheckForUpdatesTask::MaybeCheckForUpdatesDone,
+                             this),
+              config_)),
+      UpdateCheckJitter());
 }
 
 void CheckForUpdatesTask::MaybeCheckForUpdatesDone() {
@@ -266,6 +271,11 @@ void CheckForUpdatesTask::UninstallPingSent(update_client::Error error) {
 
 }  // namespace
 
+base::TimeDelta UpdateCheckJitter() {
+  return base::TimeDelta::FromSecondsD(base::RandDouble() *
+                                       kUpdateCheckJitterMultiplier);
+}
+
 UpdateServiceInternalImpl::UpdateServiceInternalImpl(
     scoped_refptr<updater::Configurator> config)
     : config_(config) {}

chrome/updater/update_service_internal_impl.h

@@ -19,6 +19,13 @@
 
 namespace updater {
 
+// Returns a random `TimeDelta` value used to delay the start of the automated
+// background tasks such as update checks. This distributes the update server
+// load more uniformly and avoids the problem of a large number of clients
+// creating load spikes on servers when checking for updates and their system
+// time is synchronized by a time server.
+base::TimeDelta UpdateCheckJitter();
+
 class Configurator;
 
 // All functions and callbacks must be called on the same sequence.

chrome/updater/update_service_internal_impl_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/updater/update_service_internal_impl.h"
+
+#include "base/time/time.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace updater {
+
+TEST(UpdateServiceInternalImplTest, UpdateCheckJitter) {
+  for (int i = 0; i < 100; ++i) {
+    base::TimeDelta jitter = UpdateCheckJitter();
+    EXPECT_GE(jitter, base::TimeDelta::FromSeconds(0));
+    EXPECT_LT(jitter, base::TimeDelta::FromSeconds(60));
+  }
+}
+
+}  // namespace updater