Fix deadlock issue with AllStatesToString

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 18 matching lines @@
 const char kPersistentStringSeparator = '/';  // Currently a slash.
 
 // Define a marker character to be used as a prefix to a trial name on the
 // command line which forces its activation.
 const char kActivationMarker = '*';
 
 // Use shared memory to communicate field trial (experiment) state. Set to false
 // 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";
 const uint32_t kFieldTrialType = 0xABA17E13 + 1;  // SHA1(FieldTrialEntry) v1
 #if !defined(OS_NACL)
 const size_t kFieldTrialAllocationSize = 4 << 10;  // 4 KiB = one page
 #endif
 
 // We create one FieldTrialEntry struct per field trial in shared memory (via
 // field_trial_allocator_). It contains whether or not it's activated, and the
@@ skipping 193 matching lines @@
 
   DCHECK_LE(accumulated_group_probability_, divisor_);
   if (group_ == kNotFinalized && accumulated_group_probability_ > random_) {
     // This is the group that crossed the random line, so we do the assignment.
     SetGroupChoice(name, next_group_number_);
   }
   return next_group_number_++;
 }
 
 int FieldTrial::group() {
-  FinalizeGroupChoice();
+  FinalizeGroupChoice(false);
   if (trial_registered_)
     FieldTrialList::NotifyFieldTrialGroupSelection(this);
   return group_;
 }
 
 const std::string& FieldTrial::group_name() {
   // Call |group()| to ensure group gets assigned and observers are notified.
   group();
   DCHECK(!group_name_.empty());
   return group_name_;
 }
 
 const std::string& FieldTrial::GetGroupNameWithoutActivation() {
-  FinalizeGroupChoice();
+  FinalizeGroupChoice(false);
   return group_name_;
 }
 
 void FieldTrial::SetForced() {
   // We might have been forced before (e.g., by CreateFieldTrial) and it's
   // first come first served, e.g., command line switch has precedence.
   if (forced_)
     return;
 
   // And we must finalize the group choice before we mark ourselves as forced.
-  FinalizeGroupChoice();
+  FinalizeGroupChoice(false);
   forced_ = true;
 }
 
 // static
 void FieldTrial::EnableBenchmarking() {
   DCHECK_EQ(0u, FieldTrialList::GetFieldTrialCount());
   enable_benchmarking_ = true;
 }
 
 // static
@@ skipping 37 matching lines @@
 
 void FieldTrial::SetGroupChoice(const std::string& group_name, int number) {
   group_ = number;
   if (group_name.empty())
     StringAppendF(&group_name_, "%d", group_);
   else
     group_name_ = group_name;
   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_;
   // Here it's OK to use |kDefaultGroupNumber| since we can't be forced and not
   // finalized.
   DCHECK(!forced_);
   SetGroupChoice(default_group_name_, kDefaultGroupNumber);
 
   // Add the field trial to shared memory.
   if (kUseSharedMemoryForFieldTrials)
-    FieldTrialList::OnGroupFinalized(this);
+    FieldTrialList::OnGroupFinalized(is_locked, this);
 }
 
 bool FieldTrial::GetActiveGroup(ActiveGroup* active_group) const {
   if (!group_reported_ || !enable_field_trial_)
     return false;
   DCHECK_NE(group_, kNotFinalized);
   active_group->trial_name = trial_name_;
   active_group->group_name = group_name_;
   return true;
 }
 
 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_;
   return true;
 }
 
 //------------------------------------------------------------------------------
 // FieldTrialList methods and members.
 
 // static
 FieldTrialList* FieldTrialList::global_ = NULL;
 
 // static
@@ skipping 163 matching lines @@
 }
 
 // static
 void FieldTrialList::AllStatesToString(std::string* output) {
   if (!global_)
     return;
   AutoLock auto_lock(global_->lock_);
 
   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));
     DCHECK_EQ(std::string::npos,
               trial.group_name.find(kPersistentStringSeparator));
     if (trial.activated)
       output->append(1, kActivationMarker);
     trial.trial_name.AppendToString(output);
     output->append(1, kPersistentStringSeparator);
     trial.group_name.AppendToString(output);
@@ skipping 186 matching lines @@
 }
 
 // static
 void FieldTrialList::RemoveObserver(Observer* observer) {
   if (!global_)
     return;
   global_->observer_list_->RemoveObserver(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
 void FieldTrialList::NotifyFieldTrialGroupSelection(FieldTrial* field_trial) {
   if (!global_)
     return;
 
   {
     AutoLock auto_lock(global_->lock_);
     if (field_trial->group_reported_)
@@ skipping 173 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