Updating FeatureList default initialization pattern

base/feature_list.cc

 // Copyright 2015 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/feature_list.h"
 
 #include <stddef.h>
 
 #include <utility>
 #include <vector>
@@ skipping 17 matching lines @@
 // serialization. This function checks that the strings are ASCII (since they
 // are used in command-line API functions that require ASCII) and whether there
 // are any reserved characters present, returning true if the string is valid.
 // Only called in DCHECKs.
 bool IsValidFeatureOrFieldTrialName(const std::string& name) {
   return IsStringASCII(name) && name.find_first_of(",<*") == std::string::npos;
 }
 
 }  // namespace
 
-FeatureList::FeatureList()
-  : initialized_(false),
-    initialized_from_command_line_(false) {
-}
+FeatureList::FeatureList() {}
 
 FeatureList::~FeatureList() {}
 
 void FeatureList::InitializeFromCommandLine(
     const std::string& enable_features,
     const std::string& disable_features) {
   DCHECK(!initialized_);
 
   // Process disabled features first, so that disabled ones take precedence over
   // enabled ones (since RegisterOverride() uses insert()).
@@ skipping 74 matching lines @@
     target_list->append(entry.first);
     if (entry.second.field_trial) {
       target_list->push_back('<');
       target_list->append(entry.second.field_trial->trial_name());
     }
   }
 }
 
 // static
 bool FeatureList::IsEnabled(const Feature& feature) {
+  if (!GetInstance()) {
+    CHECK(InitializeInstance(std::string(), std::string()));

[Sergey Ulanov, 2016/07/01 21:36:10]

As discussed offline I think the problem here is this initialization is not
thread-safe. Previously InitializeInstance() wasn't thread-safe, but that would
be a problem only in case it's used incorrectly. With this change IsEnabled()
becomes non-thread-safe even in case it's used correctly.
IMO a good solution to this problem is to change this class to use
base::Singleton<> as it implements thread-safe initialization.

[Alexei Svitkine (slow), 2016/07/04 15:50:06]

I don't think we want this class to use base::Singleton since we don't want to
incur grabbing a lock when checking IsEnabled().

However, I agree that we shouldn't call InitializeInstance() from IsEnabled().

The other way to do this is something like:

if (!GetInstance()) {
  initialized_from_accessor_ = true;
  return false;
}

And make initialized_from_accessor_  a static member.

Now, it does mean that the field may be written from multiple thread without
synchronization - but actually this will be fine in practice. That's because
this field is only read from InitializeInstance which only gets called in single
threaded mode. In the case where a binary that doesn't initialize an instance
runs this, then the field will just keep getting written to but never read, so
that's OK - no races.

[Sergey Ulanov, 2016/07/04 16:52:53]

base::Singleton<>::get() doesn't have any locks. It uses atomic ops to make
access and initialization thread-safe.

[Alexei Svitkine (slow), 2016/07/04 16:55:55]

Still, that's more overhead than we have currently - atomic ops can flush caches
and so forth on multi-proc systems and we this API to very lightweight.

[joedow, 2016/07/05 21:05:56]

My concern with the feedback above is that I don't think we should always return
false from IsEnabled(), I think it is better to return the feature's
default_state instead, that way consumers of feature enabled components will get
the intended default state if the FeatureList is not initialized in that binary.

[Alexei Svitkine (slow), 2016/07/05 21:11:34]

That's a good point. Returning default state SGTM.

+    GetInstance()->initialized_from_accessor_ = true;
+  }
   return GetInstance()->IsFeatureEnabled(feature);
 }
 
 // static
 FieldTrial* FeatureList::GetFieldTrial(const Feature& feature) {
+  if (!GetInstance()) {
+    CHECK(InitializeInstance(std::string(), std::string()));
+    GetInstance()->initialized_from_accessor_ = true;
+  }
   return GetInstance()->GetAssociatedFieldTrial(feature);
 }
 
 // static
 std::vector<std::string> FeatureList::SplitFeatureListString(
     const std::string& input) {
   return SplitString(input, ",", TRIM_WHITESPACE, SPLIT_WANT_NONEMPTY);
 }
 
 // static
 bool FeatureList::InitializeInstance(const std::string& enable_features,
                                      const std::string& disable_features) {
   // We want to initialize a new instance here to support command-line features
   // in testing better. For example, we initialize a dummy instance in
   // base/test/test_suite.cc, and override it in content/browser/
   // browser_main_loop.cc.
   // On the other hand, we want to avoid re-initialization from command line.
   // For example, we initialize an instance in chrome/browser/
   // chrome_browser_main.cc and do not override it in content/browser/
   // browser_main_loop.cc.
+  // If the singleton was previously initialized from within an accessor, we
+  // want to prevent callers from reinitializing the singleton and masking the
+  // access call(s) which likely returned incorrect information.
   bool instance_existed_before = false;
   if (g_instance) {
+    DCHECK(!g_instance->initialized_from_accessor_);
     if (g_instance->initialized_from_command_line_)
       return false;
 
     delete g_instance;
     g_instance = nullptr;
     instance_existed_before = true;
   }
 
   std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
   feature_list->InitializeFromCommandLine(enable_features, disable_features);
@@ skipping 115 matching lines @@
   return it->second == &feature;
 }
 
 FeatureList::OverrideEntry::OverrideEntry(OverrideState overridden_state,
                                           FieldTrial* field_trial)
     : overridden_state(overridden_state),
       field_trial(field_trial),
       overridden_by_field_trial(field_trial != nullptr) {}
 
 }  // namespace base