Add OnAppRegistryCacheWillBeDestroyed method

This allows cleanly breaking observer-observee reference cycles,
regardless of which object is destroyed first. Prior to this commit, the
implicit assumption was that the observer was always destroyed first.

A follow-up commit will introduce some tests that create and destroy
AppRegistryCache objects.

BUG=826982

Change-Id: I94966439001ba310b5ee824ff62e72a533887c0d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1629892
Commit-Queue: Nigel Tao <nigeltao@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Lucas Tenório <ltenorio@chromium.org>
Reviewed-by: Yury Khmel <khmel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664095}

chrome/browser/chromeos/kiosk_next_home/app_controller_service.cc

@@ -65,7 +65,7 @@ AppControllerService::AppControllerService(Profile* profile)
       app_service_proxy_(apps::AppServiceProxyFactory::GetForProfile(profile)),
       intent_config_helper_(IntentConfigHelper::GetInstance()) {
   DCHECK(profile);
-  app_service_proxy_->AppRegistryCache().AddObserver(this);
+  Observe(&app_service_proxy_->AppRegistryCache());
 
   // Add the chrome://app-icon URL data source.
   // TODO(ltenorio): Move this to a more suitable location when we change
@@ -74,9 +74,7 @@ AppControllerService::AppControllerService(Profile* profile)
                               std::make_unique<apps::AppIconSource>(profile));
 }
 
-AppControllerService::~AppControllerService() {
-  app_service_proxy_->AppRegistryCache().RemoveObserver(this);
-}
+AppControllerService::~AppControllerService() = default;
 
 void AppControllerService::BindRequest(mojom::AppControllerRequest request) {
   bindings_.AddBinding(this, std::move(request));
@@ -171,6 +169,11 @@ void AppControllerService::OnAppUpdate(const apps::AppUpdate& update) {
   }
 }
 
+void AppControllerService::OnAppRegistryCacheWillBeDestroyed(
+    apps::AppRegistryCache* cache) {
+  Observe(nullptr);
+}
+
 void AppControllerService::SetIntentConfigHelperForTesting(
     std::unique_ptr<IntentConfigHelper> helper) {
   intent_config_helper_ = std::move(helper);

chrome/browser/chromeos/kiosk_next_home/app_controller_service.h

@@ -59,6 +59,8 @@ class AppControllerService : public mojom::AppController,
 
   // apps::AppRegistryCache::Observer:
   void OnAppUpdate(const apps::AppUpdate& update) override;
+  void OnAppRegistryCacheWillBeDestroyed(
+      apps::AppRegistryCache* cache) override;
 
   // Allows overriding the intent config helper for tests.
   void SetIntentConfigHelperForTesting(

chrome/browser/ui/app_list/app_service_app_model_builder.cc

@@ -105,3 +105,8 @@ void AppServiceAppModelBuilder::OnAppUpdate(const apps::AppUpdate& update) {
         profile(), model_updater(), GetSyncItem(update.AppId()), update));
   }
 }
+
+void AppServiceAppModelBuilder::OnAppRegistryCacheWillBeDestroyed(
+    apps::AppRegistryCache* cache) {
+  Observe(nullptr);
+}

chrome/browser/ui/app_list/app_service_app_model_builder.h

@@ -26,6 +26,8 @@ class AppServiceAppModelBuilder : public AppListModelBuilder,
 
   // apps::AppRegistryCache::Observer overrides:
   void OnAppUpdate(const apps::AppUpdate& update) override;
+  void OnAppRegistryCacheWillBeDestroyed(
+      apps::AppRegistryCache* cache) override;
 
   std::unique_ptr<AppListModelUpdaterObserver> crostini_folder_observer_;
 

chrome/browser/ui/app_list/search/app_search_provider.cc

@@ -361,6 +361,11 @@ class AppServiceDataSource : public AppSearchProvider::DataSource,
     }
   }
 
+  void OnAppRegistryCacheWillBeDestroyed(
+      apps::AppRegistryCache* cache) override {
+    Observe(nullptr);
+  }
+
   // The AppServiceDataSource seems like one (but not the only) good place to
   // add an App Service icon caching wrapper, because (1) the AppSearchProvider
   // destroys and creates multiple search results in a short period of time,

chrome/browser/ui/webui/app_management/app_management_page_handler.cc

@@ -220,3 +220,8 @@ void AppManagementPageHandler::OnAppUpdate(const apps::AppUpdate& update) {
     page_->OnAppChanged(CreateUIAppPtr(update));
   }
 }
+
+void AppManagementPageHandler::OnAppRegistryCacheWillBeDestroyed(
+    apps::AppRegistryCache* cache) {
+  Observe(nullptr);
+}

chrome/browser/ui/webui/app_management/app_management_page_handler.h

@@ -44,6 +44,8 @@ class AppManagementPageHandler : public app_management::mojom::PageHandler,
 
   // apps::AppRegistryCache::Observer overrides:
   void OnAppUpdate(const apps::AppUpdate& update) override;
+  void OnAppRegistryCacheWillBeDestroyed(
+      apps::AppRegistryCache* cache) override;
 
   mojo::Binding<app_management::mojom::PageHandler> binding_;
 

chrome/services/app_service/public/cpp/app_registry_cache.cc

@@ -36,7 +36,12 @@ void AppRegistryCache::Observer::Observe(AppRegistryCache* cache) {
 
 AppRegistryCache::AppRegistryCache() = default;
 
-AppRegistryCache::~AppRegistryCache() = default;
+AppRegistryCache::~AppRegistryCache() {
+  for (auto& obs : observers_) {
+    obs.OnAppRegistryCacheWillBeDestroyed(this);
+  }
+  DCHECK(!observers_.might_have_observers());
+}
 
 void AppRegistryCache::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);

chrome/services/app_service/public/cpp/app_registry_cache.h

@@ -37,20 +37,27 @@ class AppRegistryCache {
     // returns.
     virtual void OnAppUpdate(const AppUpdate& update) = 0;
 
+    // Called when the AppRegistryCache object (the thing that this observer
+    // observes) will be destroyed. In response, the observer, |this|, should
+    // call "cache->RemoveObserver(this)", whether directly or indirectly (e.g.
+    // via ScopedObserver::Remove or via Observe(nullptr)).
+    virtual void OnAppRegistryCacheWillBeDestroyed(AppRegistryCache* cache) = 0;
+
    protected:
-    // Use this constructor when the object is tied to a single
-    // AppRegistryCache for its entire lifetime.
+    // Use this constructor when the observer |this| is tied to a single
+    // AppRegistryCache for its entire lifetime, or until the observee (the
+    // AppRegistryCache) is destroyed, whichever comes first.
     explicit Observer(AppRegistryCache* cache);
 
-    // Use this constructor when the object wants to observe a AppRegistryCache
-    // for part of its lifetime. It can then call Observe() to start and stop
-    // observing.
+    // Use this constructor when the observer |this| wants to observe a
+    // AppRegistryCache for part of its lifetime. It can then call Observe() to
+    // start and stop observing.
     Observer();
 
     ~Observer() override;
 
-    // Start observing a different AppRegistryCache; used with the default
-    // constructor.
+    // Start observing a different AppRegistryCache. |cache| may be nullptr,
+    // meaning to stop observing.
     void Observe(AppRegistryCache* cache);
 
    private:

chrome/services/app_service/public/cpp/app_registry_cache_unittest.cc

@@ -47,6 +47,12 @@ class AppRegistryCacheTest : public testing::Test,
     updated_names_.insert(update.Name());
   }
 
+  void OnAppRegistryCacheWillBeDestroyed(
+      apps::AppRegistryCache* cache) override {
+    // The test code explicitly calls both AddObserver and RemoveObserver.
+    NOTREACHED();
+  }
+
   int num_freshly_installed_ = 0;
   std::set<std::string> updated_ids_;
   std::set<std::string> updated_names_;
@@ -63,10 +69,10 @@ class AppRegistryCacheTest : public testing::Test,
 class RecursiveObserver : public apps::AppRegistryCache::Observer {
  public:
   explicit RecursiveObserver(apps::AppRegistryCache* cache) : cache_(cache) {
-    cache_->AddObserver(this);
+    Observe(cache);
   }
 
-  ~RecursiveObserver() override { cache_->RemoveObserver(this); }
+  ~RecursiveObserver() override = default;
 
   void PrepareForOnApps(
       int expected_num_apps,
@@ -156,6 +162,11 @@ class RecursiveObserver : public apps::AppRegistryCache::Observer {
     num_apps_seen_on_app_update_++;
   }
 
+  void OnAppRegistryCacheWillBeDestroyed(
+      apps::AppRegistryCache* cache) override {
+    Observe(nullptr);
+  }
+
   static void ExpectEq(const apps::AppUpdate& outer,
                        const apps::AppUpdate& inner) {
     EXPECT_EQ(outer.AppType(), inner.AppType());