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

chrome/browser/chromeos/file_system_provider/service.cc

Index: chrome/browser/chromeos/file_system_provider/service.cc
diff --git a/chrome/browser/chromeos/file_system_provider/service.cc b/chrome/browser/chromeos/file_system_provider/service.cc
index 66c7df347a5b9c5a5a45198db76777334974453c..0527cac200b99dd5d22f91c1d4aed9c1ade66262 100644
--- a/chrome/browser/chromeos/file_system_provider/service.cc
+++ b/chrome/browser/chromeos/file_system_provider/service.cc
@@ -53,11 +53,23 @@ Service::Service(Profile* profile,
       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());
+  pref_service->ClearPref(prefs::kFileSystemProviderMounted);

[hashimoto, 2014/07/02 08:59:37]

Is it OK to clear the pref at this point?
Extensions may not be loaded if a crash happens, or the batter becomes empty.

[mtomasz, 2014/07/03 05:24:07]

Done.

+  pref_service->CommitPendingWrite();
 }
 
 Service::~Service() {
   extension_registry_->RemoveObserver(this);
-  RememberFileSystems();
 
   ProvidedFileSystemMap::iterator it = file_system_map_.begin();
   while (it != file_system_map_.end()) {
@@ -66,7 +78,7 @@ Service::~Service() {
     const std::string extension_id =
         it->second->GetFileSystemInfo().extension_id();
     ++it;
-    UnmountFileSystem(extension_id, file_system_id);
+    UnmountFileSystem(extension_id, file_system_id, UNMOUNT_MODE_SHUTDOWN);
   }
 
   DCHECK_EQ(0u, file_system_map_.size());
@@ -156,6 +168,7 @@ bool Service::MountFileSystem(const std::string& extension_id,
   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,
@@ -166,7 +179,8 @@ bool Service::MountFileSystem(const std::string& extension_id,
 }
 
 bool Service::UnmountFileSystem(const std::string& extension_id,
-                                const std::string& file_system_id) {
+                                const std::string& file_system_id,
+                                UnmountMode mode) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   const ProvidedFileSystemMap::iterator file_system_it =
@@ -206,6 +220,9 @@ bool Service::UnmountFileSystem(const std::string& extension_id,
 
   mount_point_name_to_key_map_.erase(mount_point_name);
 
+  if (mode == UNMOUNT_MODE_USER)
+    ForgetFileSystem(file_system_info);
+
   delete file_system_it->second;
   file_system_map_.erase(file_system_it);
 
@@ -257,11 +274,6 @@ 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()) {
@@ -271,8 +283,12 @@ void Service::OnExtensionUnloaded(
     // by the UnmountFileSystem() call.
     ++it;
     if (file_system_info.extension_id() == extension->id()) {
-      bool result = UnmountFileSystem(file_system_info.extension_id(),
-                                      file_system_info.file_system_id());
+      bool result = UnmountFileSystem(
+          file_system_info.extension_id(),
+          file_system_info.file_system_id(),
+          reason == extensions::UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN

[hashimoto, 2014/07/02 08:59:37]

If this method gets called during shutdown, is it needed to call
UnmountFileSystem() in the dtor?

[mtomasz, 2014/07/03 05:24:07]

That's correct but but not in case of unit tests. I'd like to clean it up in a
separate patch, since there is no quick fix for it.

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

Please add a TODO comment to remove the redundant code.

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

Done.

+              ? UNMOUNT_MODE_SHUTDOWN
+              : UNMOUNT_MODE_USER);
       DCHECK(result);
     }
   }
@@ -313,69 +329,75 @@ void Service::OnRequestUnmountStatus(
   }
 }
 
-void Service::RememberFileSystems() {
-  base::DictionaryValue extensions;
-  const std::vector<ProvidedFileSystemInfo> file_system_info_list =
-      GetProvidedFileSystemInfoList();
+void Service::RememberFileSystem(
+    const ProvidedFileSystemInfo& file_system_info) {
+  base::DictionaryValue* file_system = new base::DictionaryValue();
+  file_system->SetString(kPrefKeyFileSystemId,

[hashimoto, 2014/07/02 08:59:37]

Please use one of "FooWithoutPathExpansion" or "Foo" variants exclusively.
("WithoutPathExpansion" is better)

[mtomasz, 2014/07/03 05:24:07]

Currently I'm using WithoutPathExpansion only when the key can contain a dot.
Using it everywhere would make the code bloated.

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

The problem is that nothing can prevent you and other developers from using a
wrong variant when adding a new pref member.
I don't think we should make the product code bug-prone just to save a handful
of key strokes.
Please use WithoutPathExpansion variants everywhere.

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

Do you want me to use WithoutPathExpansion variants in the entire File System
Provider API code for every base::DictionaryValue?

There are two variants of methods, one with expansion and one without. I don't
see reason why not using both of them depending on the situation.

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

What Set/Get does is Set/GetWithoutPathExpansion + path expansion.
When you don't need the path expansion functionality, you should use the
WithoutPathExpansion variant.

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

According to code search's xrefs:
DictionaryValue::SetStringWithoutPathExpansion() 176 calls
DictionaryValue::SetString() 2852 calls

But since you prefer the longer version, fixed. Done.

[hashimoto, 2014/07/09 08:36:39]

So what?
Generally speaking, WithoutPathExpansion() variants should be used when the path
expansion behavior is not desired.
Here, the path expansion behavior is more dangerous than usual because you are
dealing with both types of strings which may contain dots and which may not.
Following one simple rule to use WithoutPathExpansion() variants exclusively is
much safer than trusting you and future developers who touch this file.

[mtomasz, 2014/07/09 09:18:17]

DictionaryValue::SetString() sounds like a default method for setting strings,
while DictionaryValue::SetStringWithoutPathExpansion() sounds like a version for
special cases (when path expansion is *not allowed* rather than *not needed*).

Hence, I was using SetStringWithoutPathExpansion() for that special cases, where
dot may happen.

I'm not saying I disagree with you. I just don't think it's such a big deal. As
I showed in stats, seems that almost everyone use Set() unless a dot may appear
there.

Done.

[hashimoto, 2014/07/11 06:09:05]

Set() has a shorter name and is popular only because SetWithoutPathExpansion()
was not there until recently (http://crrev.com/156716).
There is no reason to avoid being fool proof when writing new code.

[mtomasz, 2014/07/11 06:10:36]

Got it. That makes sense.

+                         file_system_info.file_system_id());
+  file_system->SetString(kPrefKeyFileSystemName,
+                         file_system_info.file_system_name());
 
-  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);
-    }
+  PrefService* pref_service = profile_->GetPrefs();
+  DCHECK(pref_service);
 
-    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);
+  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);
   }
 
-  PrefService* pref_service = profile_->GetPrefs();
-  DCHECK(pref_service);
-  pref_service->Set(prefs::kFileSystemProviderMounted, extensions);
+  file_systems_per_extension->SetWithoutPathExpansion(
+      file_system_info.file_system_id(), file_system);
   pref_service->CommitPendingWrite();
 }
 
-void Service::ForgetFileSystems(const std::string& extension_id) {
+void Service::ForgetFileSystem(const ProvidedFileSystemInfo& file_system_info) {
   PrefService* pref_service = profile_->GetPrefs();
   DCHECK(pref_service);
 
-  DictionaryPrefUpdate update(pref_service, prefs::kFileSystemProviderMounted);
-  base::DictionaryValue* extensions = update.Get();
-  DCHECK(extensions);
-
-  extensions->Remove(extension_id, NULL);
-}
+  DictionaryPrefUpdate dict_update(pref_service,
+                                   prefs::kFileSystemProviderMounted);
 
-void Service::RestoreFileSystems(const std::string& extension_id) {
-  PrefService* pref_service = profile_->GetPrefs();
-  DCHECK(pref_service);
+  base::DictionaryValue* file_systems_per_extension = NULL;
+  if (!dict_update->GetDictionary(file_system_info.extension_id(),
+                                  &file_systems_per_extension)) {
+    return;  // Nothing to forget.
+  }
 
-  const base::DictionaryValue* extensions =
-      pref_service->GetDictionary(prefs::kFileSystemProviderMounted);
-  DCHECK(extensions);
+  file_systems_per_extension->RemoveWithoutPathExpansion(
+      file_system_info.file_system_id(), NULL);
+  if (!file_systems_per_extension->size())
+    dict_update->Remove(file_system_info.extension_id(), NULL);
 
-  const base::ListValue* file_systems = NULL;
+  pref_service->CommitPendingWrite();
+}
 
-  if (!extensions->GetList(extension_id, &file_systems))
-    return;
+void Service::RestoreFileSystems(const std::string& extension_id) {
+  const base::DictionaryValue* file_systems_per_extension = NULL;
+  if (!file_systems_to_restore_->GetDictionary(extension_id,

[hashimoto, 2014/07/02 08:59:37]

Is it OK to be unable to read prefs saved by the older version code?

[mtomasz, 2014/07/03 05:24:07]

Yes. The API is not released yet, and it is available when built from sources
only.

+                                               &file_systems_per_extension)) {
+    return;  // Nothing to restore.
+  }
 
-  for (size_t i = 0; i < file_systems->GetSize(); ++i) {
+  base::DictionaryValue::Iterator it(*file_systems_per_extension);
+  while (!it.IsAtEnd()) {

[hashimoto, 2014/07/02 08:59:37]

nit: How about "for (base::DicationaryValue::Iterator it(...); !it.IsAtEnd();
it.Advance())"?

[mtomasz, 2014/07/03 05:24:07]

Done.

+    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)
@@ -390,7 +412,11 @@ void Service::RestoreFileSystems(const std::string& extension_id) {
                  << "preferences: " << extension_id << ", " << file_system_id
                  << ", " << file_system_name << ".";
     }
+
+    it.Advance();
   }
+
+  file_systems_to_restore_->Remove(extension_id, NULL);
 }
 
 }  // namespace file_system_provider