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 c3c353dd67c0f01d7a8b61cbe68a6b2af6cd9099..6df0d79995987dee5756862e8db7967b1502af0c 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -860,14 +860,26 @@ 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) {
+      const size_t buf_size = 100;

[Alexei Svitkine (slow), 2016/11/21 16:26:33]

Nit: constexpr

[lawrencewu, 2016/11/21 20:08:15]

Done.

       std::string trial_name_string = trial_name.as_string();
+      char trial_name_c_str[buf_size];

[Alexei Svitkine (slow), 2016/11/21 16:26:33]

= {0};

On all of these.

Actually, you only really need it for existing_group_name_c_str once you move it
up, since it may not always be initialized - but suggest doing it for all.

[lawrencewu, 2016/11/21 20:08:15]

Done.

+      strncpy(trial_name_c_str, trial_name_string.c_str(), buf_size);

[Alexei Svitkine (slow), 2016/11/21 16:26:32]

Use strlcpy() instead.

strncpy will not terminate the string if it takes the full size.

[lawrencewu, 2016/11/21 20:08:15]

Done.

+
       std::string group_name_string = group_name.as_string();
+      char group_name_c_str[buf_size];

[Alexei Svitkine (slow), 2016/11/21 16:26:32]

Nit: Suggest moving all the buffer declarations to the start of the {} block
together and then you can debug::Alias() them all.

[lawrencewu, 2016/11/21 20:08:15]

Done.

+      strncpy(group_name_c_str, group_name_string.c_str(), buf_size);
+
       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();
+        char existing_group_name_c_str[buf_size];

[Alexei Svitkine (slow), 2016/11/21 16:26:32]

This will go out of scope when we hit the crash and therefor may not be valid
anymore. Move it up to the outer scope.

[lawrencewu, 2016/11/21 20:08:15]

Done.

+        strncpy(existing_group_name_c_str, group_name_string.c_str(), buf_size);
+        debug::Alias(existing_group_name_c_str);
+      }
+      debug::Alias(trial_name_c_str);
+      debug::Alias(group_name_c_str);
       CHECK(!trial_name_string.empty());
       CHECK(!group_name_string.empty());
       CHECK_EQ(existing_field_trial->group_name_internal(),
@@ -920,6 +932,34 @@ void FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded() {
 
 // static
 void FieldTrialList::AddToAllocatorWhileLocked(FieldTrial* field_trial) {
+  // 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.
+  // TODO(665129): Remove this code once the bug has been fixed.
+  FieldTrial* possible_duplicate =
+      global_->PreLockedFind(field_trial->trial_name());

[Alexei Svitkine (slow), 2016/11/21 16:26:32]

Wouldn't this find the same |field_trial| object as being passed in?

In what cases where you thinking this would find a different one? I guess if
|field_trial| is not registered in the global_? (If so, maybe that's what cases
your crash and the fix would be to only add it to the map if it's registered.)

(My originally suggestion for this check was to actually iterate through the
allocator objects.)

[lawrencewu, 2016/11/21 20:08:15]

Looks like that was what was causing the crash after all. (Hopefully). I still
wrote a check here going through the allocator, though, in case that doesn't
solve our problem.

+  if (possible_duplicate) {
+    const size_t buf_size = 100;
+
+    std::string trial_name = field_trial->trial_name();
+    char trial_name_c_str[buf_size];
+    strncpy(trial_name_c_str, trial_name.c_str(), buf_size);
+    debug::Alias(trial_name_c_str);
+
+    std::string existing_group_name = possible_duplicate->group_name_;
+    char existing_group_name_c_str[buf_size];
+    strncpy(existing_group_name_c_str, existing_group_name.c_str(), buf_size);
+    debug::Alias(existing_group_name_c_str);
+
+    std::string group_name = field_trial->group_name_;
+    char group_name_c_str[buf_size];
+    strncpy(group_name_c_str, group_name.c_str(), buf_size);
+    debug::Alias(group_name_c_str);
+
+    CHECK_EQ(existing_group_name, group_name);
+  }
+
   SharedPersistentMemoryAllocator* allocator =
       global_->field_trial_allocator_.get();