Media Galleries API: Fix MediaGalleriesPreferences finders race.

chrome/browser/media_galleries/media_file_system_registry.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // MediaFileSystemRegistry implementation.
 
 #include "chrome/browser/media_galleries/media_file_system_registry.h"
 
 #include <set>
 #include <vector>
@@ skipping 420 matching lines @@
 
 void MediaFileSystemRegistry::GetMediaFileSystemsForExtension(
     const content::RenderViewHost* rvh,
     const extensions::Extension* extension,
     const MediaFileSystemsCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   Profile* profile =
       Profile::FromBrowserContext(rvh->GetProcess()->GetBrowserContext());
   MediaGalleriesPreferences* preferences = GetPreferences(profile);
-  MediaGalleryPrefIdSet galleries =
-      preferences->GalleriesForExtension(*extension);
-
-  if (galleries.empty()) {
-    callback.Run(std::vector<MediaFileSystemInfo>());
-    return;
-  }
-
-  ExtensionGalleriesHostMap::iterator extension_hosts =
-      extension_hosts_map_.find(profile);
-  if (extension_hosts->second.empty())
-    preferences->AddGalleryChangeObserver(this);
-
-  ExtensionGalleriesHost* extension_host =
-      extension_hosts->second[extension->id()].get();
-  if (!extension_host) {
-    extension_host = new ExtensionGalleriesHost(
-        file_system_context_.get(),
-        base::Bind(&MediaFileSystemRegistry::OnExtensionGalleriesHostEmpty,
-                   base::Unretained(this), profile, extension->id()));
-    extension_hosts_map_[profile][extension->id()] = extension_host;
-  }
-  extension_host->ReferenceFromRVH(rvh);
-
-  extension_host->GetMediaFileSystems(galleries, preferences->known_galleries(),
-                                      callback);
+  preferences->EnsureInitialized(base::Bind(

[vandebo (ex-Chrome), 2013/09/21 01:20:59]

Are we not already sure that preferences have been initialized?  IIRC, the only
way to get to this code is through the API or the dialog...

[tommycli, 2013/09/23 20:39:12]

I made this assume it was already initialized, and made all the API callsites
initialize the media preferences beforehand.

+      &MediaFileSystemRegistry::FinishGetMediaFileSystemsForExtension,
+      weak_factory_.GetWeakPtr(),
+      rvh,
+      extension,
+      callback));
 }
 
 MediaGalleriesPreferences* MediaFileSystemRegistry::GetPreferences(
     Profile* profile) {
-  MediaGalleriesPreferences* preferences =
-      MediaGalleriesPreferencesFactory::GetForProfile(profile);
-  if (ContainsKey(extension_hosts_map_, profile))
-    return preferences;
+  // Create an empty ExtensionHostMap for this profile on first initialization.
+  if (!ContainsKey(extension_hosts_map_, profile))
+    extension_hosts_map_[profile] = ExtensionHostMap();
 
-  // Create an empty entry so the initialization code below only gets called
-  // once per profile.
-  extension_hosts_map_[profile] = ExtensionHostMap();
-
-  // TODO(gbillock): Move this stanza to MediaGalleriesPreferences init code.
-  StorageMonitor* monitor = StorageMonitor::GetInstance();
-  DCHECK(monitor->IsInitialized());
-  std::vector<StorageInfo> existing_devices =
-      monitor->GetAllAvailableStorages();
-  for (size_t i = 0; i < existing_devices.size(); i++) {
-    if (!(StorageInfo::IsMediaDevice(existing_devices[i].device_id()) &&
-          StorageInfo::IsRemovableDevice(existing_devices[i].device_id())))
-      continue;
-    preferences->AddGallery(existing_devices[i].device_id(),
-                            base::FilePath(),
-                            false,
-                            existing_devices[i].storage_label(),
-                            existing_devices[i].vendor_name(),
-                            existing_devices[i].model_name(),
-                            existing_devices[i].total_size_in_bytes(),
-                            base::Time::Now());
-  }
-  return preferences;
+  return MediaGalleriesPreferencesFactory::GetForProfile(profile);
 }
 
 void MediaFileSystemRegistry::OnRemovableStorageDetached(
     const StorageInfo& info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Since revoking a gallery in the ExtensionGalleriesHost may cause it
   // to be removed from the map and therefore invalidate any iterator pointing
   // to it, this code first copies all the invalid gallery ids and the
   // extension hosts in which they may appear (per profile) and revoked it in
   // a second step.
   std::vector<InvalidatedGalleriesInfo> invalid_galleries_info;
 
   for (ExtensionGalleriesHostMap::iterator profile_it =
            extension_hosts_map_.begin();
        profile_it != extension_hosts_map_.end();
        ++profile_it) {
     MediaGalleriesPreferences* preferences = GetPreferences(profile_it->first);
+    // If |preferences| is not yet initialized, it won't contain any galleries.
+    if (!preferences->IsInitialized())
+      continue;
+
     InvalidatedGalleriesInfo invalid_galleries_in_profile;
     invalid_galleries_in_profile.pref_ids =
         preferences->LookUpGalleriesByDeviceId(info.device_id());
 
     for (ExtensionHostMap::const_iterator extension_host_it =
              profile_it->second.begin();
          extension_host_it != profile_it->second.end();
          ++extension_host_it) {
       invalid_galleries_in_profile.extension_hosts.insert(
           extension_host_it->second.get());
@@ skipping 85 matching lines @@
 
     IsolatedContext::GetInstance()->RevokeFileSystem(fsid);
   }
 
  private:
   MediaFileSystemRegistry* registry_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaFileSystemContextImpl);
 };
 
+// Constructor in 'private' section because depends on private class definition.
 MediaFileSystemRegistry::MediaFileSystemRegistry()
-    : file_system_context_(new MediaFileSystemContextImpl(this)) {
+    : weak_factory_(this),
+      file_system_context_(new MediaFileSystemContextImpl(this)) {
   StorageMonitor::GetInstance()->AddObserver(this);
 }
 
 MediaFileSystemRegistry::~MediaFileSystemRegistry() {
   // TODO(gbillock): This is needed because the unit test uses the
   // g_browser_process registry. We should create one in the unit test,
   // and then can remove this.
   if (StorageMonitor::GetInstance())
     StorageMonitor::GetInstance()->RemoveObserver(this);
   DCHECK(mtp_device_delegate_map_.empty());
 }
 
+void MediaFileSystemRegistry::FinishGetMediaFileSystemsForExtension(
+    const content::RenderViewHost* rvh,
+    const extensions::Extension* extension,
+    const MediaFileSystemsCallback& callback) {
+  Profile* profile =
+      Profile::FromBrowserContext(rvh->GetProcess()->GetBrowserContext());
+  MediaGalleriesPreferences* preferences = GetPreferences(profile);
+  DCHECK(preferences->IsInitialized());
+  MediaGalleryPrefIdSet galleries =
+      preferences->GalleriesForExtension(*extension);
+
+  if (galleries.empty()) {
+    callback.Run(std::vector<MediaFileSystemInfo>());
+    return;
+  }
+
+  ExtensionGalleriesHostMap::iterator extension_hosts =
+      extension_hosts_map_.find(profile);
+  if (extension_hosts->second.empty())
+    preferences->AddGalleryChangeObserver(this);
+
+  ExtensionGalleriesHost* extension_host =
+      extension_hosts->second[extension->id()].get();
+  if (!extension_host) {
+    extension_host = new ExtensionGalleriesHost(
+        file_system_context_.get(),
+        base::Bind(&MediaFileSystemRegistry::OnExtensionGalleriesHostEmpty,
+                   base::Unretained(this),
+                   profile,
+                   extension->id()));
+    extension_hosts_map_[profile][extension->id()] = extension_host;
+  }
+  extension_host->ReferenceFromRVH(rvh);
+
+  extension_host->GetMediaFileSystems(
+      galleries, preferences->known_galleries(), callback);
+}
+
 void MediaFileSystemRegistry::OnPermissionRemoved(
     MediaGalleriesPreferences* prefs,
     const std::string& extension_id,
     MediaGalleryPrefId pref_id) {
   Profile* profile = prefs->profile();
   ExtensionGalleriesHostMap::const_iterator host_map_it =
       extension_hosts_map_.find(profile);
   DCHECK(host_map_it != extension_hosts_map_.end());
   const ExtensionHostMap& extension_host_map = host_map_it->second;
   ExtensionHostMap::const_iterator gallery_host_it =
@@ skipping 80 matching lines @@
   DCHECK(extension_hosts != extension_hosts_map_.end());
   ExtensionHostMap::size_type erase_count =
       extension_hosts->second.erase(extension_id);
   DCHECK_EQ(1U, erase_count);
   if (extension_hosts->second.empty()) {
     // When a profile has no ExtensionGalleriesHosts left, remove the
     // matching gallery-change-watcher since it is no longer needed. Leave the
     // |extension_hosts| entry alone, since it indicates the profile has been
     // previously used.
     MediaGalleriesPreferences* preferences = GetPreferences(profile);
+    DCHECK(preferences->IsInitialized());
     preferences->RemoveGalleryChangeObserver(this);
   }
 }