diff options
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_; |