Add DCHECK and TODO to keyed service about lazy-initialised factories.

Lazily-initialised keyed service factories are not correctly set to
produce null services while testing, nor can they correctly run
ServiceIsCreatedWithContext. This introduces bugs that are non-obvious
from the interface in keyed_service_base_factory.

This CL adds a DCHECK so users of ServiceIsCreatedWithContext and
ServiceIsNULLWhileTesting will be notified that those methods won't work
as expected, and a TODO to enforce the check on all factories.

Split out from crrev.com/c/2539813

Bug: 1150733
Change-Id: I628fe3268a287d2e2e9a6eab94f0d91e9633a62a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2554194
Commit-Queue: Glen Robertson <glenrob@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832637}

components/keyed_service/core/dependency_manager.cc

@@ -5,7 +5,7 @@
 #include "components/keyed_service/core/dependency_manager.h"
 
 #include "base/bind.h"
-#include "base/check_op.h"
+#include "base/check.h"
 #include "base/debug/dump_without_crashing.h"
 #include "base/notreached.h"
 #include "base/supports_user_data.h"
@@ -23,6 +23,19 @@ DependencyManager::~DependencyManager() {
 }
 
 void DependencyManager::AddComponent(KeyedServiceBaseFactory* component) {
+#if DCHECK_IS_ON()
+  // TODO(crbug.com/1150733): Tighten this check to ensure that no factories are
+  // registered after CreateContextServices() is called.
+  DCHECK(!context_services_created_ ||
+         !(component->ServiceIsCreatedWithContext() ||
+           component->ServiceIsNULLWhileTesting()))
+      << "Tried to construct " << component->name()
+      << " after context.\n"
+         "Keyed Service Factories must be constructed before the context is "
+         "created. Typically this is done by calling FooFactory::GetInstance() "
+         "for all factories in a method called "
+         "Ensure.*KeyedServiceFactoriesBuilt().";
+#endif  // DCHECK_IS_ON()
   dependency_graph_.AddNode(component);
 }
 
@@ -51,6 +64,9 @@ void DependencyManager::RegisterPrefsForServices(
 
 void DependencyManager::CreateContextServices(void* context,
                                               bool is_testing_context) {
+#if DCHECK_IS_ON()
+  context_services_created_ = true;
+#endif
   MarkContextLive(context);
 
   std::vector<DependencyNode*> construction_order;

components/keyed_service/core/dependency_manager.h

@@ -8,6 +8,7 @@
 #include <set>
 #include <string>
 
+#include "base/dcheck_is_on.h"
 #include "base/macros.h"
 #include "components/keyed_service/core/dependency_graph.h"
 #include "components/keyed_service/core/keyed_service_export.h"
@@ -117,6 +118,10 @@ class KEYED_SERVICE_EXPORT DependencyManager {
   // with them.
   std::set<void*> dead_context_pointers_;
 
+#if DCHECK_IS_ON()
+  bool context_services_created_ = false;
+#endif
+
   DISALLOW_COPY_AND_ASSIGN(DependencyManager);
 };
 

components/keyed_service/core/keyed_service_base_factory.h

@@ -23,6 +23,13 @@ class PrefRegistrySyncable;
 //
 // This object describes general dependency management between factories while
 // direct subclasses react to lifecycle events and implement memory management.
+//
+// All factories must have been created at least once before the context is
+// created in order to work correctly (see crbug.com/1150733). The standard way
+// to do this in a //content-based embedder is to call FooFactory::GetInstance()
+// for each factory used by your embedder from your embedder's implementation of
+// content::BrowserMainParts::PreMainMessageLoopRun(). See //weblayer's
+// browser_main_parts_impl.cc for a straightforward example.
 class KEYED_SERVICE_EXPORT KeyedServiceBaseFactory : public DependencyNode {
  public:
   // The type is used to determine whether a service can depend on another.
@@ -69,7 +76,7 @@ class KEYED_SERVICE_EXPORT KeyedServiceBaseFactory : public DependencyNode {
   virtual bool ServiceIsCreatedWithContext() const;
 
   // By default, testing contexts will be treated like normal contexts. If this
-  // method is overriden to return true, then the service associated with the
+  // method is overridden to return true, then the service associated with the
   // testing context will be null.
   virtual bool ServiceIsNULLWhileTesting() const;
 
@@ -96,8 +103,8 @@ class KEYED_SERVICE_EXPORT KeyedServiceBaseFactory : public DependencyNode {
   // by all the factories of a given type. Unit tests will use their own copy.
   DependencyManager* dependency_manager_;
 
-  // Registers any preferences used by this service. This should be overriden by
-  // any services that want to register context-specific preferences.
+  // Registers any preferences used by this service. This should be overridden
+  // by any services that want to register context-specific preferences.
   virtual void RegisterPrefs(user_prefs::PrefRegistrySyncable* registry) {}
 
   // Used by DependencyManager to disable creation of the service when the