Reland 0a1b4fb27a194a6c4ec3fa22040a4e8152cd45b
(https://chromium-review.googlesource.com/c/chromium/src/+/1344880)
Relative to the original CL, this changes the setter for skipping
scheduled updates into a global value that can be set by browser tests
prior to constructing and kicking off the extension updater. This is
the pattern used by content verification and other extension-behavior
customization.
It's my hope that taking this approach will eliminate a lot of long-
standing flakiness in several of the tests, so if this sticks I can
follow it up with a CL re-enabling several tests. (Or another person
who has a better relationship with extensions can.)
OCL description:
> Simplify extension update check delay after browser start.
>
> Bug: 907219
> Change-Id: I067fcb333b43f60441e1789cac8caf5b996b7e5d
> Reviewed-on: https://chromium-review.googlesource.com/c/1344880
> Commit-Queue: Joshua Pawlicki <[email protected]>
> Reviewed-by: Gabriel Charette <[email protected]>
> Reviewed-by: Sergey Poromov <[email protected]>
> Reviewed-by: Brian White <[email protected]>
> Reviewed-by: Devlin <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#620845}
Bug: 907219, 920040
Change-Id: I35ff0311f84294eca6cd1b23a3fd612d53ea42db
Reviewed-on: https://chromium-review.googlesource.com/c/1412796
Auto-Submit: Joshua Pawlicki <[email protected]>
Reviewed-by: Devlin <[email protected]>
Reviewed-by: Brian White <[email protected]>
Reviewed-by: Sergey Poromov <[email protected]>
Reviewed-by: Gabriel Charette <[email protected]>
Commit-Queue: Joshua Pawlicki <[email protected]>
Cr-Commit-Position: refs/heads/master@{#624302}diff --git a/chrome/browser/extensions/extension_browsertest.h b/chrome/browser/extensions/extension_browsertest.h
index 1a8cdad..4c8544c9 100644
--- a/chrome/browser/extensions/extension_browsertest.h
+++ b/chrome/browser/extensions/extension_browsertest.h
@@ -15,6 +15,7 @@
#include "build/build_config.h"
#include "chrome/browser/extensions/chrome_extension_test_notification_observer.h"
#include "chrome/browser/extensions/install_verifier.h"
+#include "chrome/browser/extensions/updater/extension_updater.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/test/base/in_process_browser_test.h"
@@ -418,6 +419,8 @@
std::unique_ptr<ScopedInstallVerifierBypassForTest>
ignore_install_verification_;
+ ExtensionUpdater::ScopedSkipScheduledCheckForTest skip_scheduled_check_;
+
DISALLOW_COPY_AND_ASSIGN(ExtensionBrowserTest);
};
diff --git a/chrome/browser/extensions/updater/extension_updater.cc b/chrome/browser/extensions/updater/extension_updater.cc
index 3d91582..00aef112 100644
--- a/chrome/browser/extensions/updater/extension_updater.cc
+++ b/chrome/browser/extensions/updater/extension_updater.cc
@@ -52,20 +52,14 @@
namespace {
-// Wait at least 60 seconds after browser startup before we do any checks. If
-// you change this value, make sure to update comments where it is used.
-const int kStartupWaitSeconds = 60;
-
-// The minimum number of seconds there should be for the delay passed to
-// ScheduleNextCheck.
-const int kScheduleNextCheckMinGapSecs = 1;
-
// For sanity checking on update frequency - enforced in release mode only.
#if defined(NDEBUG)
const int kMinUpdateFrequencySeconds = 30;
#endif
const int kMaxUpdateFrequencySeconds = 60 * 60 * 24 * 7; // 7 days
+bool g_skip_scheduled_checks_for_tests = false;
+
// When we've computed a days value, we want to make sure we don't send a
// negative value (due to the system clock being set backwards, etc.), since -1
// is a special sentinel value that means "never pinged", and other negative
@@ -146,7 +140,7 @@
service_(service),
downloader_factory_(downloader_factory),
update_service_(nullptr),
- frequency_seconds_(frequency_seconds),
+ frequency_(base::TimeDelta::FromSeconds(frequency_seconds)),
will_check_soon_(false),
extension_prefs_(extension_prefs),
prefs_(prefs),
@@ -155,13 +149,13 @@
crx_install_is_running_(false),
extension_cache_(cache),
weak_ptr_factory_(this) {
- DCHECK_GE(frequency_seconds_, 5);
- DCHECK_LE(frequency_seconds_, kMaxUpdateFrequencySeconds);
+ DCHECK_LE(frequency_seconds, kMaxUpdateFrequencySeconds);
#if defined(NDEBUG)
// In Release mode we enforce that update checks don't happen too often.
- frequency_seconds_ = std::max(frequency_seconds_, kMinUpdateFrequencySeconds);
+ frequency_seconds = std::max(frequency_seconds, kMinUpdateFrequencySeconds);
#endif
- frequency_seconds_ = std::min(frequency_seconds_, kMaxUpdateFrequencySeconds);
+ frequency_seconds = std::min(frequency_seconds, kMaxUpdateFrequencySeconds);
+ frequency_ = base::TimeDelta::FromSeconds(frequency_seconds);
}
ExtensionUpdater::~ExtensionUpdater() {
@@ -177,38 +171,6 @@
}
}
-// The overall goal here is to balance keeping clients up to date while
-// avoiding a thundering herd against update servers.
-base::TimeDelta ExtensionUpdater::DetermineFirstCheckDelay() {
- DCHECK(alive_);
- // If someone's testing with a quick frequency, just allow it.
- if (frequency_seconds_ < kStartupWaitSeconds)
- return base::TimeDelta::FromSeconds(frequency_seconds_);
-
- // If we've never scheduled a check before, start at a random time up to
- // frequency_seconds_ away.
- if (!prefs_->HasPrefPath(pref_names::kNextUpdateCheck))
- return base::TimeDelta::FromSeconds(
- RandInt(kStartupWaitSeconds, frequency_seconds_));
-
- // Read the persisted next check time, and use that if it isn't in the past
- // or too far in the future (this can happen with system clock changes).
- base::Time saved_next = base::Time::FromInternalValue(
- prefs_->GetInt64(pref_names::kNextUpdateCheck));
- base::Time now = base::Time::Now();
- base::Time earliest =
- now + base::TimeDelta::FromSeconds(kScheduleNextCheckMinGapSecs);
- base::Time latest = now + base::TimeDelta::FromSeconds(frequency_seconds_);
- if (saved_next > earliest && saved_next < latest) {
- return saved_next - now;
- }
-
- // In most cases we'll get here because the persisted next check time passed
- // while we weren't running, so pick something soon.
- return base::TimeDelta::FromSeconds(
- RandInt(kStartupWaitSeconds, kStartupWaitSeconds * 5));
-}
-
void ExtensionUpdater::Start() {
DCHECK(!alive_);
// If these are NULL, then that means we've been called after Stop()
@@ -219,8 +181,11 @@
DCHECK(profile_);
DCHECK(!weak_ptr_factory_.HasWeakPtrs());
alive_ = true;
- // Make sure our prefs are registered, then schedule the first check.
- ScheduleNextCheck(DetermineFirstCheckDelay());
+ // Check soon, and set up the first delayed check.
+ if (!g_skip_scheduled_checks_for_tests) {
+ CheckSoon();
+ ScheduleNextCheck();
+ }
}
void ExtensionUpdater::Stop() {
@@ -230,55 +195,29 @@
extension_prefs_ = NULL;
prefs_ = NULL;
profile_ = NULL;
- timer_.Stop();
will_check_soon_ = false;
downloader_.reset();
update_service_ = nullptr;
}
-void ExtensionUpdater::ScheduleNextCheck(const base::TimeDelta& target_delay) {
+void ExtensionUpdater::ScheduleNextCheck() {
DCHECK(alive_);
- DCHECK(!timer_.IsRunning());
- DCHECK(target_delay >=
- base::TimeDelta::FromSeconds(kScheduleNextCheckMinGapSecs));
-
- // Add +/- 10% random jitter.
- double delay_ms = target_delay.InMillisecondsF();
- double jitter_factor = (RandDouble() * .2) - 0.1;
- delay_ms += delay_ms * jitter_factor;
- base::TimeDelta actual_delay =
- base::TimeDelta::FromMilliseconds(static_cast<int64_t>(delay_ms));
-
- // Save the time of next check.
- base::Time next = base::Time::Now() + actual_delay;
- prefs_->SetInt64(pref_names::kNextUpdateCheck, next.ToInternalValue());
-
- timer_.Start(FROM_HERE, actual_delay, this, &ExtensionUpdater::TimerFired);
+ // Jitter the frequency by +/- 20%.
+ const double jitter_factor = RandDouble() * 0.4 + 0.8;
+ base::TimeDelta delay = base::TimeDelta::FromMilliseconds(
+ static_cast<int64_t>(frequency_.InMilliseconds() * jitter_factor));
+ base::PostDelayedTaskWithTraits(
+ FROM_HERE, {base::TaskPriority::BEST_EFFORT, BrowserThread::UI},
+ base::BindOnce(&ExtensionUpdater::NextCheck,
+ weak_ptr_factory_.GetWeakPtr()),
+ delay);
}
-void ExtensionUpdater::TimerFired() {
- DCHECK(alive_);
+void ExtensionUpdater::NextCheck() {
+ if (!alive_)
+ return;
CheckNow(CheckParams());
-
- // If the user has overridden the update frequency, don't bother reporting
- // this.
- if (frequency_seconds_ == kDefaultUpdateFrequencySeconds) {
- base::Time last = base::Time::FromInternalValue(
- prefs_->GetInt64(pref_names::kLastUpdateCheck));
- if (last.ToInternalValue() != 0) {
- // Use counts rather than time so we can use minutes rather than millis.
- UMA_HISTOGRAM_CUSTOM_COUNTS(
- "Extensions.UpdateCheckGap", (base::Time::Now() - last).InMinutes(),
- base::TimeDelta::FromSeconds(kStartupWaitSeconds).InMinutes(),
- base::TimeDelta::FromDays(40).InMinutes(),
- 50); // 50 buckets seems to be the default.
- }
- }
-
- // Save the last check time, and schedule the next check.
- int64_t now = base::Time::Now().ToInternalValue();
- prefs_->SetInt64(pref_names::kLastUpdateCheck, now);
- ScheduleNextCheck(base::TimeDelta::FromSeconds(frequency_seconds_));
+ ScheduleNextCheck();
}
void ExtensionUpdater::CheckSoon() {
@@ -286,7 +225,7 @@
if (will_check_soon_)
return;
if (base::PostTaskWithTraits(
- FROM_HERE, {BrowserThread::UI},
+ FROM_HERE, {base::TaskPriority::BEST_EFFORT, BrowserThread::UI},
base::BindOnce(&ExtensionUpdater::DoCheckSoon,
weak_ptr_factory_.GetWeakPtr()))) {
will_check_soon_ = true;
@@ -304,10 +243,6 @@
extension_cache_ = extension_cache;
}
-void ExtensionUpdater::StopTimerForTesting() {
- timer_.Stop();
-}
-
void ExtensionUpdater::DoCheckSoon() {
DCHECK(will_check_soon_);
CheckNow(CheckParams());
@@ -706,4 +641,15 @@
requests_in_progress_.erase(request_id);
}
+ExtensionUpdater::ScopedSkipScheduledCheckForTest::
+ ScopedSkipScheduledCheckForTest() {
+ DCHECK(!g_skip_scheduled_checks_for_tests);
+ g_skip_scheduled_checks_for_tests = true;
+}
+
+ExtensionUpdater::ScopedSkipScheduledCheckForTest::
+ ~ScopedSkipScheduledCheckForTest() {
+ g_skip_scheduled_checks_for_tests = false;
+}
+
} // namespace extensions
diff --git a/chrome/browser/extensions/updater/extension_updater.h b/chrome/browser/extensions/updater/extension_updater.h
index 656484e..3e4f1b56 100644
--- a/chrome/browser/extensions/updater/extension_updater.h
+++ b/chrome/browser/extensions/updater/extension_updater.h
@@ -18,8 +18,6 @@
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "base/scoped_observer.h"
-#include "base/time/time.h"
-#include "base/timer/timer.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "extensions/browser/extension_registry_observer.h"
@@ -78,9 +76,9 @@
// right away.
bool install_immediately;
- // An extension update check can be originated by a user or by a timer.
- // When the value of |fetch_priority| is FOREGROUND, the update request was
- // initiated by a user.
+ // An extension update check can be originated by a user or by a scheduled
+ // task. When the value of |fetch_priority| is FOREGROUND, the update
+ // request was initiated by a user.
ManifestFetchData::FetchPriority fetch_priority;
// Callback to call when the update check is complete. Can be null, if
@@ -88,6 +86,18 @@
FinishedCallback callback;
};
+ // A class for use in tests to skip scheduled update checks for extensions
+ // during the lifetime of an instance of it. Only one instance should be alive
+ // at any given time.
+ class ScopedSkipScheduledCheckForTest {
+ public:
+ ScopedSkipScheduledCheckForTest();
+ ~ScopedSkipScheduledCheckForTest();
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(ScopedSkipScheduledCheckForTest);
+ };
+
// Holds a pointer to the passed |service|, using it for querying installed
// extensions and installing updated ones. The |frequency_seconds| parameter
// controls how often update checks are scheduled.
@@ -128,9 +138,6 @@
// Overrides the extension cache with |extension_cache| for testing.
void SetExtensionCacheForTesting(ExtensionCache* extension_cache);
- // Stop the timer to prevent scheduled updates for testing.
- void StopTimerForTesting();
-
private:
friend class ExtensionUpdaterTest;
friend class ExtensionUpdaterFileHandler;
@@ -170,14 +177,9 @@
// |downloader|.
void EnsureDownloaderCreated();
- // Computes when to schedule the first update check.
- base::TimeDelta DetermineFirstCheckDelay();
-
- // Sets the timer to call TimerFired after roughly |target_delay| from now.
- // To help spread load evenly on servers, this method adds some random
- // jitter. It also saves the scheduled time so it can be reloaded on
- // browser restart.
- void ScheduleNextCheck(const base::TimeDelta& target_delay);
+ // Schedules a task to call NextCheck after |frequency_| delay, plus
+ // or minus 0 to 20% (to help spread load evenly on servers).
+ void ScheduleNextCheck();
// Add fetch records for extensions that are installed to the downloader,
// ignoring |pending_ids| so the extension isn't fetched again.
@@ -187,8 +189,8 @@
ManifestFetchData::FetchPriority fetch_priority,
ExtensionUpdateCheckParams* update_check_params);
- // BaseTimer::ReceiverMethod callback.
- void TimerFired();
+ // Conduct a check as scheduled by ScheduleNextCheck.
+ void NextCheck();
// Posted by CheckSoon().
void DoCheckSoon();
@@ -254,8 +256,8 @@
// shutdown.
UpdateService* update_service_;
- base::OneShotTimer timer_;
- int frequency_seconds_;
+ bool do_scheduled_checks_;
+ base::TimeDelta frequency_;
bool will_check_soon_;
ExtensionPrefs* extension_prefs_;
diff --git a/chrome/browser/extensions/updater/extension_updater_unittest.cc b/chrome/browser/extensions/updater/extension_updater_unittest.cc
index 6013e41..2d8d76f 100644
--- a/chrome/browser/extensions/updater/extension_updater_unittest.cc
+++ b/chrome/browser/extensions/updater/extension_updater_unittest.cc
@@ -685,11 +685,7 @@
base::RunLoop().RunUntilIdle();
}
- void SimulateTimerFired(ExtensionUpdater* updater) {
- EXPECT_TRUE(updater->timer_.IsRunning());
- updater->timer_.Stop();
- updater->TimerFired();
- }
+ void SimulateTimerFired(ExtensionUpdater* updater) { updater->NextCheck(); }
// Adds a Result with the given data to results.
void AddParseResult(const std::string& id,
@@ -1436,6 +1432,7 @@
}
void TestSingleExtensionDownloading(bool pending, bool retry, bool fail) {
+ ExtensionUpdater::ScopedSkipScheduledCheckForTest skip_scheduled_checks;
std::unique_ptr<ServiceForDownloadTests> service =
std::make_unique<ServiceForDownloadTests>(
prefs_.get(), test_shared_url_loader_factory_);
@@ -2589,6 +2586,8 @@
service.GetDownloaderFactory());
EXPECT_FALSE(updater.WillCheckSoon());
updater.Start();
+ EXPECT_TRUE(updater.WillCheckSoon());
+ RunUntilIdle();
EXPECT_FALSE(updater.WillCheckSoon());
updater.CheckSoon();
EXPECT_TRUE(updater.WillCheckSoon());
@@ -2648,23 +2647,6 @@
RunUntilIdle();
}
-// Tests that we don't get a DCHECK failure when the next check time saved in
-// prefs happens to be within one second of startup.
-TEST_F(ExtensionUpdaterTest, TestPersistedNextCheckTime) {
- base::Time next_check_time =
- base::Time::Now() + base::TimeDelta::FromMilliseconds(500);
- prefs_->pref_service()->SetInt64(pref_names::kNextUpdateCheck,
- next_check_time.ToInternalValue());
- ServiceForManifestTests service(prefs_.get(),
- test_shared_url_loader_factory_);
- ExtensionUpdater updater(&service, service.extension_prefs(),
- service.pref_service(), service.profile(),
- kDefaultUpdateFrequencySeconds, nullptr,
- service.GetDownloaderFactory());
- updater.Start();
- updater.Stop();
-}
-
TEST_F(ExtensionUpdaterTest, TestManifestFetchDataAddExtension) {
TestManifestAddExtension(ManifestFetchData::FetchPriority::BACKGROUND,
ManifestFetchData::FetchPriority::BACKGROUND,
diff --git a/chrome/browser/policy/policy_browsertest.cc b/chrome/browser/policy/policy_browsertest.cc
index 6340ee5f..8d1ccf3 100644
--- a/chrome/browser/policy/policy_browsertest.cc
+++ b/chrome/browser/policy/policy_browsertest.cc
@@ -1114,6 +1114,8 @@
MockConfigurationPolicyProvider provider_;
std::unique_ptr<extensions::ExtensionCacheFake> test_extension_cache_;
extensions::ScopedIgnoreContentVerifierForTest ignore_content_verifier_;
+ extensions::ExtensionUpdater::ScopedSkipScheduledCheckForTest
+ skip_scheduled_extension_checks_;
};
// A subclass of PolicyTest that runs each test with the old interstitial code
@@ -2810,9 +2812,6 @@
extensions::ExtensionPrefs* extension_prefs =
extensions::ExtensionPrefs::Get(browser()->profile());
- // Explicitly stop the timer to avoid all scheduled extension auto-updates.
- service->updater()->StopTimerForTesting();
-
// Install the extension.
EXPECT_TRUE(InstallExtension(kGoodV1CrxName));
EXPECT_TRUE(registry->enabled_extensions().Contains(kGoodCrxId));
@@ -2883,9 +2882,6 @@
extensions::ExtensionPrefs* extension_prefs =
extensions::ExtensionPrefs::Get(browser()->profile());
- // Explicitly stop the timer to avoid all scheduled extension auto-updates.
- service->updater()->StopTimerForTesting();
-
// Set the policy to require an even higher minimum version this time.
{
extensions::ExtensionManagementPolicyUpdater management_policy(&provider_);
diff --git a/chrome/browser/prefs/browser_prefs.cc b/chrome/browser/prefs/browser_prefs.cc
index 554195b..326be03 100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/chrome/browser/prefs/browser_prefs.cc
@@ -384,6 +384,10 @@
// perhaps be kept around longer than the others.
const char kHttpServerProperties[] = "net.http_server_properties";
+// Deprecated 1/2019.
+const char kNextUpdateCheck[] = "extensions.autoupdate.next_check";
+const char kLastUpdateCheck[] = "extensions.autoupdate.last_check";
+
// Register prefs used only for migration (clearing or moving to a new key).
void RegisterProfilePrefsForMigration(
user_prefs::PrefRegistrySyncable* registry) {
@@ -411,6 +415,8 @@
registry->RegisterIntegerPref(kNuxOnboardGroup, 0);
registry->RegisterDictionaryPref(kHttpServerProperties,
PrefRegistry::LOSSY_PREF);
+ registry->RegisterIntegerPref(kLastUpdateCheck, 0);
+ registry->RegisterIntegerPref(kNextUpdateCheck, 0);
}
} // namespace
@@ -891,4 +897,8 @@
// Added 12/2018.
profile_prefs->ClearPref(prefs::kDataSaverPromptsShown);
#endif
+
+ // Added 1/2019.
+ profile_prefs->ClearPref(kLastUpdateCheck);
+ profile_prefs->ClearPref(kNextUpdateCheck);
}
