Updating FeatureList default initialization pattern

base/feature_list.cc

Index: base/feature_list.cc
diff --git a/base/feature_list.cc b/base/feature_list.cc
index 46732108dd6fcf0004e14b7ad6da06faf3561718..e1fa6f2787dcbc8a589b2bae1c5f74e64ff04dc7 100644
--- a/base/feature_list.cc
+++ b/base/feature_list.cc
@@ -35,10 +35,7 @@ bool IsValidFeatureOrFieldTrialName(const std::string& name) {
 
 }  // namespace
 
-FeatureList::FeatureList()
-  : initialized_(false),
-    initialized_from_command_line_(false) {
-}
+FeatureList::FeatureList() {}
 
 FeatureList::~FeatureList() {}
 
@@ -133,11 +130,19 @@ void FeatureList::GetFeatureOverrides(std::string* enable_overrides,
 
 // 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);
 }
 
@@ -158,8 +163,12 @@ bool FeatureList::InitializeInstance(const std::string& enable_features,
   // 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;