Adding RevokeFileSystemByPath

so that we can make sure we invalidate all file systems associated to a given path (e.g. for a detached device)

BUG=none
TEST=IsolatedContextTest.*

Review URL: https://chromiumcodereview.appspot.com/10829136

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@149732 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/media_gallery/media_file_system_registry.cc

@@ -186,6 +186,8 @@ void MediaFileSystemRegistry::RevokeMediaFileSystem(const FilePath& path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   IsolatedContext* isolated_context = IsolatedContext::GetInstance();
+  isolated_context->RevokeFileSystemByPath(path);
+
   for (ChildIdToMediaFSMap::iterator child_it = media_fs_map_.begin();
        child_it != media_fs_map_.end();
        ++child_it) {
@@ -193,7 +195,6 @@ void MediaFileSystemRegistry::RevokeMediaFileSystem(const FilePath& path) {
     MediaPathToFSIDMap::iterator media_path_it = child_map.find(path);
     if (media_path_it == child_map.end())
       continue;
-    isolated_context->RevokeFileSystem(media_path_it->second);
     child_map.erase(media_path_it);
   }
 }

webkit/fileapi/isolated_context.cc

@@ -39,6 +39,31 @@ FilePath::StringType GetRegisterNameForPath(const FilePath& path) {
 #endif
 }
 
+bool IsSinglePathIsolatedFileSystem(FileSystemType type) {
+  switch (type) {
+    // As of writing dragged file system is the only filesystem
+    // which could have multiple toplevel paths.
+    case kFileSystemTypeDragged:
+      return false;
+
+    // Regular file systems.
+    case kFileSystemTypeIsolated:
+    case kFileSystemTypeNativeMedia:
+    case kFileSystemTypeDeviceMedia:
+    case kFileSystemTypeTemporary:
+    case kFileSystemTypePersistent:
+    case kFileSystemTypeExternal:
+    case kFileSystemTypeTest:
+      return true;
+
+    case kFileSystemTypeUnknown:
+      NOTREACHED();
+      return true;
+  }
+  NOTREACHED();
+  return true;
+}
+
 }
 
 static base::LazyInstance<IsolatedContext>::Leaky g_isolated_context =
@@ -91,29 +116,38 @@ IsolatedContext::Instance::Instance(FileSystemType type,
                                     const FileInfo& file_info)
     : type_(type),
       file_info_(file_info),
-      ref_counts_(0) {}
+      ref_counts_(0) {
+  DCHECK(IsSinglePathIsolatedFileSystem(type_));
+}
 
-IsolatedContext::Instance::Instance(const std::set<FileInfo>& dragged_files)
-    : type_(kFileSystemTypeDragged),
-      dragged_files_(dragged_files),
-      ref_counts_(0) {}
+IsolatedContext::Instance::Instance(FileSystemType type,
+                                    const std::set<FileInfo>& files)
+    : type_(type),
+      files_(files),
+      ref_counts_(0) {
+  DCHECK(!IsSinglePathIsolatedFileSystem(type_));
+}
 
 IsolatedContext::Instance::~Instance() {}
 
 bool IsolatedContext::Instance::ResolvePathForName(const std::string& name,
-                                                   FilePath* path) {
-  if (type_ != kFileSystemTypeDragged) {
+                                                   FilePath* path) const {
+  if (IsSinglePathIsolatedFileSystem(type_)) {
     *path = file_info_.path;
     return file_info_.name == name;
   }
-  std::set<FileInfo>::const_iterator found = dragged_files_.find(
+  std::set<FileInfo>::const_iterator found = files_.find(
       FileInfo(name, FilePath()));
-  if (found == dragged_files_.end())
+  if (found == files_.end())
     return false;
   *path = found->path;
   return true;
 }
 
+bool IsolatedContext::Instance::IsSinglePathInstance() const {
+  return IsSinglePathIsolatedFileSystem(type_);
+}
+
 //--------------------------------------------------------------------------
 
 // static
@@ -125,7 +159,8 @@ std::string IsolatedContext::RegisterDraggedFileSystem(
     const FileInfoSet& files) {
   base::AutoLock locker(lock_);
   std::string filesystem_id = GetNewFileSystemId();
-  instance_map_[filesystem_id] = new Instance(files.fileset());
+  instance_map_[filesystem_id] = new Instance(
+      kFileSystemTypeDragged, files.fileset());
   return filesystem_id;
 }
 
@@ -146,16 +181,25 @@ std::string IsolatedContext::RegisterFileSystemForPath(
   base::AutoLock locker(lock_);
   std::string filesystem_id = GetNewFileSystemId();
   instance_map_[filesystem_id] = new Instance(type, FileInfo(name, path));
+  path_to_id_map_[path].insert(filesystem_id);
   return filesystem_id;
 }
 
-void IsolatedContext::RevokeFileSystem(const std::string& filesystem_id) {
+void IsolatedContext::RevokeFileSystemByPath(const FilePath& path) {
   base::AutoLock locker(lock_);
-  IDToInstance::iterator found = instance_map_.find(filesystem_id);
-  if (found == instance_map_.end())
+  PathToID::iterator ids_iter = path_to_id_map_.find(path);
+  if (ids_iter == path_to_id_map_.end())
     return;
-  delete found->second;
-  instance_map_.erase(found);
+  std::set<std::string>& ids = ids_iter->second;
+  for (std::set<std::string>::iterator iter = ids.begin();
+       iter != ids.end(); ++iter) {
+    IDToInstance::iterator found = instance_map_.find(*iter);
+    if (found != instance_map_.end()) {
+      delete found->second;
+      instance_map_.erase(found);
+    }
+  }
+  path_to_id_map_.erase(ids_iter);
 }
 
 void IsolatedContext::AddReference(const std::string& filesystem_id) {
@@ -167,14 +211,23 @@ void IsolatedContext::AddReference(const std::string& filesystem_id) {
 void IsolatedContext::RemoveReference(const std::string& filesystem_id) {
   base::AutoLock locker(lock_);
   // This could get called for non-existent filesystem if it has been
-  // already deleted by RevokeFileSystem.
+  // already deleted by RevokeFileSystemByPath.
   IDToInstance::iterator found = instance_map_.find(filesystem_id);
   if (found == instance_map_.end())
     return;
-  DCHECK(found->second->ref_counts() > 0);
-  found->second->RemoveRef();
-  if (found->second->ref_counts() == 0) {
-    delete found->second;
+  Instance* instance = found->second;
+  DCHECK(instance->ref_counts() > 0);
+  instance->RemoveRef();
+  if (instance->ref_counts() == 0) {
+    if (instance->IsSinglePathInstance()) {
+      PathToID::iterator ids_iter = path_to_id_map_.find(
+          instance->file_info().path);
+      DCHECK(ids_iter != path_to_id_map_.end());
+      ids_iter->second.erase(filesystem_id);
+      if (ids_iter->second.empty())
+        path_to_id_map_.erase(ids_iter);
+    }
+    delete instance;
     instance_map_.erase(found);
   }
 }
@@ -229,8 +282,8 @@ bool IsolatedContext::GetDraggedFileInfo(
   if (found == instance_map_.end() ||
       found->second->type() != kFileSystemTypeDragged)
     return false;
-  files->assign(found->second->dragged_files().begin(),
-                found->second->dragged_files().end());
+  files->assign(found->second->files().begin(),
+                found->second->files().end());
   return true;
 }
 
@@ -239,8 +292,7 @@ bool IsolatedContext::GetRegisteredPath(
   DCHECK(path);
   base::AutoLock locker(lock_);
   IDToInstance::const_iterator found = instance_map_.find(filesystem_id);
-  if (found == instance_map_.end() ||
-      found->second->type() == kFileSystemTypeDragged)
+  if (found == instance_map_.end() || !found->second->IsSinglePathInstance())
     return false;
   *path = found->second->file_info().path;
   return true;

webkit/fileapi/isolated_context.h

@@ -88,7 +88,7 @@ class FILEAPI_EXPORT IsolatedContext {
   // cracked into '/a/b/dir/foo'.
   //
   // Note that the path in |fileset| that contains '..' or is not an
-  // absolute path is skipped and is not registerred.
+  // absolute path is skipped and is not registered.
   std::string RegisterDraggedFileSystem(const FileInfoSet& files);
 
   // Registers a new isolated filesystem for a given |path| of filesystem
@@ -101,11 +101,15 @@ class FILEAPI_EXPORT IsolatedContext {
                                         const FilePath& path,
                                         std::string* register_name);
 
-  // Revokes filesystem specified by the given filesystem_id.
+  // Revokes all filesystem(s) registered for the given path.
+  // This is assumed to be called when the registered path becomes
+  // globally invalid, e.g. when a device for the path is detached.
+  //
   // Note that this revokes the filesystem no matter how many references it has.
-  // It is ok to call this on the filesystem that has been already deleted
-  // (if its reference count had reached 0).
-  void RevokeFileSystem(const std::string& filesystem_id);
+  // It is ok to call this for the path that has no associated filesystems.
+  // Note that this only works for the filesystems registered by
+  // |RegisterFileSystemForPath|.
+  void RevokeFileSystemByPath(const FilePath& path);
 
   // Adds a reference to a filesystem specified by the given filesystem_id.
   void AddReference(const std::string& filesystem_id);
@@ -113,13 +117,13 @@ class FILEAPI_EXPORT IsolatedContext {
   // Removes a reference to a filesystem specified by the given filesystem_id.
   // If the reference count reaches 0 the isolated context gets destroyed.
   // It is ok to call this on the filesystem that has been already deleted
-  // (e.g. by RevokeFileSystem).
+  // (e.g. by RevokeFileSystemByPath).
   void RemoveReference(const std::string& filesystem_id);
 
   // Cracks the given |virtual_path| (which should look like
   // "/<filesystem_id>/<registered_name>/<relative_path>") and populates
   // the |filesystem_id| and |path| if the embedded <filesystem_id>
-  // is registerred to this context.  |root_path| is also populated to have
+  // is registered to this context.  |root_path| is also populated to have
   // the registered root (toplevel) file info for the |virtual_path|.
   //
   // Returns false if the given virtual_path or the cracked filesystem_id
@@ -155,26 +159,39 @@ class FILEAPI_EXPORT IsolatedContext {
   // Represents each isolated file system instance.
   class Instance {
    public:
+    // For a single-path file system, which could be registered by
+    // IsolatedContext::RegisterFileSystemForPath().
+    // Most of isolated file system contexts should be of this type.
     Instance(FileSystemType type, const FileInfo& file_info);
-    explicit Instance(const std::set<FileInfo>& dragged_files);
+
+    // For a multi-paths file system.  As of writing only file system
+    // type which could have multi-paths is Dragged file system, and
+    // could be registered by IsolatedContext::RegisterDraggedFileSystem().
+    Instance(FileSystemType type, const std::set<FileInfo>& files);
+
     ~Instance();
 
     FileSystemType type() const { return type_; }
     const FileInfo& file_info() const { return file_info_; }
-    const std::set<FileInfo>& dragged_files() const { return dragged_files_; }
+    const std::set<FileInfo>& files() const { return files_; }
     int ref_counts() const { return ref_counts_; }
 
     void AddRef() { ++ref_counts_; }
     void RemoveRef() { --ref_counts_; }
 
-    bool ResolvePathForName(const std::string& name, FilePath* path);
+    bool ResolvePathForName(const std::string& name, FilePath* path) const;
+
+    // Returns true if the instance is a single-path instance.
+    bool IsSinglePathInstance() const;
 
    private:
     const FileSystemType type_;
+
+    // For single-path instance.
     const FileInfo file_info_;
 
-    // For dragged file system.
-    const std::set<FileInfo> dragged_files_;
+    // For multiple-path instance (e.g. dragged file system).
+    const std::set<FileInfo> files_;
 
     // Reference counts. Note that an isolated filesystem is created with ref==0
     // and will get deleted when the ref count reaches <=0.
@@ -185,6 +202,9 @@ class FILEAPI_EXPORT IsolatedContext {
 
   typedef std::map<std::string, Instance*> IDToInstance;
 
+  // Reverse map from registered path to IDs.
+  typedef std::map<FilePath, std::set<std::string> > PathToID;
+
   // Obtain an instance of this class via GetInstance().
   IsolatedContext();
   ~IsolatedContext();
@@ -196,6 +216,7 @@ class FILEAPI_EXPORT IsolatedContext {
   mutable base::Lock lock_;
 
   IDToInstance instance_map_;
+  PathToID path_to_id_map_;
 
   DISALLOW_COPY_AND_ASSIGN(IsolatedContext);
 };

webkit/fileapi/isolated_context_unittest.cc

@@ -58,11 +58,12 @@ class IsolatedContextTest : public testing::Test {
       names_.push_back(name);
     }
     id_ = IsolatedContext::GetInstance()->RegisterDraggedFileSystem(files);
+    IsolatedContext::GetInstance()->AddReference(id_);
     ASSERT_FALSE(id_.empty());
   }
 
   void TearDown() {
-    IsolatedContext::GetInstance()->RevokeFileSystem(id_);
+    IsolatedContext::GetInstance()->RemoveReference(id_);
   }
 
   IsolatedContext* isolated_context() const {
@@ -109,8 +110,9 @@ TEST_F(IsolatedContextTest, RegisterAndRevokeTest) {
   FilePath path;
   ASSERT_FALSE(isolated_context()->GetRegisteredPath(id_, &path));
 
-  // Revoking the current one and registering a new one.
-  isolated_context()->RevokeFileSystem(id_);
+  // Deref the current one and registering a new one.
+  isolated_context()->RemoveReference(id_);
+
   std::string id2 = isolated_context()->RegisterFileSystemForPath(
       kFileSystemTypeIsolated, FilePath(DRIVE FPL("/foo")), NULL);
 
@@ -119,10 +121,31 @@ TEST_F(IsolatedContextTest, RegisterAndRevokeTest) {
   ASSERT_FALSE(isolated_context()->GetDraggedFileInfo(id_, &toplevels));
 
   // Make sure the GetRegisteredPath returns true only for the new one.
-  ASSERT_TRUE(isolated_context()->GetRegisteredPath(id2, &path));
   ASSERT_FALSE(isolated_context()->GetRegisteredPath(id_, &path));
+  ASSERT_TRUE(isolated_context()->GetRegisteredPath(id2, &path));
+
+  // Try registering two more file systems for the same path as id2.
+  std::string id3 = isolated_context()->RegisterFileSystemForPath(
+      kFileSystemTypeIsolated, path, NULL);
+  std::string id4 = isolated_context()->RegisterFileSystemForPath(
+      kFileSystemTypeIsolated, path, NULL);
+
+  // Remove file system for id4.
+  isolated_context()->AddReference(id4);
+  isolated_context()->RemoveReference(id4);
+
+  // Only id4 should become invalid now.
+  ASSERT_TRUE(isolated_context()->GetRegisteredPath(id2, &path));
+  ASSERT_TRUE(isolated_context()->GetRegisteredPath(id3, &path));
+  ASSERT_FALSE(isolated_context()->GetRegisteredPath(id4, &path));
+
+  // Revoke the file systems by path.
+  isolated_context()->RevokeFileSystemByPath(path);
 
-  isolated_context()->RevokeFileSystem(id2);
+  // Now all the file systems associated to the path must be invalid.
+  ASSERT_FALSE(isolated_context()->GetRegisteredPath(id2, &path));
+  ASSERT_FALSE(isolated_context()->GetRegisteredPath(id3, &path));
+  ASSERT_FALSE(isolated_context()->GetRegisteredPath(id4, &path));
 }
 
 TEST_F(IsolatedContextTest, CrackWithRelativePaths) {

webkit/fileapi/isolated_file_util_unittest.cc

@@ -75,10 +75,12 @@ class IsolatedFileUtilTest : public testing::Test {
     // For cross-FileUtil copy/move tests.
     other_file_util_.reset(new LocalFileUtil());
     other_file_util_helper_.SetUp(file_system_context_, other_file_util_.get());
+
+    isolated_context()->AddReference(filesystem_id_);
   }
 
   void TearDown() {
-    isolated_context()->RevokeFileSystem(filesystem_id_);
+    isolated_context()->RemoveReference(filesystem_id_);
     other_file_util_helper_.TearDown();
   }