Media Galleries API: Fix MediaGalleriesPreferences finders race.

chrome/browser/media_galleries/media_galleries_preferences.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.
 
 #include "chrome/browser/media_galleries/media_galleries_preferences.h"
 
+#include "base/callback.h"
 #include "base/i18n/time_formatting.h"
 #include "base/path_service.h"
 #include "base/prefs/pref_service.h"
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/media_galleries/fileapi/itunes_finder.h"
 #include "chrome/browser/media_galleries/fileapi/picasa_finder.h"
 #include "chrome/browser/media_galleries/media_file_system_registry.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/storage_monitor/media_storage_util.h"
 #include "chrome/browser/storage_monitor/storage_monitor.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/permissions/api_permission.h"
 #include "chrome/common/extensions/permissions/media_galleries_permission.h"
 #include "chrome/common/extensions/permissions/permissions_data.h"
 #include "chrome/common/pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
+#include "content/public/browser/browser_thread.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/text/bytes_formatting.h"
 
 using base::DictionaryValue;
 using base::ListValue;
 using extensions::ExtensionPrefs;
 
 namespace {
 
@@ skipping 282 matching lines @@
 
 bool MediaGalleryPrefInfo::IsGalleryAvailable() const {
   return !StorageInfo::IsRemovableDevice(device_id) ||
          MediaStorageUtil::IsRemovableStorageAttached(device_id);
 }
 
 MediaGalleriesPreferences::GalleryChangeObserver::~GalleryChangeObserver() {}
 
 MediaGalleriesPreferences::MediaGalleriesPreferences(Profile* profile)
     : weak_factory_(this),
+      initialized_(false),
+      pre_initialization_callbacks_waiting_(0),
       profile_(profile),
-      extension_prefs_for_testing_(NULL) {
-  AddDefaultGalleriesIfFreshProfile();
-
-  // Look for optional default galleries every time.
-  itunes::ITunesFinder::FindITunesLibrary(
-      base::Bind(&MediaGalleriesPreferences::OnITunesDeviceID,
-                 weak_factory_.GetWeakPtr()));
-
-  // TODO(tommycli): Turn on when Picasa code is ready.
-#if 0
-  picasa::PicasaFinder::FindPicasaDatabaseOnUIThread(
-      base::Bind(&MediaGalleriesPreferences::OnPicasaDeviceID,
-                 weak_factory_.GetWeakPtr()));
-#endif
-
-  InitFromPrefs();
-
-  StorageMonitor::GetInstance()->AddObserver(this);
-}
+      extension_prefs_for_testing_(NULL) {}
 
 MediaGalleriesPreferences::~MediaGalleriesPreferences() {
   if (StorageMonitor::GetInstance())
     StorageMonitor::GetInstance()->RemoveObserver(this);
 }
 
-Profile* MediaGalleriesPreferences::profile() {
-  return profile_;
+void MediaGalleriesPreferences::EnsureInitialized(base::Closure callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (IsInitialized()) {
+    if (!callback.is_null())
+      callback.Run();
+    return;
+  }
+
+  on_initialize_callbacks_.push_back(callback);
+  if (on_initialize_callbacks_.size() > 1)
+    return;
+
+  AddDefaultGalleriesIfFreshProfile();
+
+  pre_initialization_callbacks_waiting_ = 2;

[vandebo (ex-Chrome), 2013/09/26 22:42:36]

nit: I think it would be better to increment this on the line just before the
callback is passed in, i.e. 364, 369.  It would make it clearer where the number
is coming from and harder for it to get out of sync.

[tommycli, 2013/09/26 23:53:33]

So this is ... haha...

I originally incremented it.

Unfortunately, StorageMonitor was already ready and called the callback
immediately, decrementing it to zero and calling FinishInitialization
immediately...

The 'real' solution is probably not not use a simple integer like this and use
Scheduler. (https://codereview.chromium.org/24298002/)

[vandebo (ex-Chrome), 2013/09/27 17:07:27]

I see.  Please add a comment explaining that this has to match the number of
things dispatched below and why you can't do it inline like I suggested.

[tommycli, 2013/09/27 17:54:46]

Done.

+
+  // Ensure StorageMonitor is initialized.
+  StorageMonitor::GetInstance()->EnsureInitialized(
+      base::Bind(&MediaGalleriesPreferences::OnInitializationCallbackReturned,
+                 weak_factory_.GetWeakPtr()));
+
+  // Look for optional default galleries every time.
+  itunes::ITunesFinder::FindITunesLibrary(
+      base::Bind(&MediaGalleriesPreferences::OnFinderDeviceID,
+                 weak_factory_.GetWeakPtr()));
+
+  // TODO(tommycli): Turn on when Picasa code is ready.
+#if 0
+  picasa::PicasaFinder::FindPicasaDatabaseOnUIThread(
+      base::Bind(&MediaGalleriesPreferences::OnFinderDeviceID,
+                 weak_factory_.GetWeakPtr()));
+#endif
+}
+
+bool MediaGalleriesPreferences::IsInitialized() const { return initialized_; }
+
+Profile* MediaGalleriesPreferences::profile() { return profile_; }
+
+void MediaGalleriesPreferences::OnInitializationCallbackReturned() {
+  DCHECK(!IsInitialized());
+  DCHECK(pre_initialization_callbacks_waiting_ > 0);
+  if (--pre_initialization_callbacks_waiting_ == 0)
+    FinishInitialization();
+}
+
+void MediaGalleriesPreferences::FinishInitialization() {
+  DCHECK(!IsInitialized());
+
+  initialized_ = true;
+
+  StorageMonitor* monitor = StorageMonitor::GetInstance();
+  DCHECK(monitor->IsInitialized());
+
+  InitFromPrefs();
+
+  StorageMonitor::GetInstance()->AddObserver(this);
+
+  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;
+    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());
+  }
+
+  for (std::vector<base::Closure>::iterator iter =
+           on_initialize_callbacks_.begin();
+       iter != on_initialize_callbacks_.end();
+       ++iter) {
+    iter->Run();
+  }
+  on_initialize_callbacks_.clear();
 }
 
 void MediaGalleriesPreferences::AddDefaultGalleriesIfFreshProfile() {
   // Only add defaults the first time.
   if (APIHasBeenUsed(profile_))
     return;
 
   // Fresh profile case.
   const int kDirectoryKeys[] = {
     chrome::DIR_USER_MUSIC,
@@ skipping 50 matching lines @@
         FOR_EACH_OBSERVER(GalleryChangeObserver,
                           gallery_change_observers_,
                           OnGalleryInfoUpdated(this, pref_id));
       }
       return true;
     }
   }
   return false;
 }
 
-void MediaGalleriesPreferences::OnITunesDeviceID(const std::string& device_id) {
-  if (device_id.empty())
-    return;
-  if (!UpdateDeviceIDForSingletonType(device_id)) {
-    AddGalleryInternal(device_id, ASCIIToUTF16(kITunesGalleryName),
+void MediaGalleriesPreferences::OnFinderDeviceID(const std::string& device_id) {
+  if (!device_id.empty() && !UpdateDeviceIDForSingletonType(device_id)) {
+    std::string gallery_name;
+    if (StorageInfo::IsITunesDevice(device_id))
+      gallery_name = kITunesGalleryName;
+    else if (StorageInfo::IsPicasaDevice(device_id))
+      gallery_name = kPicasaGalleryName;
+    else
+      NOTREACHED();
+
+    AddGalleryInternal(device_id, ASCIIToUTF16(gallery_name),
                        base::FilePath(), false /*not user added*/,
                        string16(), string16(), string16(), 0,
                        base::Time(), false, 2);
   }
-}
 
-void MediaGalleriesPreferences::OnPicasaDeviceID(const std::string& device_id) {
-  DCHECK(!device_id.empty());
-  if (!UpdateDeviceIDForSingletonType(device_id)) {
-    AddGalleryInternal(device_id, ASCIIToUTF16(kPicasaGalleryName),
-                       base::FilePath(), false /*not user added*/,
-                       string16(), string16(), string16(), 0,
-                       base::Time(), false, 2);
-  }
+  OnInitializationCallbackReturned();
 }
 
 void MediaGalleriesPreferences::InitFromPrefs() {
   known_galleries_.clear();
   device_map_.clear();
 
   PrefService* prefs = profile_->GetPrefs();
   const ListValue* list = prefs->GetList(
       prefs::kMediaGalleriesRememberedGalleries);
   if (list) {
     for (ListValue::const_iterator it = list->begin();
          it != list->end(); ++it) {
       const DictionaryValue* dict = NULL;
       if (!(*it)->GetAsDictionary(&dict))
         continue;
 
       MediaGalleryPrefInfo gallery_info;
       if (!PopulateGalleryPrefInfoFromDictionary(*dict, &gallery_info))
         continue;
 
       known_galleries_[gallery_info.pref_id] = gallery_info;
       device_map_[gallery_info.device_id].insert(gallery_info.pref_id);
     }
   }
 }
 
 void MediaGalleriesPreferences::AddGalleryChangeObserver(
     GalleryChangeObserver* observer) {
+  DCHECK(IsInitialized());
   gallery_change_observers_.AddObserver(observer);
 }
 
 void MediaGalleriesPreferences::RemoveGalleryChangeObserver(
     GalleryChangeObserver* observer) {
+  DCHECK(IsInitialized());
   gallery_change_observers_.RemoveObserver(observer);
 }
 
 void MediaGalleriesPreferences::OnRemovableStorageAttached(
     const StorageInfo& info) {
+  DCHECK(IsInitialized());
   if (!StorageInfo::IsMediaDevice(info.device_id()))
     return;
 
   AddGallery(info.device_id(), base::FilePath(),
              false /*not user added*/,
              info.storage_label(),
              info.vendor_name(),
              info.model_name(),
              info.total_size_in_bytes(),
              base::Time::Now());
 }
 
 bool MediaGalleriesPreferences::LookUpGalleryByPath(
     const base::FilePath& path,
     MediaGalleryPrefInfo* gallery_info) const {
+  DCHECK(IsInitialized());
   StorageInfo info;
   base::FilePath relative_path;
   if (!MediaStorageUtil::GetDeviceInfoFromPath(path, &info, &relative_path)) {
     if (gallery_info)
       *gallery_info = MediaGalleryPrefInfo();
     return false;
   }
 
   relative_path = relative_path.NormalizePathSeparators();
   MediaGalleryPrefIdSet galleries_on_device =
@@ skipping 38 matching lines @@
   DeviceIdPrefIdsMap::const_iterator found = device_map_.find(device_id);
   if (found == device_map_.end())
     return MediaGalleryPrefIdSet();
   return found->second;
 }
 
 base::FilePath MediaGalleriesPreferences::LookUpGalleryPathForExtension(
     MediaGalleryPrefId gallery_id,
     const extensions::Extension* extension,
     bool include_unpermitted_galleries) {
+  DCHECK(IsInitialized());
   DCHECK(extension);
   if (!include_unpermitted_galleries &&
       !ContainsKey(GalleriesForExtension(*extension), gallery_id))
     return base::FilePath();
 
   MediaGalleriesPrefInfoMap::const_iterator it =
       known_galleries_.find(gallery_id);
   if (it == known_galleries_.end())
     return base::FilePath();
   return MediaStorageUtil::FindDevicePathById(it->second.device_id);
 }
 
 MediaGalleryPrefId MediaGalleriesPreferences::AddGallery(
     const std::string& device_id,
     const base::FilePath& relative_path, bool user_added,
     const string16& volume_label, const string16& vendor_name,
     const string16& model_name, uint64 total_size_in_bytes,
     base::Time last_attach_time) {
+  DCHECK(IsInitialized());
   return AddGalleryInternal(device_id, string16(), relative_path, user_added,
                             volume_label, vendor_name, model_name,
                             total_size_in_bytes, last_attach_time, true, 2);
 }
 
 MediaGalleryPrefId MediaGalleriesPreferences::AddGalleryInternal(
     const std::string& device_id, const string16& display_name,
     const base::FilePath& relative_path, bool user_added,
     const string16& volume_label, const string16& vendor_name,
     const string16& model_name, uint64 total_size_in_bytes,
@@ skipping 112 matching lines @@
   InitFromPrefs();
   FOR_EACH_OBSERVER(GalleryChangeObserver,
                     gallery_change_observers_,
                     OnGalleryAdded(this, gallery_info.pref_id));
 
   return gallery_info.pref_id;
 }
 
 MediaGalleryPrefId MediaGalleriesPreferences::AddGalleryByPath(
     const base::FilePath& path) {
+  DCHECK(IsInitialized());
   MediaGalleryPrefInfo gallery_info;
   if (LookUpGalleryByPath(path, &gallery_info) &&
       gallery_info.type != MediaGalleryPrefInfo::kBlackListed) {
     return gallery_info.pref_id;
   }
   return AddGalleryInternal(gallery_info.device_id,
                             gallery_info.display_name,
                             gallery_info.path,
                             true /*user added*/,
                             gallery_info.volume_label,
                             gallery_info.vendor_name,
                             gallery_info.model_name,
                             gallery_info.total_size_in_bytes,
                             gallery_info.last_attach_time,
                             gallery_info.volume_metadata_valid,
                             gallery_info.prefs_version);
 }
 
 void MediaGalleriesPreferences::ForgetGalleryById(MediaGalleryPrefId pref_id) {
+  DCHECK(IsInitialized());
   PrefService* prefs = profile_->GetPrefs();
   scoped_ptr<ListPrefUpdate> update(new ListPrefUpdate(
       prefs, prefs::kMediaGalleriesRememberedGalleries));
   ListValue* list = update->Get();
 
   if (!ContainsKey(known_galleries_, pref_id))
     return;
 
   for (ListValue::iterator iter = list->begin(); iter != list->end(); ++iter) {
     DictionaryValue* dict;
@@ skipping 15 matching lines @@
       FOR_EACH_OBSERVER(GalleryChangeObserver,
                         gallery_change_observers_,
                         OnGalleryRemoved(this, pref_id));
       return;
     }
   }
 }
 
 MediaGalleryPrefIdSet MediaGalleriesPreferences::GalleriesForExtension(
     const extensions::Extension& extension) const {
+  DCHECK(IsInitialized());
   MediaGalleryPrefIdSet result;
 
   if (HasAutoDetectedGalleryPermission(extension)) {
     for (MediaGalleriesPrefInfoMap::const_iterator it =
              known_galleries_.begin(); it != known_galleries_.end(); ++it) {
       if (it->second.type == MediaGalleryPrefInfo::kAutoDetected)
         result.insert(it->second.pref_id);
     }
   }
 
@@ skipping 14 matching lines @@
       }
     }
   }
   return result;
 }
 
 void MediaGalleriesPreferences::SetGalleryPermissionForExtension(
     const extensions::Extension& extension,
     MediaGalleryPrefId pref_id,
     bool has_permission) {
+  DCHECK(IsInitialized());
   // The gallery may not exist anymore if the user opened a second config
   // surface concurrently and removed it. Drop the permission update if so.
   MediaGalleriesPrefInfoMap::const_iterator gallery_info =
       known_galleries_.find(pref_id);
   if (gallery_info == known_galleries_.end())
     return;
 
   bool default_permission = false;
   if (gallery_info->second.type == MediaGalleryPrefInfo::kAutoDetected)
     default_permission = HasAutoDetectedGalleryPermission(extension);
   // When the permission matches the default, we don't need to remember it.
   if (has_permission == default_permission) {
     if (!UnsetGalleryPermissionInPrefs(extension.id(), pref_id))
       // If permission wasn't set, assume nothing has changed.
       return;
   } else {
     if (!SetGalleryPermissionInPrefs(extension.id(), pref_id, has_permission))
       return;
   }
   if (has_permission)
     FOR_EACH_OBSERVER(GalleryChangeObserver,
                       gallery_change_observers_,
                       OnPermissionAdded(this, extension.id(), pref_id));
   else
     FOR_EACH_OBSERVER(GalleryChangeObserver,
                       gallery_change_observers_,
                       OnPermissionRemoved(this, extension.id(), pref_id));
 }
 
+const MediaGalleriesPrefInfoMap& MediaGalleriesPreferences::known_galleries()
+    const {
+  DCHECK(IsInitialized());
+  return known_galleries_;
+}
+
 void MediaGalleriesPreferences::Shutdown() {
   weak_factory_.InvalidateWeakPtrs();
   profile_ = NULL;
 }
 
 // static
 bool MediaGalleriesPreferences::APIHasBeenUsed(Profile* profile) {
   MediaGalleryPrefId current_id =
       profile->GetPrefs()->GetUint64(prefs::kMediaGalleriesUniqueId);
   return current_id != kInvalidMediaGalleryPrefId + 1;
 }
 
 // static
 void MediaGalleriesPreferences::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterListPref(prefs::kMediaGalleriesRememberedGalleries,
                              user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
   registry->RegisterUint64Pref(
       prefs::kMediaGalleriesUniqueId,
       kInvalidMediaGalleryPrefId + 1,
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
 bool MediaGalleriesPreferences::SetGalleryPermissionInPrefs(
     const std::string& extension_id,
     MediaGalleryPrefId gallery_id,
     bool has_access) {
+  DCHECK(IsInitialized());
   ExtensionPrefs::ScopedListUpdate update(GetExtensionPrefs(),
                                           extension_id,
                                           kMediaGalleriesPermissions);
   ListValue* permissions = update.Get();
   if (!permissions) {
     permissions = update.Create();
   } else {
     // If the gallery is already in the list, update the permission...
     for (ListValue::iterator iter = permissions->begin();
          iter != permissions->end(); ++iter) {
@@ skipping 17 matching lines @@
   DictionaryValue* dict = new DictionaryValue;
   dict->SetString(kMediaGalleryIdKey, base::Uint64ToString(gallery_id));
   dict->SetBoolean(kMediaGalleryHasPermissionKey, has_access);
   permissions->Append(dict);
   return true;
 }
 
 bool MediaGalleriesPreferences::UnsetGalleryPermissionInPrefs(
     const std::string& extension_id,
     MediaGalleryPrefId gallery_id) {
+  DCHECK(IsInitialized());
   ExtensionPrefs::ScopedListUpdate update(GetExtensionPrefs(),
                                           extension_id,
                                           kMediaGalleriesPermissions);
   ListValue* permissions = update.Get();
   if (!permissions)
     return false;
 
   for (ListValue::iterator iter = permissions->begin();
        iter != permissions->end(); ++iter) {
     const DictionaryValue* dict = NULL;
     if (!(*iter)->GetAsDictionary(&dict))
       continue;
     MediaGalleryPermission perm;
     if (!GetMediaGalleryPermissionFromDictionary(dict, &perm))
       continue;
     if (perm.pref_id == gallery_id) {
       permissions->Erase(iter, NULL);
       return true;
     }
   }
   return false;
 }
 
 std::vector<MediaGalleryPermission>
 MediaGalleriesPreferences::GetGalleryPermissionsFromPrefs(
     const std::string& extension_id) const {
+  DCHECK(IsInitialized());
   std::vector<MediaGalleryPermission> result;
   const ListValue* permissions;
   if (!GetExtensionPrefs()->ReadPrefAsList(extension_id,
                                            kMediaGalleriesPermissions,
                                            &permissions)) {
     return result;
   }
 
   for (ListValue::const_iterator iter = permissions->begin();
        iter != permissions->end(); ++iter) {
     DictionaryValue* dict = NULL;
     if (!(*iter)->GetAsDictionary(&dict))
       continue;
     MediaGalleryPermission perm;
     if (!GetMediaGalleryPermissionFromDictionary(dict, &perm))
       continue;
     result.push_back(perm);
   }
 
   return result;
 }
 
 void MediaGalleriesPreferences::RemoveGalleryPermissionsFromPrefs(
     MediaGalleryPrefId gallery_id) {
+  DCHECK(IsInitialized());
   ExtensionPrefs* prefs = GetExtensionPrefs();
   const DictionaryValue* extensions =
       prefs->pref_service()->GetDictionary(prefs::kExtensionsPref);
   if (!extensions)
     return;
 
   for (DictionaryValue::Iterator iter(*extensions); !iter.IsAtEnd();
        iter.Advance()) {
     if (!extensions::Extension::IdIsValid(iter.key())) {
       NOTREACHED();
       continue;
     }
     UnsetGalleryPermissionInPrefs(iter.key(), gallery_id);
   }
 }
 
 ExtensionPrefs* MediaGalleriesPreferences::GetExtensionPrefs() const {
+  DCHECK(IsInitialized());
   if (extension_prefs_for_testing_)
     return extension_prefs_for_testing_;
   return extensions::ExtensionPrefs::Get(profile_);
 }
 
 void MediaGalleriesPreferences::SetExtensionPrefsForTesting(
     extensions::ExtensionPrefs* extension_prefs) {
+  DCHECK(IsInitialized());
   extension_prefs_for_testing_ = extension_prefs;
 }