Make extensions auto-update schedule persist across browser restarts.

Instead of starting the timer all over again, we instead persist some 
information about when we had scheduled it and when it actually fired. We then 
try to balance keeping clients reasonably up to date with avoiding a 
thundering herd against servers.

BUG=http://crbug.com/12545
TEST=none

Review URL: http://codereview.chromium.org/160433

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@22286 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 163 insertions, 22 deletions

diff --git a/chrome/browser/extensions/extension_updater.cc b/chrome/browser/extensions/extension_updater.cc
index e41071b..6a67dcd 100644
--- a/chrome/browser/extensions/extension_updater.cc
+++ b/chrome/browser/extensions/extension_updater.cc
@@ -10,7 +10,9 @@
 #include "base/logging.h"
 #include "base/file_util.h"
 #include "base/file_version_info.h"
+#include "base/rand_util.h"
 #include "base/string_util.h"
+#include "base/time.h"
 #include "base/thread.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extensions_service.h"
@@ -18,15 +20,28 @@
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_error_reporter.h"
 #include "chrome/common/libxml_utils.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/common/pref_service.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/escape.h"
 #include "net/url_request/url_request_status.h"
 #include "libxml/tree.h"
 
+using base::RandDouble;
+using base::RandInt;
+using base::Time;
+using base::TimeDelta;
+using prefs::kLastExtensionsUpdateCheck;
+using prefs::kNextExtensionsUpdateCheck;
+
 const char* ExtensionUpdater::kExpectedGupdateProtocol = "2.0";
 const char* ExtensionUpdater::kExpectedGupdateXmlns =
     "http://www.google.com/update2/response";
 
+// Wait at least 5 minutes 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 * 5;
+
 // For sanity checking on update frequency - enforced in release mode only.
 static const int kMinUpdateFrequencySeconds = 30;
 static const int kMaxUpdateFrequencySeconds = 60 * 60 * 24 * 7;  // 7 days
@@ -83,10 +98,11 @@ class ExtensionUpdaterFileHandler
 
 
 ExtensionUpdater::ExtensionUpdater(ExtensionUpdateService* service,
+                                   PrefService* prefs,
                                    int frequency_seconds,
                                    MessageLoop* file_io_loop)
     : service_(service), frequency_seconds_(frequency_seconds),
-      file_io_loop_(file_io_loop),
+      file_io_loop_(file_io_loop), prefs_(prefs),
       file_handler_(new ExtensionUpdaterFileHandler(MessageLoop::current(),
                                                     file_io_loop_)) {
   Init();
@@ -110,14 +126,62 @@ void ExtensionUpdater::Init() {
 
 ExtensionUpdater::~ExtensionUpdater() {}
 
+static void EnsureInt64PrefRegistered(PrefService* prefs,
+                                      const wchar_t name[]) {
+  if (!prefs->IsPrefRegistered(name))
+    prefs->RegisterInt64Pref(name, 0);
+}
+
+
+// The overall goal here is to balance keeping clients up to date while
+// avoiding a thundering herd against update servers.
+TimeDelta ExtensionUpdater::DetermineFirstCheckDelay() {
+  // If someone's testing with a quick frequency, just allow it.
+  if (frequency_seconds_ < kStartupWaitSeconds)
+    return TimeDelta::FromSeconds(frequency_seconds_);
+
+  // If we've never scheduled a check before, start at frequency_seconds_.
+  if (!prefs_->HasPrefPath(kNextExtensionsUpdateCheck))
+    return TimeDelta::FromSeconds(frequency_seconds_);
+
+  // If it's been a long time since our last actual check, we want to do one
+  // relatively soon.
+  Time now = Time::Now();
+  Time last = Time::FromInternalValue(prefs_->GetInt64(
+      kLastExtensionsUpdateCheck));
+  int days = (now - last).InDays();
+  if (days >= 30) {
+    // Wait 5-10 minutes.
+    return TimeDelta::FromSeconds(RandInt(kStartupWaitSeconds,
+                                          kStartupWaitSeconds * 2));
+  } else if (days >= 14) {
+    // Wait 10-20 minutes.
+    return TimeDelta::FromSeconds(RandInt(kStartupWaitSeconds * 2,
+                                          kStartupWaitSeconds * 4));
+  } else if (days >= 3) {
+    // Wait 20-40 minutes.
+    return TimeDelta::FromSeconds(RandInt(kStartupWaitSeconds * 4,
+                                          kStartupWaitSeconds * 8));
+  }
+
+  // Read the persisted next check time, and use that if it isn't too soon.
+  // Otherwise pick something random.
+  Time saved_next = Time::FromInternalValue(prefs_->GetInt64(
+      kNextExtensionsUpdateCheck));
+  Time earliest = now + TimeDelta::FromSeconds(kStartupWaitSeconds);
+  if (saved_next >= earliest) {
+    return saved_next - now;
+  } else {
+    return TimeDelta::FromSeconds(RandInt(kStartupWaitSeconds,
+                                          frequency_seconds_));
+  }
+}
+
 void ExtensionUpdater::Start() {
-  // TODO(asargent) Make sure update schedules work across browser restarts by
-  // reading/writing update frequency in profile/settings. *But*, make sure to
-  // wait a reasonable amount of time after browser startup to do the first
-  // check during a given run of the browser. Also remember to update the header
-  // comments when this is implemented. (http://crbug.com/12545).
-  timer_.Start(base::TimeDelta::FromSeconds(frequency_seconds_), this,
-      &ExtensionUpdater::TimerFired);
+  // Make sure our prefs are registered, then schedule the first check.
+  EnsureInt64PrefRegistered(prefs_, kLastExtensionsUpdateCheck);
+  EnsureInt64PrefRegistered(prefs_, kNextExtensionsUpdateCheck);
+  ScheduleNextCheck(DetermineFirstCheckDelay());
 }
 
 void ExtensionUpdater::Stop() {
@@ -254,6 +318,25 @@ void AppendExtensionInfo(std::string* str, const Extension& extension) {
     str->append("x=" + EscapeQueryParamValue(JoinString(parts, '&')));
 }
 
+void ExtensionUpdater::ScheduleNextCheck(const TimeDelta& target_delay) {
+  DCHECK(!timer_.IsRunning());
+  DCHECK(target_delay >= TimeDelta::FromSeconds(1));
+
+  // Add +/- 10% random jitter.
+  double delay_ms = target_delay.InMillisecondsF();
+  double jitter_factor = (RandDouble() * .2) - 0.1;
+  delay_ms += delay_ms * jitter_factor;
+  TimeDelta actual_delay = TimeDelta::FromMilliseconds(
+      static_cast<int64>(delay_ms));
+
+  // Save the time of next check.
+  Time next = Time::Now() + actual_delay;
+  prefs_->SetInt64(kNextExtensionsUpdateCheck, next.ToInternalValue());
+  prefs_->ScheduleSavePersistentPrefs();
+
+  timer_.Start(actual_delay, this, &ExtensionUpdater::TimerFired);
+}
+
 void ExtensionUpdater::TimerFired() {
   // Generate a set of update urls for loaded extensions.
   std::set<GURL> urls;
@@ -290,7 +373,11 @@ void ExtensionUpdater::TimerFired() {
     // scheduled, so we don't need to check before calling it.
     StartUpdateCheck(*iter);
   }
-  timer_.Reset();
+
+  // Save the last check time, and schedule the next check.
+  int64 now = Time::Now().ToInternalValue();
+  prefs_->SetInt64(kLastExtensionsUpdateCheck, now);
+  ScheduleNextCheck(TimeDelta::FromSeconds(frequency_seconds_));
 }
 
 static void ManifestParseError(const char* details, ...) {
diff --git a/chrome/browser/extensions/extension_updater.h b/chrome/browser/extensions/extension_updater.h
index 3973b4e..52b4542 100644
--- a/chrome/browser/extensions/extension_updater.h
+++ b/chrome/browser/extensions/extension_updater.h
@@ -25,10 +25,12 @@ class Extension;
 class ExtensionUpdaterTest;
 class MessageLoop;
 class ExtensionUpdaterFileHandler;
+class PrefService;
 
 // A class for doing auto-updates of installed Extensions. Used like this:
 //
 // ExtensionUpdater* updater = new ExtensionUpdater(my_extensions_service,
+//                                                  pref_service,
 //                                                  update_frequency_secs,
 //                                                  file_io_loop);
 // updater.Start();
@@ -42,16 +44,13 @@ class ExtensionUpdater
   // extensions and installing updated ones. The |frequency_seconds| parameter
   // controls how often update checks are scheduled.
   ExtensionUpdater(ExtensionUpdateService* service,
+                   PrefService* prefs,
                    int frequency_seconds,
                    MessageLoop* file_io_loop);
 
   virtual ~ExtensionUpdater();
 
-  // Starts the updater running, with the first check scheduled for
-  // |frequency_seconds| from now. Eventually ExtensionUpdater will persist the
-  // time the last check happened, and do the first check |frequency_seconds_|
-  // from then (potentially adding a short wait if the browser just started).
-  // (http://crbug.com/12545).
+  // Starts the updater running.
   void Start();
 
   // Stops the updater running, cancelling any outstanding update manifest and
@@ -107,6 +106,9 @@ class ExtensionUpdater
   // Does common work from constructors.
   void Init();
 
+  // Computes when to schedule the first update check.
+  base::TimeDelta DetermineFirstCheckDelay();
+
   // URLFetcher::Delegate interface.
   virtual void OnURLFetchComplete(const URLFetcher* source,
                                   const GURL& url,
@@ -132,6 +134,12 @@ class ExtensionUpdater
   // Callback for when ExtensionsService::Install is finished.
   void OnExtensionInstallFinished(const FilePath& path, Extension* extension);
 
+  // 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);
+
   // BaseTimer::ReceiverMethod callback.
   void TimerFired();
 
@@ -167,12 +175,14 @@ class ExtensionUpdater
   // Pointer back to the service that owns this ExtensionUpdater.
   ExtensionUpdateService* service_;
 
-  base::RepeatingTimer<ExtensionUpdater> timer_;
+  base::OneShotTimer<ExtensionUpdater> timer_;
   int frequency_seconds_;
 
   // The MessageLoop where we should do file I/O.
   MessageLoop* file_io_loop_;
 
+  PrefService* prefs_;
+
   scoped_refptr<ExtensionUpdaterFileHandler> file_handler_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionUpdater);
diff --git a/chrome/browser/extensions/extension_updater_unittest.cc b/chrome/browser/extensions/extension_updater_unittest.cc
index 3db5fed..7c8c2ed 100644
--- a/chrome/browser/extensions/extension_updater_unittest.cc
+++ b/chrome/browser/extensions/extension_updater_unittest.cc
@@ -109,6 +109,25 @@ class MockService : public ExtensionUpdateService {
   DISALLOW_COPY_AND_ASSIGN(MockService);
 };
 
+// Class that contains a PrefService and handles cleanup of a temporary file
+// backing it.
+class ScopedTempPrefService {
+ public:
+  ScopedTempPrefService() {
+    FilePath pref_file = temp_dir_.path().AppendASCII("prefs");
+    prefs_.reset(new PrefService(pref_file, NULL));
+  }
+
+  ~ScopedTempPrefService() {}
+
+  PrefService* get() {
+    return prefs_.get();
+  }
+
+ private:
+  scoped_ptr<PrefService> prefs_;
+  ScopedTempDir temp_dir_;
+};
 
 const char* kIdPrefix = "000000000000000000000000000000000000000";
 
@@ -206,6 +225,12 @@ class ExtensionUpdaterTest : public testing::Test {
     EXPECT_FALSE(ExtensionUpdater::Parse(xml, &result.get()));
   }
 
+  static void SimulateTimerFired(ExtensionUpdater* updater) {
+    EXPECT_TRUE(updater->timer_.IsRunning());
+    updater->timer_.Stop();
+    updater->TimerFired();
+  }
+
   // Make a test ParseResult
   static ExtensionUpdater::ParseResult* MakeParseResult(
       const std::string& id,
@@ -258,12 +283,13 @@ class ExtensionUpdaterTest : public testing::Test {
     TestURLFetcherFactory factory;
     URLFetcher::set_factory(&factory);
     MessageLoop message_loop;
+    ScopedTempPrefService prefs;
     scoped_refptr<ExtensionUpdater> updater =
-      new ExtensionUpdater(&service, 60*60*24, &message_loop);
+      new ExtensionUpdater(&service, prefs.get(), 60*60*24, &message_loop);
     updater->Start();
 
     // Tell the update that it's time to do update checks.
-    updater->TimerFired();
+    SimulateTimerFired(updater.get());
 
     // Get the url our mock fetcher was asked to fetch.
     TestURLFetcher* fetcher =
@@ -301,8 +327,10 @@ class ExtensionUpdaterTest : public testing::Test {
     service.set_extensions(tmp);
 
     MessageLoop message_loop;
+    ScopedTempPrefService prefs;
     scoped_refptr<ExtensionUpdater> updater =
-      new ExtensionUpdater(&service, kUpdateFrequencySecs, &message_loop);
+      new ExtensionUpdater(&service, prefs.get(), kUpdateFrequencySecs,
+                           &message_loop);
 
     // Check passing an empty list of parse results to DetermineUpdates
     ExtensionUpdater::ParseResultList updates;
@@ -331,8 +359,10 @@ class ExtensionUpdaterTest : public testing::Test {
     URLFetcher::set_factory(&factory);
     ServiceForDownloadTests service;
     MessageLoop message_loop;
+    ScopedTempPrefService prefs;
     scoped_refptr<ExtensionUpdater> updater =
-      new ExtensionUpdater(&service, kUpdateFrequencySecs, &message_loop);
+      new ExtensionUpdater(&service, prefs.get(), kUpdateFrequencySecs,
+                           &message_loop);
 
     GURL url1("http://localhost/manifest1");
     GURL url2("http://localhost/manifest2");
@@ -364,8 +394,10 @@ class ExtensionUpdaterTest : public testing::Test {
     TestURLFetcher* fetcher = NULL;
     URLFetcher::set_factory(&factory);
     ServiceForDownloadTests service;
+    ScopedTempPrefService prefs;
     scoped_refptr<ExtensionUpdater> updater =
-      new ExtensionUpdater(&service, kUpdateFrequencySecs, &message_loop);
+      new ExtensionUpdater(&service, prefs.get(), kUpdateFrequencySecs,
+                           &message_loop);
 
     GURL test_url("http://localhost/extension.crx");
 
@@ -401,8 +433,10 @@ class ExtensionUpdaterTest : public testing::Test {
     TestURLFetcher* fetcher = NULL;
     URLFetcher::set_factory(&factory);
     ServiceForDownloadTests service;
+    ScopedTempPrefService prefs;
     scoped_refptr<ExtensionUpdater> updater =
-      new ExtensionUpdater(&service, kUpdateFrequencySecs, &message_loop);
+      new ExtensionUpdater(&service, prefs.get(), kUpdateFrequencySecs,
+                           &message_loop);
 
     GURL url1("http://localhost/extension1.crx");
     GURL url2("http://localhost/extension2.crx");
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index bf657c7..7f2eb05 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -81,7 +81,8 @@ ExtensionsService::ExtensionsService(Profile* profile,
       update_frequency = StringToInt(WideToASCII(command_line->GetSwitchValue(
           switches::kExtensionsUpdateFrequency)));
     }
-    updater_ = new ExtensionUpdater(this, update_frequency, backend_loop_);
+    updater_ = new ExtensionUpdater(this, prefs, update_frequency,
+                                    backend_loop_);
   }
 
   backend_ = new ExtensionsServiceBackend(install_directory_, frontend_loop);
diff --git a/chrome/common/pref_names.cc b/chrome/common/pref_names.cc
index ed4c086..2414dc1 100644
--- a/chrome/common/pref_names.cc
+++ b/chrome/common/pref_names.cc
@@ -527,6 +527,12 @@ const wchar_t kNumKeywords[] = L"user_experience_metrics.num_keywords";
 const wchar_t kEnableExtensions[] = L"extensions.enabled";
 const wchar_t kEnableUserScripts[] = L"extensions.user_scripts_enabled";
 
+// Time of the last, and next scheduled, extensions auto-update checks.
+const wchar_t kLastExtensionsUpdateCheck[] =
+    L"extensions.autoupdate.last_check";
+const wchar_t kNextExtensionsUpdateCheck[] =
+    L"extensions.autoupdate.next_check";
+
 // New Tab Page URLs that should not be shown as most visited thumbnails.
 const wchar_t kNTPMostVisitedURLsBlacklist[] = L"ntp.most_visited_blacklist";
 
diff --git a/chrome/common/pref_names.h b/chrome/common/pref_names.h
index a33ca22..d620347 100644
--- a/chrome/common/pref_names.h
+++ b/chrome/common/pref_names.h
@@ -197,6 +197,9 @@ extern const wchar_t kNumKeywords[];
 extern const wchar_t kEnableExtensions[];
 extern const wchar_t kEnableUserScripts[];
 
+extern const wchar_t kLastExtensionsUpdateCheck[];
+extern const wchar_t kNextExtensionsUpdateCheck[];
+
 extern const wchar_t kNTPMostVisitedURLsBlacklist[];
 extern const wchar_t kNTPMostVisitedPinnedURLs[];
 extern const wchar_t kNTPTipsCache[];