Add debug info for field trial allocation crash

base/metrics/field_trial.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/metrics/field_trial.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/base_switches.h"
@@ skipping 842 matching lines @@
       return false;
 
     // TODO(lawrencewu): Convert the API for CreateFieldTrial to take
     // StringPieces.
     FieldTrial* trial =
         CreateFieldTrial(trial_name.as_string(), group_name.as_string());
 
     // 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(),
                group_name.as_string());
       return false;
     }
 
     trial->ref_ = ref;
     if (entry->activated) {
       // Call |group()| to mark the trial as "used" and notify observers, if
@@ skipping 32 matching lines @@
   // Set |readonly_allocator_handle_| so we can pass it to be inherited and
   // via the command line.
   global_->readonly_allocator_handle_ =
       CreateReadOnlyHandle(global_->field_trial_allocator_.get());
 #endif
 }
 #endif
 
 // 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();
 
   // Don't do anything if the allocator hasn't been instantiated yet.
   if (allocator == nullptr)
     return;
 
   // Or if the allocator is read only, which means we are in a child process and
   // shouldn't be writing to it.
   if (allocator->IsReadonly())
@@ skipping 82 matching lines @@
     return;
   }
   AutoLock auto_lock(global_->lock_);
   CHECK(!global_->PreLockedFind(trial->trial_name())) << trial->trial_name();
   trial->AddRef();
   trial->SetTrialRegistered();
   global_->registered_[trial->trial_name()] = trial;
 }
 
 }  // namespace base