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 316 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() {
+  FinalizeGroupChoiceImpl(false);
+}
+
+void FieldTrial::FinalizeGroupChoiceImpl(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();
   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;
+  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.
 
 // static
 FieldTrialList* FieldTrialList::global_ = NULL;
 
 // static
 bool FieldTrialList::used_without_global_ = false;
 
 FieldTrialList::Observer::~Observer() {
@@ skipping 160 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 104 matching lines @@
 }
 
 #if defined(OS_WIN)
 // static
 void FieldTrialList::AppendFieldTrialHandleIfNeeded(
     HandlesToInheritVector* handles) {
   if (!global_)
     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_);
   }
 }
 #endif
 
 // static
 void FieldTrialList::CopyFieldTrialStateToFlags(
     const char* field_trial_handle_switch,
     CommandLine* cmd_line) {
 #if defined(OS_WIN)
   // Use shared memory to pass the state if the feature is enabled, otherwise
   // fallback to passing it via the command line as a string.
   if (kUseSharedMemoryForFieldTrials) {
     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;
     }
 
     // HANDLE is just typedef'd to void *. We basically cast the handle into an
     // int (uintptr_t, to be exact), stringify the int, and pass it as a
     // command-line flag. The child process will do the reverse conversions to
     // retrieve the handle. See http://stackoverflow.com/a/153077
     auto uintptr_handle =
         reinterpret_cast<uintptr_t>(global_->readonly_allocator_handle_);
@@ skipping 46 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 171 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