Fix leaks of the SiteDataCacheFactory instance between tests

The BrowserContextKeyedServiceFactory are leaked between tests (by
design) and this is problematic with the SiteDataCacheFacadeFactory
because it owns an SiteDataCacheFactory object in a SequenceBound
wrapper. Leaking this between tests causes UAFs on the task runner used
by this SequenceBound wrapper.

The solution consists in tracking the number of alive
SiteDataCacheFacade instances and releasing the SiteDataCacheFactory
when this count reaches 0. As profiles always get destroyed before
terminating a test this ensures that we don't leak the
SiteDataCacheFactory anymore.

Bug: 1127928
Change-Id: I88777d8761f60aeeb90322234f861d196ed9e548
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2422079
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810518}

chrome/browser/performance_manager/persistence/site_data/site_data_cache_facade.cc

@@ -10,6 +10,7 @@
 
 #include "base/bind.h"
 #include "base/run_loop.h"
+#include "base/util/type_safety/pass_key.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/performance_manager/persistence/site_data/site_data_cache_facade_factory.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
@@ -23,11 +24,14 @@
 namespace performance_manager {
 
 class GraphImpl;
+using PassKey = util::PassKey<SiteDataCacheFacade>;
 
 SiteDataCacheFacade::SiteDataCacheFacade(
     content::BrowserContext* browser_context)
     : browser_context_(browser_context) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  SiteDataCacheFacadeFactory::GetInstance()->OnBeforeFacadeCreated(PassKey());
+
   base::Optional<std::string> parent_context_id;
   if (browser_context->IsOffTheRecord()) {
     content::BrowserContext* parent_context =
@@ -53,6 +57,7 @@ SiteDataCacheFacade::~SiteDataCacheFacade() {
   SiteDataCacheFacadeFactory::GetInstance()->cache_factory()->Post(
       FROM_HERE, &SiteDataCacheFactory::OnBrowserContextDestroyed,
       browser_context_->UniqueId());
+  SiteDataCacheFacadeFactory::GetInstance()->OnFacadeDestroyed(PassKey());
 }
 
 void SiteDataCacheFacade::IsDataCacheRecordingForTesting(

chrome/browser/performance_manager/persistence/site_data/site_data_cache_facade_factory.cc

@@ -19,18 +19,13 @@
 namespace performance_manager {
 
 namespace {
-
-SiteDataCacheFacadeFactory* g_instance = nullptr;
-
 // Tests that want to use this factory will have to explicitly enable it.
 bool g_enable_for_testing = false;
 }
 
 SiteDataCacheFacadeFactory* SiteDataCacheFacadeFactory::GetInstance() {
-  if (!g_instance)
-    new SiteDataCacheFacadeFactory();
-  DCHECK(g_instance);
-  return g_instance;
+  static base::NoDestructor<SiteDataCacheFacadeFactory> instance;
+  return instance.get();
 }
 
 // static
@@ -48,30 +43,14 @@ void SiteDataCacheFacadeFactory::DisassociateForTesting(Profile* profile) {
   GetInstance()->Disassociate(profile);
 }
 
-// static
-void SiteDataCacheFacadeFactory::ReleaseInstanceForTesting() {
-  base::RunLoop run_loop;
-  g_instance->cache_factory()->ResetWithCallbackAfterDestruction(
-      run_loop.QuitClosure());
-  run_loop.Run();
-  delete g_instance;
-  DCHECK(!g_instance);
-}
-
 SiteDataCacheFacadeFactory::SiteDataCacheFacadeFactory()
     : BrowserContextKeyedServiceFactory(
           "SiteDataCacheFacadeFactory",
-          BrowserContextDependencyManager::GetInstance()),
-      cache_factory_(performance_manager::PerformanceManager::GetTaskRunner()) {
-  DCHECK(!g_instance);
-  g_instance = this;
+          BrowserContextDependencyManager::GetInstance()) {
   DependsOn(HistoryServiceFactory::GetInstance());
 }
 
-SiteDataCacheFacadeFactory::~SiteDataCacheFacadeFactory() {
-  DCHECK_EQ(this, g_instance);
-  g_instance = nullptr;
-}
+SiteDataCacheFacadeFactory::~SiteDataCacheFacadeFactory() = default;
 
 KeyedService* SiteDataCacheFacadeFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
@@ -93,4 +72,23 @@ bool SiteDataCacheFacadeFactory::ServiceIsNULLWhileTesting() const {
   return !g_enable_for_testing;
 }
 
+void SiteDataCacheFacadeFactory::OnBeforeFacadeCreated(
+    util::PassKey<SiteDataCacheFacade>) {
+  if (service_instance_count_ == 0U) {
+    DCHECK(cache_factory_.is_null());
+    cache_factory_ = base::SequenceBound<SiteDataCacheFactory>(
+        performance_manager::PerformanceManager::GetTaskRunner());
+  }
+  ++service_instance_count_;
+}
+
+void SiteDataCacheFacadeFactory::OnFacadeDestroyed(
+    util::PassKey<SiteDataCacheFacade>) {
+  DCHECK_GT(service_instance_count_, 0U);
+  // Destroy the cache factory if there's no more SiteDataCacheFacade needing
+  // it.
+  if (--service_instance_count_ == 0)
+    cache_factory_.Reset();
+}
+
 }  // namespace performance_manager

chrome/browser/performance_manager/persistence/site_data/site_data_cache_facade_factory.h

@@ -9,6 +9,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/no_destructor.h"
 #include "base/threading/sequence_bound.h"
+#include "base/util/type_safety/pass_key.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
 
 class Profile;
@@ -43,9 +44,9 @@ class SiteDataCacheFacadeTest;
 //   - When a browser context is destroyed the corresponding SiteDataCacheFacade
 //     is destroyed and this also destroys the corresponding SiteDataCache on
 //     the proper sequence (via the SequenceBound object).
-//   - At shutdown, the SiteDataCacheFacadeFactory is destroyed shortly before
-//     terminating the thread pool. Destruction of this object causes the
-//     SiteDataCacheFactory to be destroyed on its sequence.
+//   - At shutdown, when the last SiteDataCacheFacade is destroyed, a task is
+//     posted to ensure that the SiteDataCacheFactory is destroyed on its
+//     sequence.
 class SiteDataCacheFacadeFactory : public BrowserContextKeyedServiceFactory {
  public:
   ~SiteDataCacheFacadeFactory() override;
@@ -54,7 +55,6 @@ class SiteDataCacheFacadeFactory : public BrowserContextKeyedServiceFactory {
 
   static std::unique_ptr<base::AutoReset<bool>> EnableForTesting();
   static void DisassociateForTesting(Profile* profile);
-  static void ReleaseInstanceForTesting();
 
  protected:
   friend class base::NoDestructor<SiteDataCacheFacadeFactory>;
@@ -67,6 +67,14 @@ class SiteDataCacheFacadeFactory : public BrowserContextKeyedServiceFactory {
     return &cache_factory_;
   }
 
+  // Should be called early in the creation of a SiteDataCacheFacade to make
+  // sure that |cache_factory_| gets created.
+  void OnBeforeFacadeCreated(util::PassKey<SiteDataCacheFacade> key);
+
+  // Should be called at the end of the destruction of a SiteDataCacheFacade to
+  // release |cache_factory_| if there's no more profile needing it.
+  void OnFacadeDestroyed(util::PassKey<SiteDataCacheFacade> key);
+
  private:
   // BrowserContextKeyedServiceFactory:
   KeyedService* BuildServiceInstanceFor(
@@ -79,6 +87,9 @@ class SiteDataCacheFacadeFactory : public BrowserContextKeyedServiceFactory {
   // The counterpart of this factory living on the SiteDataCache's sequence.
   base::SequenceBound<SiteDataCacheFactory> cache_factory_;
 
+  // The number of SiteDataCacheFacade currently in existence.
+  size_t service_instance_count_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(SiteDataCacheFacadeFactory);
 };
 

chrome/browser/performance_manager/persistence/site_data/site_data_cache_facade_unittest.cc

@@ -113,7 +113,6 @@ class SiteDataCacheFacadeTest : public testing::TestWithPerformanceManager {
   void TearDown() override {
     use_in_memory_db_for_testing_.reset();
     profile_.reset();
-    SiteDataCacheFacadeFactory::ReleaseInstanceForTesting();
     testing::TestWithPerformanceManager::TearDown();
   }
 

chrome/browser/performance_manager/test_support/site_data_utils.cc

@@ -46,7 +46,6 @@ void SiteDataTestHarness::TearDown(Profile* profile) {
 }
 
 void SiteDataTestHarness::TearDown() {
-  SiteDataCacheFacadeFactory::ReleaseInstanceForTesting();
   PerformanceManagerTestHarnessHelper::TearDown();
 }