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..46a2a62f16500121bbfc09c626952806dbd05faf 100644
--- a/base/feature_list.cc
+++ b/base/feature_list.cc
@@ -23,6 +23,9 @@ namespace {
 // have more control over initialization timing. Leaky.
 FeatureList* g_instance = nullptr;
 
+// Tracks whether the FeatureList instance was initialized via an accessor.
+bool initialized_from_accessor = false;

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

Nit: add g_

[joedow, 2016/07/05 22:24:20]

Done.

+
 // Some characters are not allowed to appear in feature names or the associated
 // field trial names, as they are used as special characters for command-line
 // serialization. This function checks that the strings are ASCII (since they
@@ -35,10 +38,7 @@ bool IsValidFeatureOrFieldTrialName(const std::string& name) {
 
 }  // namespace
 
-FeatureList::FeatureList()
-  : initialized_(false),
-    initialized_from_command_line_(false) {
-}
+FeatureList::FeatureList() {}
 
 FeatureList::~FeatureList() {}
 
@@ -133,11 +133,19 @@ void FeatureList::GetFeatureOverrides(std::string* enable_overrides,
 
 // static
 bool FeatureList::IsEnabled(const Feature& feature) {
+  if (!GetInstance()) {

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

Nit: Change this and line 140 to g_instance. No need for extra function calls.
:)

[joedow, 2016/07/05 22:24:20]

Done.

+    initialized_from_accessor = true;
+    return feature.default_state == FEATURE_ENABLED_BY_DEFAULT;
+  }
   return GetInstance()->IsFeatureEnabled(feature);
 }
 
 // static
 FieldTrial* FeatureList::GetFieldTrial(const Feature& feature) {
+  if (!GetInstance()) {

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

Why do we need to add this check?

In a non-Chrome case, why would anyone call GetFieldTrial()?

[joedow, 2016/07/05 22:24:20]

I'm anticipating a scenario where a component which regularly ships with Chrome
wants to participate in or key off of a FieldTrial but we don't initialize the
Singleton.  In that case, we would see a crash after syncing and need to root
cause it (just like the IsEnabled() check this time around).

[Alexei Svitkine (slow), 2016/07/06 19:59:06]

It's not obvious to me this would happen. I don't foresee anything outside the
current uses ever using this API.

Now, it's used by the GetVariationParamValueByFeature() API. So if that API gets
used, then yes you could run into this. However - for that API to be used, you'd
have to have the code using it depend on components/variations. Could that ever
happen - i.e. the binaries you care about, will they ever depend on arbitrary
components such as component/variations?

[joedow, 2016/07/07 03:53:01]

I can remove it for now.  Our problem is that we don't know when a component
updates their dependencies until we sync.  If that does occur we can add this
back in.

+    initialized_from_accessor = true;
+    return nullptr;
+  }
   return GetInstance()->GetAssociatedFieldTrial(feature);
 }
 
@@ -158,6 +166,10 @@ 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
+  // accessor call(s) which likely returned incorrect information.
+  DCHECK(!initialized_from_accessor);

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

Make this a CHECK(). I would like this to fail in non-DCHECK builds too, which
is true for previous failure mode (using IsEnabled() before FeatureLIst is
initialized).

[joedow, 2016/07/05 22:24:20]

Done.

   bool instance_existed_before = false;
   if (g_instance) {
     if (g_instance->initialized_from_command_line_)
@@ -192,6 +204,7 @@ void FeatureList::SetInstance(std::unique_ptr<FeatureList> instance) {
 void FeatureList::ClearInstanceForTesting() {
   delete g_instance;
   g_instance = nullptr;
+  initialized_from_accessor = false;
 }
 
 void FeatureList::FinalizeInitialization() {