Fix crash in variations simulation code.

Crash was cauded by a null experiment, which is something we handled
elsewhere but not in this particular code. Also adds a unit test is
able to trigger the crash before the fix, although I'm not very
convinced this is the exact edge case from the wild (but either
way the fix should apply to both).

Also adds some comments about some code that was a bit confusing.
One is about using the previous group for kill switch type and
another about the threading when simulation.

Bug: 837540
Change-Id: If93f4658cd18019f3f40c82ec3172d10f61356f3
Reviewed-on: https://chromium-review.googlesource.com/1036104Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556115}

components/variations/service/variations_service.cc

@@ -437,6 +437,10 @@ bool VariationsService::StoreSeed(const std::string& seed_data,
 
   RecordSuccessfulFetch();
 
+  // Now, do simulation to determine if there are any kill-switches that were
+  // activated by this seed. To do this, first get the Chrome version to do a
+  // simulation with, which must be done on a background thread, and then do the
+  // actual simulation on the UI thread.
   base::PostTaskWithTraitsAndReplyWithResult(
       FROM_HERE, {base::MayBlock(), base::TaskPriority::BACKGROUND},
       client_->GetVersionForSimulationCallback(),

components/variations/variations_seed_processor_unittest.cc

@@ -936,6 +936,7 @@ TEST_F(VariationsSeedProcessorTest, ExpiredStudy_NoDefaultGroup) {
   auto* exp1 = AddExperiment("A", 1, &study);
   exp1->mutable_feature_association()->add_enable_feature(kFeature.name);
 
+  EXPECT_FALSE(study.has_default_experiment_name());
   EXPECT_TRUE(CreateTrialFromStudy(study));
   EXPECT_EQ("VariationsDefaultExperiment",
             base::FieldTrialList::FindFullName("Study1"));

components/variations/variations_seed_simulator.cc

@@ -209,6 +209,9 @@ VariationsSeedSimulator::PermanentStudyGroupChanged(
 
   const std::string simulated_group =
       SimulateGroupAssignment(entropy_provider, processed_study);
+
+  // Note: The current (i.e. old) group is checked for the type since that group
+  // is the one that should be annotated with the type when killing it.
   const Study_Experiment* experiment = FindExperiment(study, selected_group);
   if (simulated_group != selected_group) {
     if (experiment)
@@ -216,10 +219,12 @@ VariationsSeedSimulator::PermanentStudyGroupChanged(
     return CHANGED;
   }
 
-  // Current group exists in the study - check whether its params changed.
-  DCHECK(experiment);
-  if (!VariationParamsAreEqual(study, *experiment))
+  // If the group is unchanged, check whether its params may have changed.
+  if (experiment && !VariationParamsAreEqual(study, *experiment))
     return ConvertExperimentTypeToChangeType(experiment->type());
+
+  // Since the group name has not changed and params are either equal or the
+  // experiment was not found (and thus there are none), return NO_CHANGE.
   return NO_CHANGE;
 }
 

components/variations/variations_seed_simulator_unittest.cc

@@ -19,6 +19,12 @@
 namespace variations {
 
 namespace {
+
+// Converts |time| to Study proto format.
+int64_t TimeToProtoTime(const base::Time& time) {
+  return (time - base::Time::UnixEpoch()).InSeconds();
+}
+
 // Creates and activates a single-group field trial with name |trial_name| and
 // group |group_name| and variations |params| (if not null).
 void CreateTrial(const std::string& trial_name,
@@ -156,6 +162,9 @@ TEST_F(VariationsSeedSimulatorTest, PermanentGroupChange) {
 
   // Changing "C" group type should not affect the type of change. (Since the
   // type is evaluated for the "old" group.)
+  //
+  // Note: The current (i.e. old) group is checked for the type since that group
+  // is the one that should be annotated with the type when killing it.
   experiment->set_type(Study_Experiment_Type_NORMAL);
   EXPECT_EQ("1 0 0", SimulateStudyDifferences(&study));
   experiment->set_type(Study_Experiment_Type_IGNORE_CHANGE);
@@ -379,4 +388,28 @@ TEST_F(VariationsSeedSimulatorTest, ParamsAdded) {
   EXPECT_EQ("0 0 1", SimulateStudyDifferences(&study));
 }
 
+// Tests that simulating an expired trial without a default group doesn't crash.
+// This is very much an edge case which should generally not be encountered due
+// to server-side, but we should ensure that it still doesn't cause a client
+// side crash.
+TEST_F(VariationsSeedSimulatorTest, NoDefaultGroup) {
+  static struct base::Feature kFeature {
+    "FeatureName", base::FEATURE_ENABLED_BY_DEFAULT
+  };
+  CreateTrial("Study1", "VariationsDefaultExperiment", nullptr);
+
+  Study study;
+  study.set_consistency(Study::PERMANENT);
+  study.set_name("Study1");
+  const base::Time year_ago =
+      base::Time::Now() - base::TimeDelta::FromDays(365);
+  study.set_expiry_date(TimeToProtoTime(year_ago));
+  auto* exp1 = AddExperiment("A", 1, &study);
+  study.clear_default_experiment_name();
+  exp1->mutable_feature_association()->add_enable_feature(kFeature.name);
+
+  EXPECT_FALSE(study.has_default_experiment_name());
+  EXPECT_EQ("0 0 0", SimulateStudyDifferencesExpired(&study));
+}
+
 }  // namespace variations