Do not use variations seeds older than 30 days.

Old variations seeds can cause biased populations. Metrics show
that very few users have a seed older than 30 days.

With this change, variations seeds that have been fetched more than
30 days ago are not used. Local time is used to compute the age of
the seed.

BUG=419622

Review URL: https://codereview.chromium.org/1378723002

Cr-Commit-Position: refs/heads/master@{#351902}

4 files changed, 186 insertions, 37 deletions

diff --git a/components/variations/service/variations_service.cc b/components/variations/service/variations_service.cc
index 20ba860..2607ead 100644
--- a/components/variations/service/variations_service.cc
+++ b/components/variations/service/variations_service.cc
@@ -48,6 +48,9 @@ const int kMaxRetrySeedFetch = 5;
 // For the HTTP date headers, the resolution of the server time is 1 second.
 const int64 kServerTimeResolutionMs = 1000;
 
+// Maximum age permitted for a variations seed, in days.
+const int kMaxVariationsSeedAgeDays = 30;
+
 // Wrapper around channel checking, used to enable channel mocking for
 // testing. If the current browser channel is not UNKNOWN, this will return
 // that channel value. Otherwise, if the fake channel flag is provided, this
@@ -130,6 +133,20 @@ void RecordRequestsAllowedHistogram(ResourceRequestsAllowedState state) {
                             RESOURCE_REQUESTS_ALLOWED_ENUM_SIZE);
 }
 
+enum VariationsSeedExpiry {
+  VARIATIONS_SEED_EXPIRY_NOT_EXPIRED,
+  VARIATIONS_SEED_EXPIRY_FETCH_TIME_MISSING,
+  VARIATIONS_SEED_EXPIRY_EXPIRED,
+  VARIATIONS_SEED_EXPIRY_ENUM_SIZE,
+};
+
+// Records UMA histogram with the result of the variations seed expiry check.
+void RecordCreateTrialsSeedExpiry(VariationsSeedExpiry expiry_check_result) {
+  UMA_HISTOGRAM_ENUMERATION("Variations.CreateTrials.SeedExpiry",
+                            expiry_check_result,
+                            VARIATIONS_SEED_EXPIRY_ENUM_SIZE);
+}
+
 // Converts ResourceRequestAllowedNotifier::State to the corresponding
 // ResourceRequestsAllowedState value.
 ResourceRequestsAllowedState ResourceRequestStateToHistogramValue(
@@ -217,6 +234,9 @@ VariationsService::VariationsService(
       resource_request_allowed_notifier_(notifier.Pass()),
       request_count_(0),
       weak_ptr_factory_(this) {
+  DCHECK(client_.get());
+  DCHECK(resource_request_allowed_notifier_.get());
+
   resource_request_allowed_notifier_->Init(this);
 }
 
@@ -229,9 +249,28 @@ bool VariationsService::CreateTrialsFromSeed(base::FeatureList* feature_list) {
   create_trials_from_seed_called_ = true;
 
   variations::VariationsSeed seed;
-  if (!seed_store_.LoadSeed(&seed))
+  if (!LoadSeed(&seed))
     return false;
 
+  const int64 last_fetch_time_internal =
+      local_state_->GetInt64(prefs::kVariationsLastFetchTime);
+  const base::Time last_fetch_time =
+      base::Time::FromInternalValue(last_fetch_time_internal);
+  if (last_fetch_time.is_null()) {
+    // If the last fetch time is missing and we have a seed, then this must be
+    // the first run of Chrome. Store the current time as the last fetch time.
+    RecordLastFetchTime();
+    RecordCreateTrialsSeedExpiry(VARIATIONS_SEED_EXPIRY_FETCH_TIME_MISSING);
+  } else {
+    // Reject the seed if it is more than 30 days old.
+    const base::TimeDelta seed_age = base::Time::Now() - last_fetch_time;
+    if (seed_age.InDays() > kMaxVariationsSeedAgeDays) {
+      RecordCreateTrialsSeedExpiry(VARIATIONS_SEED_EXPIRY_EXPIRED);
+      return false;
+    }
+    RecordCreateTrialsSeedExpiry(VARIATIONS_SEED_EXPIRY_NOT_EXPIRED);
+  }
+
   const base::Version current_version(version_info::GetVersionNumber());
   if (!current_version.IsValid())
     return false;
@@ -259,8 +298,6 @@ bool VariationsService::CreateTrialsFromSeed(base::FeatureList* feature_list) {
 
   // Log the "freshness" of the seed that was just used. The freshness is the
   // time between the last successful seed download and now.
-  const int64 last_fetch_time_internal =
-      local_state_->GetInt64(prefs::kVariationsLastFetchTime);
   if (last_fetch_time_internal) {
     const base::TimeDelta delta =
         now - base::Time::FromInternalValue(last_fetch_time_internal);
@@ -269,34 +306,6 @@ bool VariationsService::CreateTrialsFromSeed(base::FeatureList* feature_list) {
         1, base::TimeDelta::FromDays(30).InMinutes(), 50);
   }
 
-  // Log the skew between the seed date and the system clock/build time to
-  // analyze whether either could be used to make old variations seeds expire
-  // after some time.
-  const int64 seed_date_internal =
-      local_state_->GetInt64(prefs::kVariationsSeedDate);
-  if (seed_date_internal) {
-    const base::Time seed_date =
-        base::Time::FromInternalValue(seed_date_internal);
-    const int system_clock_delta_days = (now - seed_date).InDays();
-    if (system_clock_delta_days < 0) {
-      UMA_HISTOGRAM_COUNTS_100("Variations.SeedDateSkew.SystemClockBehindBy",
-                               -system_clock_delta_days);
-    } else {
-      UMA_HISTOGRAM_COUNTS_100("Variations.SeedDateSkew.SystemClockAheadBy",
-                               system_clock_delta_days);
-    }
-
-    const int build_time_delta_days =
-        (base::GetBuildTime() - seed_date).InDays();
-    if (build_time_delta_days < 0) {
-      UMA_HISTOGRAM_COUNTS_100("Variations.SeedDateSkew.BuildTimeBehindBy",
-                               -build_time_delta_days);
-    } else {
-      UMA_HISTOGRAM_COUNTS_100("Variations.SeedDateSkew.BuildTimeAheadBy",
-                               build_time_delta_days);
-    }
-  }
-
   return true;
 }
 
@@ -525,6 +534,10 @@ bool VariationsService::StoreSeed(const std::string& seed_data,
   return true;
 }
 
+bool VariationsService::LoadSeed(VariationsSeed* seed) {
+  return seed_store_.LoadSeed(seed);
+}
+
 void VariationsService::FetchVariationsSeed() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
diff --git a/components/variations/service/variations_service.h b/components/variations/service/variations_service.h
index 90f7c97..e9b786f 100644
--- a/components/variations/service/variations_service.h
+++ b/components/variations/service/variations_service.h
@@ -207,6 +207,11 @@ class VariationsService
     LOAD_COUNTRY_MAX,
   };
 
+  // Loads the seed from the variations store into |seed|. If successfull,
+  // |seed| will contain the loaded data and true is returned. Set as virtual
+  // so that it can be overridden by tests.
+  virtual bool LoadSeed(VariationsSeed* seed);
+
   // Checks if prerequisites for fetching the Variations seed are met, and if
   // so, performs the actual fetch using |DoActualFetch|.
   void FetchVariationsSeed();
diff --git a/components/variations/service/variations_service_unittest.cc b/components/variations/service/variations_service_unittest.cc
index 5f2382b..1b85ce7 100644
--- a/components/variations/service/variations_service_unittest.cc
+++ b/components/variations/service/variations_service_unittest.cc
@@ -7,6 +7,7 @@
 #include <vector>
 
 #include "base/base64.h"
+#include "base/feature_list.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/message_loop/message_loop.h"
 #include "base/prefs/testing_pref_service.h"
@@ -106,21 +107,28 @@ class TestVariationsService : public VariationsService {
     VariationsService::DoActualFetch();
   }
 
- protected:
   bool StoreSeed(const std::string& seed_data,
                  const std::string& seed_signature,
                  const std::string& country_code,
                  const base::Time& date_fetched,
                  bool is_delta_compressed) override {
     seed_stored_ = true;
+    stored_seed_data_ = seed_data;
     stored_country_ = country_code;
     return true;
   }
 
  private:
+  bool LoadSeed(VariationsSeed* seed) override {
+    if (!seed_stored_)
+      return false;
+    return seed->ParseFromString(stored_seed_data_);
+  }
+
   bool intercepts_fetch_;
   bool fetch_attempted_;
   bool seed_stored_;
+  std::string stored_seed_data_;
   std::string stored_country_;
 
   DISALLOW_COPY_AND_ASSIGN(TestVariationsService);
@@ -163,6 +171,12 @@ class TestVariationsServiceObserver : public VariationsService::Observer {
   DISALLOW_COPY_AND_ASSIGN(TestVariationsServiceObserver);
 };
 
+// Constants used to create the test seed.
+const char kTestSeedStudyName[] = "test";
+const char kTestSeedExperimentName[] = "abc";
+const int kTestSeedExperimentProbability = 100;
+const char kTestSeedSerialNumber[] = "123";
+
 // Populates |seed| with simple test data. The resulting seed will contain one
 // study called "test", which contains one experiment called "abc" with
 // probability weight 100. |seed|'s study field will be cleared before adding
@@ -170,12 +184,12 @@ class TestVariationsServiceObserver : public VariationsService::Observer {
 variations::VariationsSeed CreateTestSeed() {
   variations::VariationsSeed seed;
   variations::Study* study = seed.add_study();
-  study->set_name("test");
-  study->set_default_experiment_name("abc");
+  study->set_name(kTestSeedStudyName);
+  study->set_default_experiment_name(kTestSeedExperimentName);
   variations::Study_Experiment* experiment = study->add_experiment();
-  experiment->set_name("abc");
-  experiment->set_probability_weight(100);
-  seed.set_serial_number("123");
+  experiment->set_name(kTestSeedExperimentName);
+  experiment->set_probability_weight(kTestSeedExperimentProbability);
+  seed.set_serial_number(kTestSeedSerialNumber);
   return seed;
 }
 
@@ -220,6 +234,95 @@ class VariationsServiceTest : public ::testing::Test {
   DISALLOW_COPY_AND_ASSIGN(VariationsServiceTest);
 };
 
+TEST_F(VariationsServiceTest, CreateTrialsFromSeed) {
+  TestingPrefServiceSimple prefs;
+  VariationsService::RegisterPrefs(prefs.registry());
+
+  // Setup base::FeatureList.
+  base::FeatureList::ClearInstanceForTesting();
+  base::FeatureList::SetInstance(make_scoped_ptr(new base::FeatureList()));
+
+  // Create a local base::FieldTrialList, to hold the field trials created in
+  // this test.
+  base::FieldTrialList field_trial_list(nullptr);
+
+  // Create a variations service.
+  TestVariationsService service(
+      make_scoped_ptr(new web_resource::TestRequestAllowedNotifier(&prefs)),
+      &prefs);
+
+  // Store a seed.
+  service.StoreSeed(SerializeSeed(CreateTestSeed()), std::string(),
+                    std::string(), base::Time::Now(), false);
+  prefs.SetInt64(prefs::kVariationsLastFetchTime,
+                 base::Time::Now().ToInternalValue());
+
+  // Check that field trials are created from the seed. Since the test study has
+  // only 1 experiment with 100% probability weight, we must be part of it.
+  EXPECT_TRUE(service.CreateTrialsFromSeed(base::FeatureList::GetInstance()));
+  EXPECT_EQ(base::FieldTrialList::FindFullName(kTestSeedStudyName),
+            kTestSeedExperimentName);
+}
+
+TEST_F(VariationsServiceTest, CreateTrialsFromSeedNoLastFetchTime) {
+  TestingPrefServiceSimple prefs;
+  VariationsService::RegisterPrefs(prefs.registry());
+
+  // Setup base::FeatureList.
+  base::FeatureList::ClearInstanceForTesting();
+  base::FeatureList::SetInstance(make_scoped_ptr(new base::FeatureList()));
+
+  // Create a local base::FieldTrialList, to hold the field trials created in
+  // this test.
+  base::FieldTrialList field_trial_list(nullptr);
+
+  // Create a variations service
+  TestVariationsService service(
+      make_scoped_ptr(new web_resource::TestRequestAllowedNotifier(&prefs)),
+      &prefs);
+
+  // Store a seed. To simulate a first run, |prefs::kVariationsLastFetchTime|
+  // is left empty.
+  service.StoreSeed(SerializeSeed(CreateTestSeed()), std::string(),
+                    std::string(), base::Time::Now(), false);
+  EXPECT_EQ(0, prefs.GetInt64(prefs::kVariationsLastFetchTime));
+
+  // Check that field trials are created from the seed. Since the test study has
+  // only 1 experiment with 100% probability weight, we must be part of it.
+  EXPECT_TRUE(service.CreateTrialsFromSeed(base::FeatureList::GetInstance()));
+  EXPECT_EQ(base::FieldTrialList::FindFullName(kTestSeedStudyName),
+            kTestSeedExperimentName);
+}
+
+TEST_F(VariationsServiceTest, CreateTrialsFromOutdatedSeed) {
+  TestingPrefServiceSimple prefs;
+  VariationsService::RegisterPrefs(prefs.registry());
+
+  // Setup base::FeatureList.
+  base::FeatureList::ClearInstanceForTesting();
+  base::FeatureList::SetInstance(make_scoped_ptr(new base::FeatureList()));
+
+  // Create a local base::FieldTrialList, to hold the field trials created in
+  // this test.
+  base::FieldTrialList field_trial_list(nullptr);
+
+  // Create a variations service.
+  TestVariationsService service(
+      make_scoped_ptr(new web_resource::TestRequestAllowedNotifier(&prefs)),
+      &prefs);
+
+  // Store a seed, with a fetch time 31 days in the past.
+  const base::Time seed_date =
+      base::Time::Now() - base::TimeDelta::FromDays(31);
+  service.StoreSeed(SerializeSeed(CreateTestSeed()), std::string(),
+                    std::string(), seed_date, false);
+  prefs.SetInt64(prefs::kVariationsLastFetchTime, seed_date.ToInternalValue());
+
+  // Check that field trials are not created from the seed.
+  EXPECT_FALSE(service.CreateTrialsFromSeed(base::FeatureList::GetInstance()));
+  EXPECT_TRUE(base::FieldTrialList::FindFullName(kTestSeedStudyName).empty());
+}
+
 TEST_F(VariationsServiceTest, GetVariationsServerURL) {
   TestingPrefServiceSimple prefs;
   VariationsService::RegisterPrefs(prefs.registry());
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index 8cef2df..5c6c4ca 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -48596,6 +48596,16 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   <summary>TBD</summary>
 </histogram>
 
+<histogram name="Variations.CreateTrials.SeedExpiry"
+    enum="VariationsSeedExpiry">
+  <owner>asvitkine@chromium.org</owner>
+  <owner>fdoray@chromium.org</owner>
+  <summary>
+    The result of verifying if the variations seed is expired, recorded before
+    trials are created from the seed. Expired seeds are treated as not existing.
+  </summary>
+</histogram>
+
 <histogram name="Variations.DisabledNoEntropyProvider" enum="BooleanHit">
   <obsolete>
     Deprecated 1/2013. No longer tracked.
@@ -48723,6 +48733,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 
 <histogram name="Variations.SeedDateSkew.BuildTimeAheadBy" units="days">
   <owner>gab@chromium.org</owner>
+  <obsolete>
+    Deprecated as of 9/2015.
+  </obsolete>
   <summary>
     Logged on startup when creating field trials from the variations seed if the
     build time is ahead of or within 24 hours of the kVariationsSeedDate. Used
@@ -48733,6 +48746,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 
 <histogram name="Variations.SeedDateSkew.BuildTimeBehindBy" units="days">
   <owner>gab@chromium.org</owner>
+  <obsolete>
+    Deprecated as of 9/2015.
+  </obsolete>
   <summary>
     Logged on startup when creating field trials from the variations seed if the
     build time is behind the kVariationsSeedDate by a day or more. Used as an
@@ -48743,6 +48759,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 
 <histogram name="Variations.SeedDateSkew.SystemClockAheadBy" units="days">
   <owner>gab@chromium.org</owner>
+  <obsolete>
+    Deprecated as of 9/2015.
+  </obsolete>
   <summary>
     Logged on startup when creating field trials from the variations seed if the
     system clock is ahead of or within 24 hours of the kVariationsSeedDate. Used
@@ -48753,6 +48772,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 
 <histogram name="Variations.SeedDateSkew.SystemClockBehindBy" units="days">
   <owner>gab@chromium.org</owner>
+  <obsolete>
+    Deprecated as of 9/2015.
+  </obsolete>
   <summary>
     Logged on startup when creating field trials from the variations seed if the
     system clock is behind the kVariationsSeedDate by a day or more. Used as an
@@ -73187,6 +73209,12 @@ To add a new entry, add it with any value and run test to compute valid value.
   <int value="6" label="Seed Corrupt Gzip Data"/>
 </enum>
 
+<enum name="VariationsSeedExpiry" type="int">
+  <int value="0" label="Not Expired"/>
+  <int value="1" label="Fetch Time Missing"/>
+  <int value="2" label="Expired"/>
+</enum>
+
 <enum name="VariationsSeedStoreResult" type="int">
   <int value="0" label="Success"/>
   <int value="1" label="Failed - Empty Seed"/>