Fix issue with identical group numbers being reported for forced field trials.

This fixes the following case, which was a problem with the old code:

// Trial gets forced: (e.g. from server config)
FieldTrial* forced_trial = FieldTrialList::FactoryGetFieldTrial(...);
forced_trial->AppendGroup(...);
forced_trial->SetForced();

// Client code that makes a trial:
int default_group;
FieldTrial* client_trial = FieldTrialList::FactoryGetFieldTrial(..., &default_group);
const int extra_group = client_trial->AppendGroup(...);

// Expected extra_group != default_group
// Actual: they are equal! (value 1)

Also, fixes an issue where such a trial may get marked as its group
having been reported, due to a group() call in FactoryGetFieldTrial().

Adds unit tests for the above fixes.

BUG=180008
TEST=New unit tests.

Review URL: https://chromiumcodereview.appspot.com/12393055

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

3 files changed, 143 insertions, 10 deletions

diff --git a/base/metrics/field_trial.cc b/base/metrics/field_trial.cc
index aa17dd9..4ab5ff9 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -114,6 +114,10 @@ int FieldTrial::AppendGroup(const std::string& name,
   if (forced_) {
     DCHECK(!group_name_.empty());
     if (name == group_name_) {
+      // Note that while |group_| may be equal to |kDefaultGroupNumber| on the
+      // forced trial, it will not have the same value as the default group
+      // number returned from the non-forced |FactoryGetFieldTrial()| call,
+      // which takes care to ensure that this does not happen.
       return group_;
     }
     DCHECK_NE(next_group_number_, group_);
@@ -259,11 +263,28 @@ FieldTrial* FieldTrialList::FactoryGetFieldTrial(
   FieldTrial* existing_trial = Find(name);
   if (existing_trial) {
     CHECK(existing_trial->forced_);
-    // If the field trial has already been forced, check whether it was forced
-    // to the default group. Return the chosen group number, in that case..
+    // If the default group name differs between the existing forced trial
+    // and this trial, then use a different value for the default group number.
     if (default_group_number &&
-        default_group_name == existing_trial->default_group_name()) {
-      *default_group_number = existing_trial->group();
+        default_group_name != existing_trial->default_group_name()) {
+      // If the new default group number corresponds to the group that was
+      // chosen for the forced trial (which has been finalized when it was
+      // forced), then set the default group number to that.
+      if (default_group_name == existing_trial->group_name_internal()) {
+        *default_group_number = existing_trial->group_;
+      } else {
+        // Otherwise, use |kNonConflictingGroupNumber| (-2) for the default
+        // group number, so that it does not conflict with the |AppendGroup()|
+        // result for the chosen group.
+        const int kNonConflictingGroupNumber = -2;
+        COMPILE_ASSERT(
+            kNonConflictingGroupNumber != FieldTrial::kDefaultGroupNumber,
+            conflicting_default_group_number);
+        COMPILE_ASSERT(
+            kNonConflictingGroupNumber != FieldTrial::kNotFinalized,
+            conflicting_default_group_number);
+        *default_group_number = kNonConflictingGroupNumber;
+      }
     }
     return existing_trial;
   }
@@ -389,10 +410,8 @@ FieldTrial* FieldTrialList::CreateFieldTrial(
   }
   const int kTotalProbability = 100;
   field_trial = new FieldTrial(name, kTotalProbability, group_name);
-  // This is where we may assign a group number different from
-  // kDefaultGroupNumber to the default group.
-  field_trial->AppendGroup(group_name, kTotalProbability);
-  field_trial->forced_ = true;
+  // Force the trial, which will also finalize the group choice.
+  field_trial->SetForced();
   FieldTrialList::Register(field_trial);
   return field_trial;
 }
diff --git a/base/metrics/field_trial.h b/base/metrics/field_trial.h
index 80f4601..a4e1f53 100644
--- a/base/metrics/field_trial.h
+++ b/base/metrics/field_trial.h
@@ -189,6 +189,10 @@ class BASE_EXPORT FieldTrial : public RefCounted<FieldTrial> {
   FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, HashClientIdIsUniform);
   FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, NameGroupIds);
   FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, UseOneTimeRandomization);
+  FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, SetForcedTurnFeatureOff);
+  FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, SetForcedTurnFeatureOn);
+  FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, SetForcedChangeDefault_Default);
+  FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, SetForcedChangeDefault_NonDefault);
 
   friend class base::FieldTrialList;
 
diff --git a/base/metrics/field_trial_unittest.cc b/base/metrics/field_trial_unittest.cc
index cb60f33..81c4f37 100644
--- a/base/metrics/field_trial_unittest.cc
+++ b/base/metrics/field_trial_unittest.cc
@@ -542,10 +542,14 @@ TEST_F(FieldTrialTest, ForcedFieldTrials) {
   EXPECT_NE(chosen_group, new_group);
   // The new group should not be the default group either.
   EXPECT_NE(default_group_number, new_group);
+}
 
+TEST_F(FieldTrialTest, ForcedFieldTrialsDefaultGroup) {
   // Forcing the default should use the proper group ID.
-  forced_trial = FieldTrialList::CreateFieldTrial("Trial Name", "Default");
-  factory_trial = FieldTrialList::FactoryGetFieldTrial(
+  FieldTrial* forced_trial = FieldTrialList::CreateFieldTrial("Trial Name",
+                                                              "Default");
+  int default_group_number = -1;
+  FieldTrial* factory_trial = FieldTrialList::FactoryGetFieldTrial(
       "Trial Name", 1000, "Default", next_year_, 12, 31, &default_group_number);
   EXPECT_EQ(forced_trial, factory_trial);
 
@@ -626,6 +630,112 @@ TEST_F(FieldTrialTest, SetForcedDefaultWithExtraGroup) {
   EXPECT_EQ(kDefaultGroupName, trial->group_name());
 }
 
+TEST_F(FieldTrialTest, SetForcedTurnFeatureOn) {
+  const char kTrialName[] = "SetForcedTurnFeatureOn";
+  const char kExtraGroupName[] = "Extra";
+  ASSERT_FALSE(FieldTrialList::TrialExists(kTrialName));
+
+  // Simulate a server-side (forced) config that turns the feature on when the
+  // original hard-coded config had it disabled.
+  FieldTrial* forced_trial =
+      FieldTrialList::FactoryGetFieldTrial(kTrialName, 100, kDefaultGroupName,
+                                           next_year_, 12, 31, NULL);
+  forced_trial->AppendGroup(kExtraGroupName, 100);
+  forced_trial->SetForced();
+
+  int default_group = -1;
+  FieldTrial* client_trial =
+      FieldTrialList::FactoryGetFieldTrial(kTrialName, 100, kDefaultGroupName,
+                                           next_year_, 12, 31, &default_group);
+  const int extra_group = client_trial->AppendGroup(kExtraGroupName, 0);
+  EXPECT_NE(default_group, extra_group);
+
+  EXPECT_FALSE(client_trial->group_reported_);
+  EXPECT_EQ(extra_group, client_trial->group());
+  EXPECT_TRUE(client_trial->group_reported_);
+  EXPECT_EQ(kExtraGroupName, client_trial->group_name());
+}
+
+TEST_F(FieldTrialTest, SetForcedTurnFeatureOff) {
+  const char kTrialName[] = "SetForcedTurnFeatureOff";
+  const char kExtraGroupName[] = "Extra";
+  ASSERT_FALSE(FieldTrialList::TrialExists(kTrialName));
+
+  // Simulate a server-side (forced) config that turns the feature off when the
+  // original hard-coded config had it enabled.
+  FieldTrial* forced_trial =
+      FieldTrialList::FactoryGetFieldTrial(kTrialName, 100, kDefaultGroupName,
+                                           next_year_, 12, 31, NULL);
+  forced_trial->AppendGroup(kExtraGroupName, 0);
+  forced_trial->SetForced();
+
+  int default_group = -1;
+  FieldTrial* client_trial =
+      FieldTrialList::FactoryGetFieldTrial(kTrialName, 100, kDefaultGroupName,
+                                           next_year_, 12, 31, &default_group);
+  const int extra_group = client_trial->AppendGroup(kExtraGroupName, 100);
+  EXPECT_NE(default_group, extra_group);
+
+  EXPECT_FALSE(client_trial->group_reported_);
+  EXPECT_EQ(default_group, client_trial->group());
+  EXPECT_TRUE(client_trial->group_reported_);
+  EXPECT_EQ(kDefaultGroupName, client_trial->group_name());
+}
+
+TEST_F(FieldTrialTest, SetForcedChangeDefault_Default) {
+  const char kTrialName[] = "SetForcedDefaultGroupChange";
+  const char kGroupAName[] = "A";
+  const char kGroupBName[] = "B";
+  ASSERT_FALSE(FieldTrialList::TrialExists(kTrialName));
+
+  // Simulate a server-side (forced) config that switches which group is default
+  // and ensures that the non-forced code receives the correct group numbers.
+  FieldTrial* forced_trial =
+      FieldTrialList::FactoryGetFieldTrial(kTrialName, 100, kGroupAName,
+                                           next_year_, 12, 31, NULL);
+  forced_trial->AppendGroup(kGroupBName, 100);
+  forced_trial->SetForced();
+
+  int default_group = -1;
+  FieldTrial* client_trial =
+      FieldTrialList::FactoryGetFieldTrial(kTrialName, 100, kGroupBName,
+                                           next_year_, 12, 31, &default_group);
+  const int extra_group = client_trial->AppendGroup(kGroupAName, 50);
+  EXPECT_NE(default_group, extra_group);
+
+  EXPECT_FALSE(client_trial->group_reported_);
+  EXPECT_EQ(default_group, client_trial->group());
+  EXPECT_TRUE(client_trial->group_reported_);
+  EXPECT_EQ(kGroupBName, client_trial->group_name());
+}
+
+TEST_F(FieldTrialTest, SetForcedChangeDefault_NonDefault) {
+  const char kTrialName[] = "SetForcedDefaultGroupChange";
+  const char kGroupAName[] = "A";
+  const char kGroupBName[] = "B";
+  ASSERT_FALSE(FieldTrialList::TrialExists(kTrialName));
+
+  // Simulate a server-side (forced) config that switches which group is default
+  // and ensures that the non-forced code receives the correct group numbers.
+  FieldTrial* forced_trial =
+      FieldTrialList::FactoryGetFieldTrial(kTrialName, 100, kGroupAName,
+                                           next_year_, 12, 31, NULL);
+  forced_trial->AppendGroup(kGroupBName, 0);
+  forced_trial->SetForced();
+
+  int default_group = -1;
+  FieldTrial* client_trial =
+      FieldTrialList::FactoryGetFieldTrial(kTrialName, 100, kGroupBName,
+                                           next_year_, 12, 31, &default_group);
+  const int extra_group = client_trial->AppendGroup(kGroupAName, 50);
+  EXPECT_NE(default_group, extra_group);
+
+  EXPECT_FALSE(client_trial->group_reported_);
+  EXPECT_EQ(extra_group, client_trial->group());
+  EXPECT_TRUE(client_trial->group_reported_);
+  EXPECT_EQ(kGroupAName, client_trial->group_name());
+}
+
 TEST_F(FieldTrialTest, Observe) {
   const char kTrialName[] = "TrialToObserve1";
   const char kSecondaryGroupName[] = "SecondaryGroup";