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 bb6ae0cb83fd02fa52b8d17584ee911fcef105ca..6ecdd058715ae3216da7e65d444a8182389171f9 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -334,6 +334,10 @@ void FieldTrial::SetGroupChoice(const std::string& group_name, int number) {
 }
 
 void FieldTrial::FinalizeGroupChoice() {
+  FinalizeGroupChoiceImpl(false);
+}
+
+void FieldTrial::FinalizeGroupChoiceImpl(bool is_locked) {
   if (group_ != kNotFinalized)
     return;
   accumulated_group_probability_ = divisor_;
@@ -344,7 +348,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 {
@@ -366,6 +370,16 @@ bool FieldTrial::GetState(State* field_trial_state) {
   return true;
 }
 
+bool FieldTrial::GetStateWhileLocked(State* field_trial_state) {
+  if (!enable_field_trial_)
+    return false;
+  FinalizeGroupChoiceImpl(true);
+  field_trial_state->trial_name = trial_name_;
+  field_trial_state->group_name = group_name_;
+  field_trial_state->activated = group_reported_;
+  return true;
+}
+
 //------------------------------------------------------------------------------
 // FieldTrialList methods and members.
 
@@ -546,7 +560,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));
@@ -671,7 +685,7 @@ void FieldTrialList::AppendFieldTrialHandleIfNeeded(
     return;
   if (kUseSharedMemoryForFieldTrials) {
     InstantiateFieldTrialAllocatorIfNeeded();
-    if (global_->readonly_allocator_handle_)
+    if (global_ && global_->readonly_allocator_handle_)

[Alexei Svitkine (slow), 2016/10/28 18:57:34]

There's already an early return on line 684.

[lawrencewu, 2016/10/31 18:59:25]

Fixed.

       handles->push_back(global_->readonly_allocator_handle_);
   }
 }
@@ -688,7 +702,7 @@ void FieldTrialList::CopyFieldTrialStateToFlags(
     InstantiateFieldTrialAllocatorIfNeeded();
     // If the readonly handle didn't get duplicated properly, then fallback to
     // original behavior.
-    if (!global_->readonly_allocator_handle_) {
+    if (!global_ || !global_->readonly_allocator_handle_) {

[Alexei Svitkine (slow), 2016/10/28 18:57:34]

Can we instead fix the browser test to have a FieldTrialList instance instead of
adding this check?

[lawrencewu, 2016/10/31 18:59:25]

As we discussed, I moved the check for global to the top and wrote a comment
about this.

       AddForceFieldTrialsFlag(cmd_line);
       return;
     }
@@ -755,11 +769,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