Fix adding duplicate trials to allocator (finally)

It looks like the reason we were crashing from shared memory was because
simulated field trials were getting added to the allocator when they
shouldn't be. This CL now checks |trial_registered_| before adding the
the trial to the allocator. Also includes a unittest for this behavior.

BUG=666230

Review-Url: https://codereview.chromium.org/2517193003
Cr-Commit-Position: refs/heads/master@{#434046}

base/metrics/field_trial.cc

@@ -407,7 +407,7 @@ void FieldTrial::FinalizeGroupChoiceImpl(bool is_locked) {
   SetGroupChoice(default_group_name_, kDefaultGroupNumber);
 
   // Add the field trial to shared memory.
-  if (kUseSharedMemoryForFieldTrials)
+  if (kUseSharedMemoryForFieldTrials && trial_registered_)
     FieldTrialList::OnGroupFinalized(is_locked, this);
 }
 

base/metrics/field_trial.h

@@ -221,6 +221,8 @@ class BASE_EXPORT FieldTrial : public RefCounted<FieldTrial> {
   FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, SetForcedChangeDefault_NonDefault);
   FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, FloatBoundariesGiveEqualGroupSizes);
   FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, DoesNotSurpassTotalProbability);
+  FRIEND_TEST_ALL_PREFIXES(FieldTrialListTest,
+                           DoNotAddSimulatedFieldTrialsToAllocator);
 
   friend class base::FieldTrialList;
 
@@ -552,6 +554,8 @@ class BASE_EXPORT FieldTrialList {
   // Allow tests to access our innards for testing purposes.
   FRIEND_TEST_ALL_PREFIXES(FieldTrialListTest, InstantiateAllocator);
   FRIEND_TEST_ALL_PREFIXES(FieldTrialListTest, AddTrialsToAllocator);
+  FRIEND_TEST_ALL_PREFIXES(FieldTrialListTest,
+                           DoNotAddSimulatedFieldTrialsToAllocator);
 
 #if defined(OS_WIN)
   // Takes in |handle| that should have been retrieved from the command line and

base/metrics/field_trial_unittest.cc

@@ -1187,11 +1187,47 @@ TEST(FieldTrialListTest, AddTrialsToAllocator) {
 
   FieldTrialList field_trial_list2(nullptr);
   std::unique_ptr<base::SharedMemory> shm(new SharedMemory(handle, true));
-  shm.get()->Map(4 << 10);  // Hardcoded, equal to kFieldTrialAllocationSize.
+  // 4 KiB is enough to hold the trials only created for this test.
+  shm.get()->Map(4 << 10);
   FieldTrialList::CreateTrialsFromSharedMemory(std::move(shm));
   std::string check_string;
   FieldTrialList::AllStatesToString(&check_string);
   EXPECT_EQ(save_string, check_string);
 }
 
+TEST(FieldTrialListTest, DoNotAddSimulatedFieldTrialsToAllocator) {
+  constexpr char kTrialName[] = "trial";
+  base::SharedMemoryHandle handle;
+  {
+    // Create a simulated trial and a real trial and call group() on them, which
+    // should only add the real trial to the field trial allocator.
+    FieldTrialList field_trial_list(nullptr);
+    FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded();
+
+    // This shouldn't add to the allocator.
+    scoped_refptr<FieldTrial> simulated_trial =
+        FieldTrial::CreateSimulatedFieldTrial(kTrialName, 100, "Simulated",
+                                              0.95);
+    simulated_trial->group();
+
+    // This should add to the allocator.
+    FieldTrial* real_trial =
+        FieldTrialList::CreateFieldTrial(kTrialName, "Real");
+    real_trial->group();
+
+    handle = base::SharedMemory::DuplicateHandle(
+        field_trial_list.field_trial_allocator_->shared_memory()->handle());
+  }
+
+  // Check that there's only one entry in the allocator.
+  FieldTrialList field_trial_list2(nullptr);
+  std::unique_ptr<base::SharedMemory> shm(new SharedMemory(handle, true));
+  // 4 KiB is enough to hold the trials only created for this test.
+  shm.get()->Map(4 << 10);
+  FieldTrialList::CreateTrialsFromSharedMemory(std::move(shm));
+  std::string check_string;
+  FieldTrialList::AllStatesToString(&check_string);
+  ASSERT_EQ(check_string.find("Simulated"), std::string::npos);
+}
+
 }  // namespace base