Eliminate potential for flaky crash in BCKSFactory::SetTestingFactory().

BCKSFactory::SetTestingFactory() calls BCKSFactory::BrowserContextShutdown(),
which a BCKSFactory subclass may override to perform operations that result in
BrowserContextDependencyManager::AssertBrowserContextWasntDestroyed() being
called (e.g., BCKSFactory::GetServiceForContext()). When used with
TestingProfile::Builder, BCKSFactory::SetTestingFactory() is called *before*
the TestingProfile calls
BrowserContextDependencyManager::CreateBrowserContextServicesForTest(). These
facts set up the potential for a flaky crash: if the BrowserContext instance
being used is at the same address as one that had been used in a previous test,
then the call to AssertBrowserContextWasntDestroyed() will raise an error. This
CL eliminates the potential for that crash by explicitly informing
BrowserContextDependencyManager that the BrowserContext instance being used in
BCKSFactory::SetTestingFactory() is live.

NOTRY=true

Review URL: https://codereview.chromium.org/159763004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@251336 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 27 insertions, 11 deletions

diff --git a/components/browser_context_keyed_service/browser_context_dependency_manager.cc b/components/browser_context_keyed_service/browser_context_dependency_manager.cc
index 913c92b..bd45e7a 100644
--- a/components/browser_context_keyed_service/browser_context_dependency_manager.cc
+++ b/components/browser_context_keyed_service/browser_context_dependency_manager.cc
@@ -71,11 +71,7 @@ void BrowserContextDependencyManager::DoCreateBrowserContextServices(
   TRACE_EVENT0("browser",
     "BrowserContextDependencyManager::DoCreateBrowserContextServices")
 #ifndef NDEBUG
-  // Unmark |context| as dead. This exists because of unit tests, which will
-  // often have similar stack structures. 0xWhatever might be created, go out
-  // of scope, and then a new BrowserContext object might be created
-  // at 0xWhatever.
-  dead_context_pointers_.erase(context);
+  MarkBrowserContextLiveForTesting(context);
 #endif
 
   std::vector<DependencyNode*> construction_order;
@@ -139,6 +135,11 @@ void BrowserContextDependencyManager::AssertBrowserContextWasntDestroyed(
                  << "services again.";
   }
 }
+
+void BrowserContextDependencyManager::MarkBrowserContextLiveForTesting(
+    content::BrowserContext* context) {
+  dead_context_pointers_.erase(context);
+}
 #endif
 
 // static
diff --git a/components/browser_context_keyed_service/browser_context_dependency_manager.h b/components/browser_context_keyed_service/browser_context_dependency_manager.h
index 6b2ba26..d51b5d8 100644
--- a/components/browser_context_keyed_service/browser_context_dependency_manager.h
+++ b/components/browser_context_keyed_service/browser_context_dependency_manager.h
@@ -68,6 +68,13 @@ class BROWSER_CONTEXT_KEYED_SERVICE_EXPORT BrowserContextDependencyManager {
   // mode. This will NOTREACHED() whenever the user is trying to access a stale
   // BrowserContext*.
   void AssertBrowserContextWasntDestroyed(content::BrowserContext* context);
+
+  // Marks |context| as live (i.e., not stale). This method can be called as a
+  // safeguard against |AssertBrowserContextWasntDestroyed()| checks going off
+  // due to |context| aliasing a BrowserContext instance from a prior test
+  // (i.e., 0xWhatever might be created, be destroyed, and then a new
+  // BrowserContext object might be created at 0xWhatever).
+  void MarkBrowserContextLiveForTesting(content::BrowserContext* context);
 #endif
 
   static BrowserContextDependencyManager* GetInstance();
diff --git a/components/browser_context_keyed_service/browser_context_keyed_base_factory.h b/components/browser_context_keyed_service/browser_context_keyed_base_factory.h
index 36f2829..4d8e95a 100644
--- a/components/browser_context_keyed_service/browser_context_keyed_base_factory.h
+++ b/components/browser_context_keyed_service/browser_context_keyed_base_factory.h
@@ -111,6 +111,12 @@ BrowserContextKeyedBaseFactory
   // Mark context as Preferences set.
   void MarkPreferencesSetOn(content::BrowserContext* context);
 
+  // Which BrowserContextDependencyManager we should communicate with.
+  // In real code, this will always be
+  // BrowserContextDependencyManager::GetInstance(), but unit tests will want
+  // to use their own copy.
+  BrowserContextDependencyManager* dependency_manager_;
+
  private:
   friend class BrowserContextDependencyManager;
   friend class BrowserContextDependencyManagerUnittests;
@@ -132,12 +138,6 @@ BrowserContextKeyedBaseFactory
   // now for when we're creating the context.
   virtual void CreateServiceNow(content::BrowserContext* context) = 0;
 
-  // Which BrowserContextDependencyManager we should communicate with.
-  // In real code, this will always be
-  // BrowserContextDependencyManager::GetInstance(), but unit tests will want
-  // to use their own copy.
-  BrowserContextDependencyManager* dependency_manager_;
-
   // BrowserContexts that have this service's preferences registered on them.
   std::set<const content::BrowserContext*> registered_preferences_;
 
diff --git a/components/browser_context_keyed_service/browser_context_keyed_service_factory.cc b/components/browser_context_keyed_service/browser_context_keyed_service_factory.cc
index 8f2bc72..c9d6646 100644
--- a/components/browser_context_keyed_service/browser_context_keyed_service_factory.cc
+++ b/components/browser_context_keyed_service/browser_context_keyed_service_factory.cc
@@ -20,6 +20,14 @@ void BrowserContextKeyedServiceFactory::SetTestingFactory(
   // 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
+  // |AssertBrowserContextWasntDestroyed| to raise an error.
+  dependency_manager_->MarkBrowserContextLiveForTesting(context);
+#endif
+
   // We have to go through the shutdown and destroy mechanisms because there
   // are unit tests that create a service on a context and then change the
   // testing service mid-test.