Allow cross dependencies between BCKSF and BSKSF

components/keyed_service/core/keyed_service_factory.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/keyed_service/core/keyed_service_factory.h"
 
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/trace_event/trace_event.h"
 #include "components/keyed_service/core/dependency_manager.h"
 #include "components/keyed_service/core/keyed_service.h"
 
 KeyedServiceFactory::KeyedServiceFactory(const char* name,
                                          DependencyManager* manager)
     : KeyedServiceBaseFactory(name, manager) {
 }
 
 KeyedServiceFactory::~KeyedServiceFactory() {
   DCHECK(mapping_.empty());
 }
 
 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
   // destruction.
   bool add_context = ArePreferencesSetOn(context);
 
 #ifndef NDEBUG
   // Ensure that |context| is not marked as stale (e.g., due to it aliasing an
   // instance that was destroyed in an earlier test) in order to avoid accesses
   // to |context| in |BrowserContextShutdown| from causing
@@ skipping 10 matching lines @@
   if (add_context)
     MarkPreferencesSetOn(context);
 
   testing_factories_[context] = testing_factory;
 }
 
 KeyedService* KeyedServiceFactory::SetTestingFactoryAndUse(
     base::SupportsUserData* context,
     TestingFactoryFunction testing_factory) {
   DCHECK(testing_factory);
+  context = GetUnderlyingContext(context);
   SetTestingFactory(context, testing_factory);
   return GetServiceForContext(context, true);
 }
 
 KeyedService* KeyedServiceFactory::GetServiceForContext(
     base::SupportsUserData* context,
     bool create) {
   TRACE_EVENT0("browser,startup", "KeyedServiceFactory::GetServiceForContext");
+  context = GetUnderlyingContext(context);
   context = GetContextToUse(context);
   if (!context)
     return nullptr;
 
   // NOTE: If you modify any of the logic below, make sure to update the
   // refcounted version in refcounted_context_keyed_service_factory.cc!
   const auto& it = mapping_.find(context);
   if (it != mapping_.end())
     return it->second;
 
@@ skipping 15 matching lines @@
   } else {
     service = BuildServiceInstanceFor(context);
   }
 
   Associate(context, service);
   return service;
 }
 
 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;
     mapping_.erase(it);
   }
 }
 
 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
   // maintain a big map of dead pointers, but also since we may have a second
   // object that lives at the same address (see other comments about unit tests
   // in this file).
   testing_factories_.erase(context);
 
   KeyedServiceBaseFactory::ContextDestroyed(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);
 }