Allow cross dependencies between BCKSF and BSKSF

components/keyed_service/core/keyed_service_factory.cc

Index: components/keyed_service/core/keyed_service_factory.cc
diff --git a/components/keyed_service/core/keyed_service_factory.cc b/components/keyed_service/core/keyed_service_factory.cc
index 1417bab80a39e88278147bd333e60854f13f5890..17d694f1fafb100c9100881f23debbd50fde2363 100644
--- a/components/keyed_service/core/keyed_service_factory.cc
+++ b/components/keyed_service/core/keyed_service_factory.cc
@@ -22,6 +22,7 @@ KeyedServiceFactory::~KeyedServiceFactory() {
 void KeyedServiceFactory::SetTestingFactory(
     base::SupportsUserData* context,
     TestingFactoryFunction testing_factory) {
+  context = GetUnderlyingContext(context);

[Elliot Glaysher, 2015/04/21 17:54:32]

This seems like it could be very error prone. Is this one of the temporary
things?

[sdefresne, 2015/04/22 08:53:15]

Yes, all calls to GetUnderlyingContext/GetOriginalContext are temporary.

The reason is that downstream iOS use a BrowserContextDependencyManager that
contains both BrowserContextKeyedServiceFactory and
BrowserStateKeyedServiceFactory.

When the service factory overrides ServiceIsNULLWhileTesting, the dependency
manager will call SetEmptyTestingFactory() with the context that is a
content::BrowserContext. But when the client code calls the service factory
GetForBrowserState() it passes a web::BrowserState to the service factory, and
the testing factory is not found (since they are different objects).

This cross-dependency is quite dangerous, but only happens on iOS downstream. To
mitigate the risk, I can change GetUnderlyingContext/GetOriginalContext to be
simple inline method doing nothing if not targetting iOS, something like:

  // Returns the underlying context to use for |context|. It is a simple
  // pass-through for all platform except iOS. On iOS is it used to allow
  // mixing dependencies between BrowserContextKeyedService and
  // BrowserStateKeyedService and returns the correctly typed context to
  // use (respectively content::BrowserContext and web::BrowserState) from
  // the one passed by DependencyManager or the super class. If the context
  // is already correctly typed, then it must be a pass-through.
#if !defined(OS_IOS)
  base::SupportsUserData* GetUnderlyingContext(
      base::SupportsUserData* context) const {
    return context;
  }
#else
  // Method comment ...
  base::SupportsUserData* GetUnderlyingContext(
      base::SupportsUserData* context) const;
  virtual base::SupportsUserData* GetUnderlyingContextInternal(
      base::SupportsUserData* context) const = 0;
#endif

[Elliot Glaysher, 2015/04/22 17:34:07]

Thanks, that sounds like a good plan.

   // Destroying the context may cause us to lose data about whether |context|
   // has our preferences registered on it (since the context object itself
   // isn't dead). See if we need to readd it once we've gone through normal
@@ -52,6 +53,7 @@ KeyedService* KeyedServiceFactory::SetTestingFactoryAndUse(
     base::SupportsUserData* context,
     TestingFactoryFunction testing_factory) {
   DCHECK(testing_factory);
+  context = GetUnderlyingContext(context);
   SetTestingFactory(context, testing_factory);
   return GetServiceForContext(context, true);
 }
@@ -60,6 +62,7 @@ KeyedService* KeyedServiceFactory::GetServiceForContext(
     base::SupportsUserData* context,
     bool create) {
   TRACE_EVENT0("browser,startup", "KeyedServiceFactory::GetServiceForContext");
+  context = GetUnderlyingContext(context);
   context = GetContextToUse(context);
   if (!context)
     return nullptr;
@@ -95,11 +98,13 @@ KeyedService* KeyedServiceFactory::GetServiceForContext(
 
 void KeyedServiceFactory::Associate(base::SupportsUserData* context,
                                     KeyedService* service) {
+  context = GetUnderlyingContext(context);
   DCHECK(!ContainsKey(mapping_, context));
   mapping_.insert(std::make_pair(context, service));
 }
 
 void KeyedServiceFactory::Disassociate(base::SupportsUserData* context) {
+  context = GetUnderlyingContext(context);
   const auto& it = mapping_.find(context);
   if (it != mapping_.end()) {
     delete it->second;
@@ -108,12 +113,14 @@ void KeyedServiceFactory::Disassociate(base::SupportsUserData* context) {
 }
 
 void KeyedServiceFactory::ContextShutdown(base::SupportsUserData* context) {
+  context = GetUnderlyingContext(context);
   const auto& it = mapping_.find(context);
   if (it != mapping_.end() && it->second)
     it->second->Shutdown();
 }
 
 void KeyedServiceFactory::ContextDestroyed(base::SupportsUserData* context) {
+  context = GetUnderlyingContext(context);
   Disassociate(context);
 
   // For unit tests, we also remove the factory function both so we don't
@@ -127,13 +134,16 @@ void KeyedServiceFactory::ContextDestroyed(base::SupportsUserData* context) {
 
 void KeyedServiceFactory::SetEmptyTestingFactory(
     base::SupportsUserData* context) {
+  context = GetUnderlyingContext(context);
   SetTestingFactory(context, nullptr);
 }
 
 bool KeyedServiceFactory::HasTestingFactory(base::SupportsUserData* context) {
+  context = GetUnderlyingContext(context);
   return testing_factories_.find(context) != testing_factories_.end();
 }
 
 void KeyedServiceFactory::CreateServiceNow(base::SupportsUserData* context) {
+  context = GetUnderlyingContext(context);
   GetServiceForContext(context, true);
 }