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..bf9de4b49fd350a752bc10e56f5d13ed4313eece 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 g_initialized_from_accessor = false;
+
 // 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 (!g_instance) {
+    g_initialized_from_accessor = true;
+    return feature.default_state == FEATURE_ENABLED_BY_DEFAULT;
+  }
   return GetInstance()->IsFeatureEnabled(feature);

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

Nit: Use g_instance here too.

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

Done.

 }
 
 // static
 FieldTrial* FeatureList::GetFieldTrial(const Feature& feature) {
+  if (!g_instance) {
+    g_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.
+  CHECK(!g_initialized_from_accessor);

[Sergey Ulanov, 2016/07/06 19:11:51]

nit: I don't think this needs to be a CHECK(). Replace with a DCHECK()?

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

See my earlier comments, I want to preserve the same conditions under which it
can fail and since previously it would crash even in release, I would like that
to still be the case with these updated checks.

   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;
+  g_initialized_from_accessor = false;
 }
 
 void FeatureList::FinalizeInitialization() {