[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());
+  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()) {
     const std::string file_system_id =
         it->second->GetFileSystemInfo().file_system_id();
     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());
   STLDeleteValues(&file_system_map_);
 }
 
 // static
 Service* Service::Get(content::BrowserContext* context) {
   return ServiceFactory::Get(context);
 }
@@ skipping 69 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,
+                                UnmountMode mode) {
   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 (mode == UNMOUNT_MODE_USER)
+    ForgetFileSystem(file_system_info);
+
   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()) {
-      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);
     }
   }
 }
 
 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,

[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());
 
   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();
 }
 
-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);
+  DictionaryPrefUpdate dict_update(pref_service,
+                                   prefs::kFileSystemProviderMounted);
 
-  extensions->Remove(extension_id, NULL);
+  base::DictionaryValue* file_systems_per_extension = NULL;
+  if (!dict_update->GetDictionary(file_system_info.extension_id(),
+                                  &file_systems_per_extension)) {
+    return;  // Nothing to forget.
+  }
+
+  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);
+
+  pref_service->CommitPendingWrite();
 }
 
 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,

[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.
+  }
 
-  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) {
+  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)
           << "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 << ".";
     }
+
+    it.Advance();
   }
+
+  file_systems_to_restore_->Remove(extension_id, NULL);
 }
 
 }  // namespace file_system_provider
 }  // namespace chromeos