[fsp] Remember mounted file systems as soon as possible.

chrome/browser/chromeos/file_system_provider/service.cc

 // Copyright 2014 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/chromeos/file_system_provider/service.h"
 
 #include "base/files/file_path.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/stl_util.h"
@@ skipping 35 matching lines @@
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
 Service::Service(Profile* profile,
                  extensions::ExtensionRegistry* extension_registry)
     : profile_(profile),
       extension_registry_(extension_registry),
       file_system_factory_(base::Bind(CreateProvidedFileSystem)),
       weak_ptr_factory_(this) {
   extension_registry_->AddObserver(this);
+
+  // Store previously mounted file systems in order to be able to remount
+  // them once related extensions are loaded.
+  PrefService* pref_service = profile_->GetPrefs();
+  DCHECK(pref_service);
+
+  const base::DictionaryValue* file_systems =
+      pref_service->GetDictionary(prefs::kFileSystemProviderMounted);
+  DCHECK(file_systems);
+
+  file_systems_to_restore_.reset(file_systems->DeepCopy());

[hashimoto, 2014/07/04 06:38:42]

Do we need this code now?
You can read the dictionary from the prefs directly without
file_systems_to_restore_.

[mtomasz, 2014/07/04 07:50:42]

It makes things easier. Note, that In #420 we want to remove the file system
from preferences, while we are iterating. If we didn't have this
|file_systems_to_restore_| member, then we could end up on a segmentation fault
error.

[hashimoto, 2014/07/07 11:07:46]

You can save a list of file systems to be removed while iterating and remove the
file systems after that.
Or you can make a copy of the dictionary just before starting the iteration.

[mtomasz, 2014/07/08 01:52:09]

Done.

 }
 
 Service::~Service() {
   extension_registry_->RemoveObserver(this);
-  RememberFileSystems();
 
+  // Provided file systems should be already unmounted because of receiving
+  // OnExtensionUnload calls for each installed extension. However, for tests
+  // we may still have mounted extensions.
   ProvidedFileSystemMap::iterator it = file_system_map_.begin();
   while (it != file_system_map_.end()) {
     const std::string file_system_id =
         it->second->GetFileSystemInfo().file_system_id();
     const std::string extension_id =
         it->second->GetFileSystemInfo().extension_id();
     ++it;
-    const bool unmount_result = UnmountFileSystem(extension_id, file_system_id);
+    const bool unmount_result = UnmountFileSystem(
+        extension_id, file_system_id, UNMOUNT_REASON_SHUTDOWN);
     DCHECK(unmount_result);
   }
 
   DCHECK_EQ(0u, file_system_map_.size());
   STLDeleteValues(&file_system_map_);
 }
 
 // static
 Service* Service::Get(content::BrowserContext* context) {
   return ServiceFactory::Get(context);
@@ skipping 70 matching lines @@
   //   mount_path = /provided/b33f1337-41-5aa5
   ProvidedFileSystemInfo file_system_info(
       extension_id, file_system_id, file_system_name, mount_path);
 
   ProvidedFileSystemInterface* file_system =
       file_system_factory_.Run(profile_, file_system_info);
   DCHECK(file_system);
   file_system_map_[FileSystemKey(extension_id, file_system_id)] = file_system;
   mount_point_name_to_key_map_[mount_point_name] =
       FileSystemKey(extension_id, file_system_id);
+  RememberFileSystem(file_system_info);
 
   FOR_EACH_OBSERVER(
       Observer,
       observers_,
       OnProvidedFileSystemMount(file_system_info, base::File::FILE_OK));
 
   return true;
 }
 
 bool Service::UnmountFileSystem(const std::string& extension_id,
-                                const std::string& file_system_id) {
+                                const std::string& file_system_id,
+                                UnmountReason reason) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   const ProvidedFileSystemMap::iterator file_system_it =
       file_system_map_.find(FileSystemKey(extension_id, file_system_id));
   if (file_system_it == file_system_map_.end()) {
     const ProvidedFileSystemInfo empty_file_system_info;
     FOR_EACH_OBSERVER(
         Observer,
         observers_,
         OnProvidedFileSystemUnmount(empty_file_system_info,
@@ skipping 19 matching lines @@
     return false;
   }
 
   FOR_EACH_OBSERVER(
       Observer,
       observers_,
       OnProvidedFileSystemUnmount(file_system_info, base::File::FILE_OK));
 
   mount_point_name_to_key_map_.erase(mount_point_name);
 
+  if (reason == UNMOUNT_REASON_USER)

[hashimoto, 2014/07/04 06:38:42]

multiple lines should be accommodated with {}.

[mtomasz, 2014/07/04 07:50:42]

Done.

+    ForgetFileSystem(file_system_info.extension_id(),
+                     file_system_info.file_system_id());
+
   delete file_system_it->second;
   file_system_map_.erase(file_system_it);
 
   return true;
 }
 
 bool Service::RequestUnmount(const std::string& extension_id,
                              const std::string& file_system_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
@@ skipping 31 matching lines @@
   if (file_system_it == file_system_map_.end())
     return NULL;
 
   return file_system_it->second;
 }
 
 void Service::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const extensions::Extension* extension,
     extensions::UnloadedExtensionInfo::Reason reason) {
-  // If the reason is not a profile shutdown, then forget the mounted file
-  // systems from preferences.
-  if (reason != extensions::UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN)
-    ForgetFileSystems(extension->id());
-
   // Unmount all of the provided file systems associated with this extension.
   ProvidedFileSystemMap::iterator it = file_system_map_.begin();
   while (it != file_system_map_.end()) {
     const ProvidedFileSystemInfo& file_system_info =
         it->second->GetFileSystemInfo();
     // Advance the iterator beforehand, otherwise it will become invalidated
     // by the UnmountFileSystem() call.
     ++it;
     if (file_system_info.extension_id() == extension->id()) {
       const bool unmount_result = UnmountFileSystem(
-          file_system_info.extension_id(), file_system_info.file_system_id());
+          file_system_info.extension_id(),
+          file_system_info.file_system_id(),
+          reason == extensions::UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN
+              ? UNMOUNT_REASON_SHUTDOWN
+              : UNMOUNT_REASON_USER);
       DCHECK(unmount_result);
     }
   }
 }
 
 void Service::OnExtensionLoaded(content::BrowserContext* browser_context,
                                 const extensions::Extension* extension) {
   RestoreFileSystems(extension->id());
 }
 
@@ skipping 20 matching lines @@
   // Notify observers about failure in unmounting, since mount() will not be
   // called by the provided file system. In case of success mount() will be
   // invoked, and observers notified, so there is no need to call them now.
   if (error != base::File::FILE_OK) {
     FOR_EACH_OBSERVER(Observer,
                       observers_,
                       OnProvidedFileSystemUnmount(file_system_info, error));
   }
 }
 
-void Service::RememberFileSystems() {
-  base::DictionaryValue extensions;
-  const std::vector<ProvidedFileSystemInfo> file_system_info_list =
-      GetProvidedFileSystemInfoList();
-
-  for (std::vector<ProvidedFileSystemInfo>::const_iterator it =
-           file_system_info_list.begin();
-       it != file_system_info_list.end();
-       ++it) {
-    base::ListValue* file_systems = NULL;
-    if (!extensions.GetList(it->extension_id(), &file_systems)) {
-      file_systems = new base::ListValue();
-      extensions.Set(it->extension_id(), file_systems);
-    }
-
-    base::DictionaryValue* file_system = new base::DictionaryValue();
-    file_system->SetString(kPrefKeyFileSystemId, it->file_system_id());
-    file_system->SetString(kPrefKeyFileSystemName, it->file_system_name());
-    file_systems->Append(file_system);
-  }
+void Service::RememberFileSystem(
+    const ProvidedFileSystemInfo& file_system_info) {
+  base::DictionaryValue* file_system = new base::DictionaryValue();
+  file_system->SetString(kPrefKeyFileSystemId,
+                         file_system_info.file_system_id());
+  file_system->SetString(kPrefKeyFileSystemName,
+                         file_system_info.file_system_name());
 
   PrefService* pref_service = profile_->GetPrefs();
   DCHECK(pref_service);
-  pref_service->Set(prefs::kFileSystemProviderMounted, extensions);
+
+  DictionaryPrefUpdate dict_update(pref_service,
+                                   prefs::kFileSystemProviderMounted);
+
+  base::DictionaryValue* file_systems_per_extension = NULL;
+  if (!dict_update->GetDictionary(file_system_info.extension_id(),
+                                  &file_systems_per_extension)) {
+    file_systems_per_extension = new base::DictionaryValue();
+    dict_update->Set(file_system_info.extension_id(),
+                     file_systems_per_extension);
+  }
+
+  file_systems_per_extension->SetWithoutPathExpansion(
+      file_system_info.file_system_id(), file_system);
   pref_service->CommitPendingWrite();

[hashimoto, 2014/07/04 06:38:42]

pref_service.h says CommitPendingWrite() should be used only during shutdown.
Do we need this now?

[mtomasz, 2014/07/04 07:50:42]

If a crash happens, then we don't want to lose mounted file systems. However, it
seems that the current pref service implementation flushes preferences every 10
seconds after a change. I think it is fast enough.

Removed. Done.

 }
 
-void Service::ForgetFileSystems(const std::string& extension_id) {
+void Service::ForgetFileSystem(const std::string& extension_id,
+                               const std::string& file_system_id) {
   PrefService* pref_service = profile_->GetPrefs();
   DCHECK(pref_service);
 
-  DictionaryPrefUpdate update(pref_service, prefs::kFileSystemProviderMounted);
-  base::DictionaryValue* extensions = update.Get();
-  DCHECK(extensions);
+  DictionaryPrefUpdate dict_update(pref_service,
+                                   prefs::kFileSystemProviderMounted);
 
-  extensions->Remove(extension_id, NULL);
+  base::DictionaryValue* file_systems_per_extension = NULL;
+  if (!dict_update->GetDictionary(extension_id, &file_systems_per_extension)) {
+    return;  // Nothing to forget.
+  }
+
+  file_systems_per_extension->RemoveWithoutPathExpansion(file_system_id, NULL);
+  if (!file_systems_per_extension->size())
+    dict_update->Remove(extension_id, NULL);
+
+  pref_service->CommitPendingWrite();

[hashimoto, 2014/07/04 06:38:41]

ditto.

[mtomasz, 2014/07/04 07:50:42]

Done.

 }
 
 void Service::RestoreFileSystems(const std::string& extension_id) {
-  PrefService* pref_service = profile_->GetPrefs();
-  DCHECK(pref_service);
+  const base::DictionaryValue* file_systems_per_extension = NULL;
+  if (!file_systems_to_restore_->GetDictionary(extension_id,
+                                               &file_systems_per_extension)) {
+    return;  // Nothing to restore.
+  }
 
-  const base::DictionaryValue* extensions =
-      pref_service->GetDictionary(prefs::kFileSystemProviderMounted);
-  DCHECK(extensions);
-
-  const base::ListValue* file_systems = NULL;
-
-  if (!extensions->GetList(extension_id, &file_systems))
-    return;
-
-  for (size_t i = 0; i < file_systems->GetSize(); ++i) {
+  for (base::DictionaryValue::Iterator it(*file_systems_per_extension);
+       !it.IsAtEnd();
+       it.Advance()) {
+    const base::Value* file_system_value = NULL;
     const base::DictionaryValue* file_system = NULL;
-    file_systems->GetDictionary(i, &file_system);
-    DCHECK(file_system);
+    file_systems_per_extension->GetWithoutPathExpansion(it.key(),
+                                                        &file_system_value);
+    DCHECK(file_system_value);
 
     std::string file_system_id;
-    file_system->GetString(kPrefKeyFileSystemId, &file_system_id);
-    DCHECK(!file_system_id.empty());
-
     std::string file_system_name;
-    file_system->GetString(kPrefKeyFileSystemName, &file_system_name);
-    DCHECK(!file_system_name.empty());
+    if (file_system_value->GetAsDictionary(&file_system)) {
+      file_system->GetString(kPrefKeyFileSystemId, &file_system_id);
+      file_system->GetString(kPrefKeyFileSystemName, &file_system_name);
+    }
 
     if (file_system_id.empty() || file_system_name.empty()) {
       LOG(ERROR)
           << "Malformed provided file system information in preferences.";
       continue;
     }
 
     const bool result =
         MountFileSystem(extension_id, file_system_id, file_system_name);
     if (!result) {
       LOG(ERROR) << "Failed to restore a provided file system from "
                  << "preferences: " << extension_id << ", " << file_system_id
                  << ", " << file_system_name << ".";
+      // In case of remounting during startup, forget the file system from
+      // preferences.
+      ForgetFileSystem(extension_id, file_system_id);
     }
   }
+
+  file_systems_to_restore_->Remove(extension_id, NULL);
 }
 
 }  // namespace file_system_provider
 }  // namespace chromeos