Add debug info for field trial allocation crash

base/metrics/field_trial.cc

Index: base/metrics/field_trial.cc
diff --git a/base/metrics/field_trial.cc b/base/metrics/field_trial.cc
index 970441160ca4afe2acb501095c8e81716e3c45ce..120ebd8537b618c8298523a300da6bfeacdc96b6 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -181,6 +181,49 @@ void AddForceFieldTrialsFlag(CommandLine* cmd_line) {
   }
 }
 
+// In order to debug the crash where two field trials with the same name but
+// different groups get added to the allocator, let's try to get the stack
+// frame where the second duplicate gets added here as well as the trial/group
+// names by going through the allocator and checking if a trial with the same
+// name already exists.
+void CheckAllocatorForTrial(FieldTrialList::FieldTrialAllocator* allocator,
+                            const std::string& trial_name,
+                            const std::string& group_name) {
+  FieldTrialList::FieldTrialAllocator::Iterator iter(allocator);
+  FieldTrial::FieldTrialRef ref;
+  while ((ref = iter.GetNextOfType(kFieldTrialType)) !=
+         FieldTrialList::FieldTrialAllocator::kReferenceNull) {
+    const FieldTrialEntry* entry =
+        allocator->GetAsObject<const FieldTrialEntry>(ref, kFieldTrialType);
+
+    StringPiece trial_name_to_check;
+    StringPiece group_name_to_check;
+    if (!entry->GetTrialAndGroupName(&trial_name_to_check,
+                                     &group_name_to_check)) {
+      CHECK(false);
+    }
+
+    std::string existing_trial_name = trial_name_to_check.as_string();

[brucedawson, 2016/11/21 22:07:00]

Why copy these to std::string objects? This requires allocating memory and I'm
not clear what it adds - why not strlcpy from the StringPiece objects?

[lawrencewu, 2016/11/22 15:17:54]

I think it's more readable this way, but yeah, since this method will be called
often we should probably focus more on resource usage. Fixed.

[brucedawson, 2016/11/22 17:55:24]

Thanks. Given the separate thread going on about the importance of reducing
allocation churn it seems particularly important to avoid unnecessary
allocations.

+    std::string existing_group_name = group_name_to_check.as_string();
+    if (existing_trial_name == trial_name) {
+      constexpr size_t buf_size = 100;
+
+      char trial_name_c_str[buf_size];
+      char group_name_c_str[buf_size];
+      char existing_group_name_c_str[buf_size];
+
+      strlcpy(trial_name_c_str, trial_name.c_str(), buf_size);
+      strlcpy(group_name_c_str, group_name.c_str(), buf_size);
+      strlcpy(existing_group_name_c_str, existing_group_name.c_str(), buf_size);
+
+      debug::Alias(trial_name_c_str);
+      debug::Alias(existing_group_name_c_str);
+      debug::Alias(group_name_c_str);

[brucedawson, 2016/11/21 22:07:00]

For consistency and to minimize confusion keep the same order in the
debug::Alias calls as for the declarations and strlcpy calls (i.e.; move
group_name_c_str up one).

[lawrencewu, 2016/11/22 15:17:54]

Done.

+      CHECK_EQ(existing_group_name, group_name);
+    }
+  }
+}
+
 #if defined(OS_WIN)
 HANDLE CreateReadOnlyHandle(FieldTrialList::FieldTrialAllocator* allocator) {
   HANDLE src = allocator->shared_memory()->handle().GetHandle();
@@ -857,14 +900,30 @@ bool FieldTrialList::CreateTrialsFromSharedMemory(
     // If we failed to create the field trial, crash with debug info.
     // TODO(665129): Remove this when the crash is resolved.
     if (!trial) {
+      constexpr size_t buf_size = 100;
+      char trial_name_c_str[buf_size] = {0};
+      char group_name_c_str[buf_size] = {0};
+      char existing_group_name_c_str[buf_size] = {0};
+
+      // Copy the names over to the stack.
       std::string trial_name_string = trial_name.as_string();
+      strlcpy(trial_name_c_str, trial_name_string.c_str(), buf_size);
       std::string group_name_string = group_name.as_string();
+      strlcpy(group_name_c_str, group_name_string.c_str(), buf_size);
+
+      // Alias the trial and group name.
+      debug::Alias(trial_name_c_str);
+      debug::Alias(group_name_c_str);
+
+      // Copy and alias the existing field trial name, if there is one.
       FieldTrial* existing_field_trial =
           FieldTrialList::Find(trial_name_string);
-      if (existing_field_trial)
-        debug::Alias(existing_field_trial->group_name_internal().c_str());
-      debug::Alias(trial_name_string.c_str());
-      debug::Alias(group_name_string.c_str());
+      if (existing_field_trial) {
+        std::string existing_group_name_string =
+            existing_field_trial->group_name_internal();
+        strlcpy(existing_group_name_c_str, group_name_string.c_str(), buf_size);
+        debug::Alias(existing_group_name_c_str);
+      }
       CHECK(!trial_name_string.empty());
       CHECK(!group_name_string.empty());
       CHECK_EQ(existing_field_trial->group_name_internal(),
@@ -928,6 +987,10 @@ void FieldTrialList::AddToAllocatorWhileLocked(FieldTrial* field_trial) {
   if (allocator->IsReadonly())
     return;
 
+  // TODO(665129): Remove this code once the bug has been fixed.
+  CheckAllocatorForTrial(allocator, field_trial->trial_name(),
+                         field_trial->group_name_internal());
+
   FieldTrial::State trial_state;
   if (!field_trial->GetStateWhileLocked(&trial_state))
     return;