Clean up media gallery services that hold on to browser contexts.

Both GalleryWatchManager and MediaFileSystemRegistry are global
singletons that track some things on a per-browser context basis. To
prevent dangling pointers they must subscribe to browser context
shutdown in order to clean up these references.

BUG=467627

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

Cr-Commit-Position: refs/heads/master@{#335740}

5 files changed, 164 insertions, 2 deletions

diff --git a/chrome/browser/media_galleries/gallery_watch_manager.cc b/chrome/browser/media_galleries/gallery_watch_manager.cc
index 6a2efd7..8ca263f 100644
--- a/chrome/browser/media_galleries/gallery_watch_manager.cc
+++ b/chrome/browser/media_galleries/gallery_watch_manager.cc
@@ -10,7 +10,9 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/media_galleries/gallery_watch_manager_observer.h"
 #include "chrome/browser/media_galleries/media_file_system_registry.h"
+#include "chrome/browser/media_galleries/media_galleries_preferences_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "components/keyed_service/content/browser_context_keyed_service_shutdown_notifier_factory.h"
 #include "components/storage_monitor/storage_monitor.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
@@ -24,6 +26,26 @@ namespace {
 // Don't send a notification more than once per 3 seconds (chosen arbitrarily).
 const int kMinNotificiationDelayInSeconds = 3;
 
+class ShutdownNotifierFactory
+    : public BrowserContextKeyedServiceShutdownNotifierFactory {
+ public:
+  static ShutdownNotifierFactory* GetInstance() {
+    return Singleton<ShutdownNotifierFactory>::get();
+  }
+
+ private:
+  friend struct DefaultSingletonTraits<ShutdownNotifierFactory>;
+
+  ShutdownNotifierFactory()
+      : BrowserContextKeyedServiceShutdownNotifierFactory(
+            "GalleryWatchManager") {
+    DependsOn(MediaGalleriesPreferencesFactory::GetInstance());
+  }
+  ~ShutdownNotifierFactory() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(ShutdownNotifierFactory);
+};
+
 }  // namespace.
 
 const char GalleryWatchManager::kInvalidGalleryIDError[] = "Invalid gallery ID";
@@ -187,6 +209,19 @@ void GalleryWatchManager::ShutdownBrowserContext(
   size_t observed = observed_preferences_.erase(preferences);
   if (observed > 0)
     preferences->RemoveGalleryChangeObserver(this);
+
+  WatchesMap::iterator it = watches_.begin();
+  while (it != watches_.end()) {
+    if (it->first.browser_context == browser_context) {
+      DeactivateFileWatch(it->first, it->second);
+      // Post increment moves iterator to next element while deleting current.
+      watches_.erase(it++);
+    } else {
+      ++it;
+    }
+  }
+
+  browser_context_subscription_map_.erase(browser_context);
 }
 
 void GalleryWatchManager::AddWatch(BrowserContext* browser_context,
@@ -234,6 +269,7 @@ void GalleryWatchManager::AddWatch(BrowserContext* browser_context,
   }
 
   watches_[owner] = path;
+  EnsureBrowserContextSubscription(owner.browser_context);
 
   // Start the FilePathWatcher on |gallery_path| if necessary.
   if (ContainsKey(watched_paths_, path)) {
@@ -302,6 +338,19 @@ MediaGalleryPrefIdSet GalleryWatchManager::GetWatchSet(
   return result;
 }
 
+void GalleryWatchManager::EnsureBrowserContextSubscription(
+    BrowserContext* browser_context) {
+  auto it = browser_context_subscription_map_.find(browser_context);
+  if (it == browser_context_subscription_map_.end()) {
+    browser_context_subscription_map_.set(
+        browser_context,
+        ShutdownNotifierFactory::GetInstance()
+            ->Get(browser_context)
+            ->Subscribe(base::Bind(&GalleryWatchManager::ShutdownBrowserContext,
+                                   base::Unretained(this), browser_context)));
+  }
+}
+
 void GalleryWatchManager::DeactivateFileWatch(const WatchOwner& owner,
                                               const base::FilePath& path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
diff --git a/chrome/browser/media_galleries/gallery_watch_manager.h b/chrome/browser/media_galleries/gallery_watch_manager.h
index 5ad691f..f4803c8 100644
--- a/chrome/browser/media_galleries/gallery_watch_manager.h
+++ b/chrome/browser/media_galleries/gallery_watch_manager.h
@@ -10,6 +10,7 @@
 
 #include "base/basictypes.h"
 #include "base/callback_forward.h"
+#include "base/containers/scoped_ptr_map.h"
 #include "base/files/file_path.h"
 #include "base/files/file_path_watcher.h"
 #include "base/memory/linked_ptr.h"
@@ -17,6 +18,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "chrome/browser/media_galleries/media_galleries_preferences.h"
+#include "components/keyed_service/core/keyed_service_shutdown_notifier.h"
 #include "components/storage_monitor/removable_storage_observer.h"
 
 class GalleryWatchManagerObserver;
@@ -107,6 +109,14 @@ class GalleryWatchManager
   typedef std::map<base::FilePath, NotificationInfo> WatchedPaths;
   typedef std::map<content::BrowserContext*, GalleryWatchManagerObserver*>
       ObserverMap;
+  typedef ScopedPtrMap<content::BrowserContext*,
+                       scoped_ptr<KeyedServiceShutdownNotifier::Subscription>>
+      BrowserContextSubscriptionMap;
+
+  // Ensure there is a subscription to shutdown notifications for
+  // |browser_context|.
+  void EnsureBrowserContextSubscription(
+      content::BrowserContext* browser_context);
 
   // Stop the FilePathWatcher for |path|. Updates |watched_paths_| but not
   // |registered_watches_|.
@@ -151,6 +161,10 @@ class GalleryWatchManager
   // Helper that does the watches on the FILE thread.
   scoped_ptr<FileWatchManager> watch_manager_;
 
+  // Removes watches when a browser context is shut down as watches contain raw
+  // pointers.
+  BrowserContextSubscriptionMap browser_context_subscription_map_;
+
   base::WeakPtrFactory<GalleryWatchManager> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(GalleryWatchManager);
diff --git a/chrome/browser/media_galleries/gallery_watch_manager_unittest.cc b/chrome/browser/media_galleries/gallery_watch_manager_unittest.cc
index 4dcb951..5a94469 100644
--- a/chrome/browser/media_galleries/gallery_watch_manager_unittest.cc
+++ b/chrome/browser/media_galleries/gallery_watch_manager_unittest.cc
@@ -87,7 +87,9 @@ class GalleryWatchManagerTest : public GalleryWatchManagerObserver,
   }
 
   void TearDown() override {
-    manager_->RemoveObserver(profile_.get());
+    if (profile_) {
+      manager_->RemoveObserver(profile_.get());
+    }
     manager_.reset();
     storage_monitor::TestStorageMonitor::Destroy();
   }
@@ -149,6 +151,8 @@ class GalleryWatchManagerTest : public GalleryWatchManagerObserver,
     pending_loop_ = loop;
   }
 
+  void ShutdownProfile() { profile_.reset(nullptr); }
+
  private:
   // GalleryWatchManagerObserver implementation.
   void OnGalleryChanged(const std::string& extension_id,
@@ -353,4 +357,48 @@ TEST_F(GalleryWatchManagerTest, TestWatchOperation) {
   success_loop.Run();
 }
 
+TEST_F(GalleryWatchManagerTest, TestWatchOperationAfterProfileShutdown) {
+  if (!GalleryWatchesSupported())
+    return;
+
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  MediaGalleryPrefId id = AddGallery(temp_dir.path());
+  AddAndConfirmWatch(id);
+
+  ShutdownProfile();
+
+  // Trigger a watch that should have been removed when the profile was
+  // destroyed to catch regressions. crbug.com/467627
+  base::RunLoop run_loop;
+  ASSERT_EQ(
+      4, base::WriteFile(temp_dir.path().AppendASCII("fake file"), "blah", 4));
+  run_loop.RunUntilIdle();
+}
+
+TEST_F(GalleryWatchManagerTest, TestStorageRemovedAfterProfileShutdown) {
+  if (!GalleryWatchesSupported())
+    return;
+
+  // Create a temporary directory and treat is as a removable storage device.
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  storage_monitor()->AddRemovablePath(temp_dir.path());
+  storage_monitor::StorageInfo storage_info;
+  ASSERT_TRUE(
+      storage_monitor()->GetStorageInfoForPath(temp_dir.path(), &storage_info));
+  storage_monitor()->receiver()->ProcessAttach(storage_info);
+
+  MediaGalleryPrefId id = AddGallery(temp_dir.path());
+  AddAndConfirmWatch(id);
+
+  ShutdownProfile();
+
+  // Trigger a removable storage event that should be ignored now that the
+  // profile has been destroyed to catch regressions. crbug.com/467627
+  base::RunLoop run_loop;
+  storage_monitor()->receiver()->ProcessDetach(storage_info.device_id());
+  run_loop.RunUntilIdle();
+}
+
 }  // namespace component_updater
diff --git a/chrome/browser/media_galleries/media_file_system_registry.cc b/chrome/browser/media_galleries/media_file_system_registry.cc
index 8d3bab8..dbec912 100644
--- a/chrome/browser/media_galleries/media_file_system_registry.cc
+++ b/chrome/browser/media_galleries/media_file_system_registry.cc
@@ -24,6 +24,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/extension_constants.h"
+#include "components/keyed_service/content/browser_context_keyed_service_shutdown_notifier_factory.h"
 #include "components/storage_monitor/media_storage_util.h"
 #include "components/storage_monitor/storage_monitor.h"
 #include "content/public/browser/browser_thread.h"
@@ -51,6 +52,26 @@ using storage_monitor::StorageMonitor;
 
 namespace {
 
+class ShutdownNotifierFactory
+    : public BrowserContextKeyedServiceShutdownNotifierFactory {
+ public:
+  static ShutdownNotifierFactory* GetInstance() {
+    return Singleton<ShutdownNotifierFactory>::get();
+  }
+
+ private:
+  friend struct DefaultSingletonTraits<ShutdownNotifierFactory>;
+
+  ShutdownNotifierFactory()
+      : BrowserContextKeyedServiceShutdownNotifierFactory(
+            "MediaFileSystemRegistry") {
+    DependsOn(MediaGalleriesPreferencesFactory::GetInstance());
+  }
+  ~ShutdownNotifierFactory() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(ShutdownNotifierFactory);
+};
+
 struct InvalidatedGalleriesInfo {
   std::set<ExtensionGalleriesHost*> extension_hosts;
   std::set<MediaGalleryPrefId> pref_ids;
@@ -558,6 +579,12 @@ MediaGalleriesPreferences* MediaFileSystemRegistry::GetPreferences(
   // Create an empty ExtensionHostMap for this profile on first initialization.
   if (!ContainsKey(extension_hosts_map_, profile)) {
     extension_hosts_map_[profile] = ExtensionHostMap();
+    DCHECK(!ContainsKey(profile_subscription_map_, profile));
+    profile_subscription_map_.set(
+        profile,
+        ShutdownNotifierFactory::GetInstance()->Get(profile)->Subscribe(
+            base::Bind(&MediaFileSystemRegistry::OnProfileShutdown,
+                       base::Unretained(this), profile)));
     media_galleries::UsageCount(media_galleries::PROFILES_WITH_USAGE);
   }
 
@@ -845,3 +872,15 @@ void MediaFileSystemRegistry::OnExtensionGalleriesHostEmpty(
     preferences->RemoveGalleryChangeObserver(this);
   }
 }
+
+void MediaFileSystemRegistry::OnProfileShutdown(Profile* profile) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  auto extension_hosts_it = extension_hosts_map_.find(profile);
+  DCHECK(extension_hosts_it != extension_hosts_map_.end());
+  extension_hosts_map_.erase(extension_hosts_it);
+
+  auto profile_subscription_it = profile_subscription_map_.find(profile);
+  DCHECK(profile_subscription_it != profile_subscription_map_.end());
+  profile_subscription_map_.erase(profile_subscription_it);
+}
diff --git a/chrome/browser/media_galleries/media_file_system_registry.h b/chrome/browser/media_galleries/media_file_system_registry.h
index 86440cd..cbeb969 100644
--- a/chrome/browser/media_galleries/media_file_system_registry.h
+++ b/chrome/browser/media_galleries/media_file_system_registry.h
@@ -14,11 +14,13 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/containers/scoped_ptr_map.h"
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/media_galleries/media_galleries_preferences.h"
+#include "components/keyed_service/core/keyed_service_shutdown_notifier.h"
 #include "components/storage_monitor/removable_storage_observer.h"
 
 class ExtensionGalleriesHost;
@@ -111,9 +113,13 @@ class MediaFileSystemRegistry
 
   // Map an extension to the ExtensionGalleriesHost.
   typedef std::map<std::string /*extension_id*/,
-                   scoped_refptr<ExtensionGalleriesHost> > ExtensionHostMap;
+                   scoped_refptr<ExtensionGalleriesHost>> ExtensionHostMap;
   // Map a profile and extension to the ExtensionGalleriesHost.
   typedef std::map<Profile*, ExtensionHostMap> ExtensionGalleriesHostMap;
+  // Map a profile to a shutdown notification subscription.
+  typedef ScopedPtrMap<Profile*,
+                       scoped_ptr<KeyedServiceShutdownNotifier::Subscription>>
+      ProfileSubscriptionMap;
 
   void OnPermissionRemoved(MediaGalleriesPreferences* pref,
                            const std::string& extension_id,
@@ -130,9 +136,15 @@ class MediaFileSystemRegistry
   void OnExtensionGalleriesHostEmpty(Profile* profile,
                                      const std::string& extension_id);
 
+  void OnProfileShutdown(Profile* profile);
+
   // This map owns all the ExtensionGalleriesHost objects created.
   ExtensionGalleriesHostMap extension_hosts_map_;
 
+  // The above map uses raw Profile pointers as keys. This map removes those
+  // entries when the Profile is destroyed.
+  ProfileSubscriptionMap profile_subscription_map_;
+
   scoped_ptr<MediaFileSystemContext> file_system_context_;
 
   scoped_ptr<MediaScanManager> media_scan_manager_;