FileSystem: Modify ObfucatedFileUtil to delete contents of the plugin private filesystem

storage/browser/fileapi/obfuscated_file_util.cc

Index: storage/browser/fileapi/obfuscated_file_util.cc
diff --git a/storage/browser/fileapi/obfuscated_file_util.cc b/storage/browser/fileapi/obfuscated_file_util.cc
index 4e202ebb68bf9d12779e11e72cf6f869e4d5f250..b7c73355382eec12826f652660d3f2e125cee8f1 100644
--- a/storage/browser/fileapi/obfuscated_file_util.cc
+++ b/storage/browser/fileapi/obfuscated_file_util.cc
@@ -15,6 +15,7 @@
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
@@ -67,6 +68,8 @@ void InitFileInfo(
 const int64 kPathCreationQuotaCost = 146;  // Bytes per inode, basically.
 const int64 kPathByteQuotaCost = 2;  // Bytes per byte of path length in UTF-8.
 
+const char kDirectoryDatabaseKeySeparator = ' ';
+
 int64 UsageForPath(size_t length) {
   return kPathCreationQuotaCost +
       static_cast<int64>(length) * kPathByteQuotaCost;
@@ -857,28 +860,26 @@ base::FilePath ObfuscatedFileUtil::GetDirectoryForOriginAndType(
 bool ObfuscatedFileUtil::DeleteDirectoryForOriginAndType(
     const GURL& origin,
     const std::string& type_string) {
-  base::File::Error error = base::File::FILE_OK;
-  base::FilePath origin_type_path = GetDirectoryForOriginAndType(
-      origin, type_string, false, &error);
-  if (origin_type_path.empty())
-    return true;
-  if (error != base::File::FILE_ERROR_NOT_FOUND) {
-    // TODO(dmikurube): Consider the return value of DestroyDirectoryDatabase.
-    // We ignore its error now since 1) it doesn't matter the final result, and
-    // 2) it always returns false in Windows because of LevelDB's
-    // implementation.
-    // Information about failure would be useful for debugging.
-    if (!type_string.empty())
-      DestroyDirectoryDatabase(origin, type_string);
-    if (!base::DeleteFile(origin_type_path, true /* recursive */))
+  if (!DestroyDirectoryDatabase(origin, type_string))
+    return false;
+
+  const base::FilePath origin_path = GetDirectoryForOrigin(origin, false, NULL);
+  if (!type_string.empty()) {
+    // Delete the filesystem type directory.
+    base::File::Error error = base::File::FILE_OK;
+    const base::FilePath origin_type_path =
+        GetDirectoryForOriginAndType(origin, type_string, false, &error);
+    if (error == base::File::FILE_ERROR_FAILED)
       return false;
-  }
+    if (error == base::File::FILE_OK &&
+        !origin_type_path.empty() &&
+        !base::DeleteFile(origin_type_path, true /* recursive */)) {
+      return false;
+    }
 
-  base::FilePath origin_path = VirtualPath::DirName(origin_type_path);
-  DCHECK_EQ(origin_path.value(),
-            GetDirectoryForOrigin(origin, false, NULL).value());
+    DCHECK_EQ(origin_path.value(),
+              GetDirectoryForOrigin(origin, false, NULL).value());

[kinuko, 2014/09/25 16:33:00]

nit: This is checking the return val of GetDirectoryForOrigin() to the same
value.

[nhiroki, 2014/11/07 08:15:42]

Done.

 
-  if (!type_string.empty()) {
     // At this point we are sure we had successfully deleted the origin/type
     // directory (i.e. we're ready to just return true).
     // See if we have other directories in this origin directory.
@@ -900,10 +901,7 @@ bool ObfuscatedFileUtil::DeleteDirectoryForOriginAndType(
     origin_database_->RemovePathForOrigin(
         storage::GetIdentifierFromOrigin(origin));
   }
-  if (!base::DeleteFile(origin_path, true /* recursive */))
-    return false;
-
-  return true;
+  return base::DeleteFile(origin_path, true /* recursive */);
 }
 
 ObfuscatedFileUtil::AbstractOriginEnumerator*
@@ -918,15 +916,19 @@ ObfuscatedFileUtil::CreateOriginEnumerator() {
 bool ObfuscatedFileUtil::DestroyDirectoryDatabase(
     const GURL& origin,
     const std::string& type_string) {
-  std::string key = GetDirectoryDatabaseKey(origin, type_string);
-  if (key.empty())
-    return true;
-  DirectoryMap::iterator iter = directories_.find(key);
-  if (iter == directories_.end())
-    return true;
-  scoped_ptr<SandboxDirectoryDatabase> database(iter->second);
-  directories_.erase(iter);
-  return database->DestroyDatabase();
+  // If |type_string| is empty, delete all filesystem types under |origin|.

[kinuko, 2014/09/25 16:33:00]

Probably we could add a test to make sure this doesn't delete
"http://example.com:1234" for "http://example.com:12"?

[nhiroki, 2014/11/07 08:15:42]

Added.

+  const std::string key_prefix = GetDirectoryDatabaseKey(origin, type_string);
+  for (DirectoryMap::iterator iter = directories_.lower_bound(key_prefix);
+       iter != directories_.end();) {
+    if (!StartsWithASCII(iter->first, key_prefix, true))
+      break;
+    DCHECK(type_string.empty() || iter->first == key_prefix);
+    scoped_ptr<SandboxDirectoryDatabase> database(iter->second);
+    directories_.erase(iter++);
+    if (!database->DestroyDatabase())
+      return false;

[kinuko, 2014/09/25 16:33:00]

It's a bit hard to say what we should do when we could destroy some of the DBs
but not all...

[nhiroki, 2014/11/07 08:15:42]

I see... I made DestroyDatabase() ignore the failure as the original code is
because this failure does not affect subsequent operations and the final result.

+  }
+  return true;
 }
 
 // static
@@ -1138,12 +1140,9 @@ base::FilePath ObfuscatedFileUtil::DataPathToLocalPath(
 
 std::string ObfuscatedFileUtil::GetDirectoryDatabaseKey(
     const GURL& origin, const std::string& type_string) {
-  if (type_string.empty()) {
-    LOG(WARNING) << "Unknown filesystem type requested:" << type_string;
-    return std::string();
-  }
   // For isolated origin we just use a type string as a key.
-  return storage::GetIdentifierFromOrigin(origin) + type_string;
+  return storage::GetIdentifierFromOrigin(origin) +
+      kDirectoryDatabaseKeySeparator + type_string;
 }
 
 // TODO(ericu): How to do the whole validation-without-creation thing?