Updater: Set jitter for the client in an external constant.

Bug: 1144151
Change-Id: Ie836759be36fc86e9a3b15b2ee2b6c086bc2f890
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2625227Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Mila Green <milagreen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843112}

chrome/updater/BUILD.gn

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

chrome/updater/configurator.cc

@@ -5,6 +5,8 @@
 #include "chrome/updater/configurator.h"
 
 #include <utility>
+
+#include "base/rand_util.h"
 #include "base/version.h"
 #include "build/build_config.h"
 #include "chrome/updater/activity.h"
@@ -49,7 +51,7 @@ Configurator::Configurator(std::unique_ptr<UpdaterPrefs> prefs)
 Configurator::~Configurator() = default;
 
 int Configurator::InitialDelay() const {
-  return 0;
+  return base::RandInt(0, external_constants_->InitialDelay());
 }
 
 int Configurator::NextCheckDelay() const {

chrome/updater/constants.cc

@@ -48,6 +48,7 @@ const char kUninstallScript[] = "uninstall.cmd";
 // Developer override key names.
 const char kDevOverrideKeyUrl[] = "url";
 const char kDevOverrideKeyUseCUP[] = "use_cup";
+const char kDevOverrideKeyInitialDelay[] = "initial_delay";
 
 // Policy Management constants.
 const char kProxyModeDirect[] = "direct";

chrome/updater/constants.h

@@ -126,6 +126,7 @@ extern const char kUninstallScript[];
 // Developer override keys.
 extern const char kDevOverrideKeyUrl[];
 extern const char kDevOverrideKeyUseCUP[];
+extern const char kDevOverrideKeyInitialDelay[];
 
 // Timing constants.
 #if defined(OS_WIN)
@@ -205,11 +206,7 @@ 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;
+constexpr int kInitialDelay = 60;
 
 }  // namespace updater
 

chrome/updater/external_constants.cc

@@ -24,6 +24,8 @@ class DefaultExternalConstants : public ExternalConstants {
   }
 
   bool UseCUP() const override { return true; }
+
+  int InitialDelay() const override { return kInitialDelay; }
 };
 
 }  // namespace

chrome/updater/external_constants.h

@@ -27,6 +27,13 @@ class ExternalConstants {
   // True if client update protocol signing of update checks is enabled.
   virtual bool UseCUP() const = 0;
 
+  // Returns a random value in seconds 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.
+  virtual int InitialDelay() const = 0;
+
  protected:
   std::unique_ptr<ExternalConstants> next_provider_;
 };

chrome/updater/external_constants_impl.h

@@ -20,6 +20,7 @@ class DevOverrideProvider : public ExternalConstants {
   // Overrides of ExternalConstants:
   std::vector<GURL> UpdateURL() const override;
   bool UseCUP() const override;
+  int InitialDelay() const override;
 };
 
 }  // namespace updater

chrome/updater/external_constants_mac.mm

@@ -42,4 +42,18 @@ bool DevOverrideProvider::UseCUP() const {
   }
 }
 
+int DevOverrideProvider::InitialDelay() const {
+  @autoreleasepool {
+    NSUserDefaults* userDefaults = [[NSUserDefaults alloc]
+        initWithSuiteName:[NSString
+                              stringWithUTF8String:kUserDefaultsSuiteName]];
+    id initial_delay = [userDefaults
+        objectForKey:[NSString
+                         stringWithUTF8String:kDevOverrideKeyInitialDelay]];
+    if (initial_delay)
+      return [initial_delay integerValue];
+    return next_provider_->InitialDelay();
+  }
+}
+
 }  // namespace updater

chrome/updater/external_constants_mac_unittest.mm

@@ -27,6 +27,9 @@ void ClearUserDefaults() {
     [userDefaults
         removeObjectForKey:[NSString
                                stringWithUTF8String:kDevOverrideKeyUseCUP]];
+    [userDefaults
+        removeObjectForKey:
+            [NSString stringWithUTF8String:kDevOverrideKeyInitialDelay]];
   }
 }
 
@@ -52,6 +55,9 @@ TEST_F(DevOverrideTest, TestDevOverrides) {
     [userDefaults
         setBool:NO
          forKey:[NSString stringWithUTF8String:kDevOverrideKeyUseCUP]];
+    [userDefaults
+        setInteger:0
+            forKey:[NSString stringWithUTF8String:kDevOverrideKeyInitialDelay]];
   }
 
   EXPECT_FALSE(consts->UseCUP());
@@ -59,6 +65,7 @@ TEST_F(DevOverrideTest, TestDevOverrides) {
   ASSERT_EQ(urls.size(), 1u);
   EXPECT_EQ(urls[0], GURL("http://localhost:8080"));
   ASSERT_TRUE(urls[0].is_valid());
+  ASSERT_EQ(consts->InitialDelay(), 0);
 }
 
 }  // namespace updater

chrome/updater/external_constants_win.cc

@@ -39,4 +39,15 @@ bool DevOverrideProvider::UseCUP() const {
   return next_provider_->UseCUP();
 }
 
+int DevOverrideProvider::InitialDelay() const {
+  base::win::RegKey key;
+  if (key.Open(HKEY_CURRENT_USER, UPDATE_DEV_KEY, KEY_READ) == ERROR_SUCCESS) {
+    DWORD initial_delay = 0;
+    if (key.ReadValueDW(base::UTF8ToUTF16(kDevOverrideKeyInitialDelay).c_str(),
+                        &initial_delay) == ERROR_SUCCESS)
+      return initial_delay;
+  }
+  return next_provider_->InitialDelay();
+}
+
 }  // namespace updater

chrome/updater/external_constants_win_unittest.cc

@@ -46,12 +46,18 @@ TEST_F(DevOverrideTest, TestDevOverrides) {
   ASSERT_EQ(
       key.WriteValue(base::UTF8ToUTF16(kDevOverrideKeyUseCUP).c_str(), use_cup),
       ERROR_SUCCESS);
+  DWORD initial_delay = 1;
+  ASSERT_EQ(
+      key.WriteValue(base::UTF8ToUTF16(kDevOverrideKeyInitialDelay).c_str(),
+                     initial_delay),
+      ERROR_SUCCESS);
 
   ASSERT_FALSE(consts->UseCUP());
   std::vector<GURL> urls = consts->UpdateURL();
   ASSERT_EQ(urls.size(), 1u);
   ASSERT_EQ(urls[0], GURL("http://localhost:8080"));
   ASSERT_TRUE(urls[0].is_valid());
+  ASSERT_EQ(consts->InitialDelay(), 1);
 }
 
 }  // namespace updater

chrome/updater/test/integration_tests_mac.mm

@@ -112,6 +112,9 @@ void EnterTestMode(const GURL& url) {
     [userDefaults
         setBool:NO
          forKey:[NSString stringWithUTF8String:kDevOverrideKeyUseCUP]];
+    [userDefaults
+        setInteger:0
+            forKey:[NSString stringWithUTF8String:kDevOverrideKeyInitialDelay]];
   }
 }
 

chrome/updater/test/integration_tests_win.cc

@@ -109,6 +109,10 @@ void EnterTestMode(const GURL& url) {
   ASSERT_EQ(key.WriteValue(base::UTF8ToUTF16(kDevOverrideKeyUseCUP).c_str(),
                            DWORD{0}),
             ERROR_SUCCESS);
+  ASSERT_EQ(
+      key.WriteValue(base::UTF8ToUTF16(kDevOverrideKeyInitialDelay).c_str(),
+                     DWORD{0}),
+      ERROR_SUCCESS);
 }
 
 void ExpectInstalled() {

chrome/updater/update_service_internal_impl.cc

@@ -14,7 +14,6 @@
 #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"
@@ -180,7 +179,7 @@ void CheckForUpdatesTask::MaybeCheckForUpdates() {
               base::BindOnce(&CheckForUpdatesTask::MaybeCheckForUpdatesDone,
                              this),
               config_)),
-      UpdateCheckJitter());
+      base::TimeDelta::FromSeconds(config_->InitialDelay()));
 }
 
 void CheckForUpdatesTask::MaybeCheckForUpdatesDone() {
@@ -271,11 +270,6 @@ 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,13 +19,6 @@
 
 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