Fix deadlock issue with AllStatesToString

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 9f8a21265550f311b24f4ad1084173f843e7e70c..8b55fe15aeaacadc80380f5dcd5f6a92cb992cc2 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -36,7 +36,7 @@ const char kActivationMarker = '*';
 // for now while the implementation is fleshed out (e.g. data format, single
 // shared memory segment). See https://codereview.chromium.org/2365273004/ and
 // crbug.com/653874
-const bool kUseSharedMemoryForFieldTrials = false;
+const bool kUseSharedMemoryForFieldTrials = true;
 
 // Constants for the field trial allocator.
 const char kAllocatorName[] = "FieldTrialAllocator";
@@ -250,7 +250,7 @@ int FieldTrial::AppendGroup(const std::string& name,
 }
 
 int FieldTrial::group() {
-  FinalizeGroupChoice();
+  FinalizeGroupChoice(false);
   if (trial_registered_)
     FieldTrialList::NotifyFieldTrialGroupSelection(this);
   return group_;
@@ -264,7 +264,7 @@ const std::string& FieldTrial::group_name() {
 }
 
 const std::string& FieldTrial::GetGroupNameWithoutActivation() {
-  FinalizeGroupChoice();
+  FinalizeGroupChoice(false);
   return group_name_;
 }
 
@@ -275,7 +275,7 @@ void FieldTrial::SetForced() {
     return;
 
   // And we must finalize the group choice before we mark ourselves as forced.
-  FinalizeGroupChoice();
+  FinalizeGroupChoice(false);
   forced_ = true;
 }
 
@@ -333,7 +333,7 @@ void FieldTrial::SetGroupChoice(const std::string& group_name, int number) {
   DVLOG(1) << "Field trial: " << trial_name_ << " Group choice:" << group_name_;
 }
 
-void FieldTrial::FinalizeGroupChoice() {
+void FieldTrial::FinalizeGroupChoice(bool is_locked) {
   if (group_ != kNotFinalized)
     return;
   accumulated_group_probability_ = divisor_;
@@ -344,7 +344,7 @@ void FieldTrial::FinalizeGroupChoice() {
 
   // Add the field trial to shared memory.
   if (kUseSharedMemoryForFieldTrials)
-    FieldTrialList::OnGroupFinalized(this);
+    FieldTrialList::OnGroupFinalized(is_locked, this);
 }
 
 bool FieldTrial::GetActiveGroup(ActiveGroup* active_group) const {
@@ -359,7 +359,17 @@ bool FieldTrial::GetActiveGroup(ActiveGroup* active_group) const {
 bool FieldTrial::GetState(State* field_trial_state) {
   if (!enable_field_trial_)
     return false;
-  FinalizeGroupChoice();
+  FinalizeGroupChoice(false);
+  field_trial_state->trial_name = trial_name_;
+  field_trial_state->group_name = group_name_;
+  field_trial_state->activated = group_reported_;
+  return true;
+}
+
+bool FieldTrial::GetStateWhileLocked(State* field_trial_state) {
+  if (!enable_field_trial_)
+    return false;
+  FinalizeGroupChoice(true);

[Alexei Svitkine (slow), 2016/10/27 21:40:43]

How about making the call that takes the param FinalizeGroupChoiceImpl() and
keeping FinalizeGroupChoice() take no param?

Then, you can make FinalizeGroupChoice() call FinalizeGroupChoiceImpl(false) so
you don't have to change all the callers.

[lawrencewu, 2016/10/28 17:20:21]

Done.

   field_trial_state->trial_name = trial_name_;
   field_trial_state->group_name = group_name_;
   field_trial_state->activated = group_reported_;
@@ -546,7 +556,7 @@ void FieldTrialList::AllStatesToString(std::string* output) {
 
   for (const auto& registered : global_->registered_) {
     FieldTrial::State trial;
-    if (!registered.second->GetState(&trial))
+    if (!registered.second->GetStateWhileLocked(&trial))
       continue;
     DCHECK_EQ(std::string::npos,
               trial.trial_name.find(kPersistentStringSeparator));
@@ -753,11 +763,15 @@ void FieldTrialList::RemoveObserver(Observer* observer) {
 }
 
 // static
-void FieldTrialList::OnGroupFinalized(FieldTrial* field_trial) {
+void FieldTrialList::OnGroupFinalized(bool is_locked, FieldTrial* field_trial) {
   if (!global_)
     return;
-  AutoLock auto_lock(global_->lock_);
-  AddToAllocatorWhileLocked(field_trial);
+  if (is_locked) {
+    AddToAllocatorWhileLocked(field_trial);
+  } else {
+    AutoLock auto_lock(global_->lock_);
+    AddToAllocatorWhileLocked(field_trial);
+  }
 }
 
 // static