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

storage/browser/fileapi/obfuscated_file_util.cc

 // Copyright (c) 2012 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 "storage/browser/fileapi/obfuscated_file_util.h"
 
 #include <queue>
 #include <string>
 #include <vector>
 
 #include "base/files/file_util.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #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"
 #include "base/time/time.h"
 #include "storage/browser/fileapi/file_observers.h"
 #include "storage/browser/fileapi/file_system_context.h"
 #include "storage/browser/fileapi/file_system_operation_context.h"
 #include "storage/browser/fileapi/file_system_url.h"
 #include "storage/browser/fileapi/native_file_util.h"
 #include "storage/browser/fileapi/sandbox_file_system_backend.h"
@@ skipping 833 matching lines @@
   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())

[kinuko, 2014/09/24 15:51:37]

These '!type_string.empty()' conditions (here and line 882) are added when we
introduce PluginPrivate filesystem, if we don't use this method for it I think
we should probably simply disallow empty type_string here (unless we're passing
empty type_string for other purpose).

Alternatively we could fix this method to correctly work for empty type_string
case.

[nhiroki, 2014/09/25 10:34:16]

I chose the latter way and made this function usable for the empty type_string
case.

       DestroyDirectoryDatabase(origin, type_string);
     if (!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());
 
   if (!type_string.empty()) {
@@ skipping 17 matching lines @@
   if (origin_database_) {
     origin_database_->RemovePathForOrigin(
         storage::GetIdentifierFromOrigin(origin));
   }
   if (!base::DeleteFile(origin_path, true /* recursive */))
     return false;
 
   return true;
 }
 
+bool ObfuscatedFileUtil::DeleteOriginDirectoryForPluginPrivateFileSystem(

[kinuko, 2014/09/24 15:51:37]

So far we hadn't introduced the term 'plugin' in this module
(ObfuscatedFileUtil), do we want to make this class 'PluginPrivate' aware?  To
make this module free from 'plugin' terminology, and also to make it clear that
this method works similar to DeleteDirectoryForOriginAndType(), it might make
better sense to rename this to DeleteDirectoryForOrigin.

[nhiroki, 2014/09/25 10:34:16]

You're right. We don't have to add a specialized function for the plugin private
filesystem. I made DeleteDirectoryForOriginAndType() available for the empty
type_string case and let PluginPrivateFSBackend call it.

(At first I thought a separated function makes things simpler :)

+    const GURL& origin) {
+  // Delete the plugin private filesystems from the directory database.
+  const std::string key_prefix = storage::GetIdentifierFromOrigin(origin);
+  for (DirectoryMap::iterator iter = directories_.lower_bound(key_prefix);
+       iter != directories_.end();) {
+    if (!StartsWithASCII(iter->first, key_prefix, true))
+      break;
+    SandboxDirectoryDatabase* database = iter->second;
+    directories_.erase(iter++);

[tzik, 2014/09/24 18:48:12]

As we chatted locally. (Sorry, I forget to leave that as a written text.)
This deletes http://example.com:1234 for http://example.com/12 deletion.
Please add extra check to ensure iter->first is the database to delete.
And for that, adding a separator between origin and type_string will be easiest
way.

[nhiroki, 2014/09/25 10:34:16]

"http://example.com/12" -> "http://example.com:12" (just in case)
Thank you for pointing it out! I added a whitespace (' ') as a key separator
which is never used for origins and readable when debugging. Wdyt?

+    delete database;
+  }
+
+  base::File::Error error = base::File::FILE_OK;
+
+  // |origin_path| points to "/path/to/File System/Plugins/<origin>".
+  base::FilePath origin_path = GetDirectoryForOrigin(origin, false, &error);
+  if (!origin_path.empty()) {
+    if (!SandboxDirectoryDatabase::DestroyDatabase(origin_path, env_override_))
+      return false;
+    if (!base::DeleteFile(origin_path, true /* recursive */))
+      return false;
+  }

[kinuko, 2014/09/24 15:51:37]

I wonder if supporting type_string.empty() case in DestroyDirectoryDatabase()
(by moving 
moving line 913-921 into DestroyDirectoryDatabase()) and just calling it instead
of line 913-932 might make the code a bit simpler, less dup?  (Either we add
this new method or not)  Wdyt?

[nhiroki, 2014/09/25 10:34:16]

(Removed entirely this function)

+
+  // Delete the origin directory from the origin database.
+  InitOriginDatabase(origin, false);
+  if (origin_database_) {
+    origin_database_->RemovePathForOrigin(
+        storage::GetIdentifierFromOrigin(origin));
+  }
+  return true;
+}
+
 ObfuscatedFileUtil::AbstractOriginEnumerator*
 ObfuscatedFileUtil::CreateOriginEnumerator() {
   std::vector<SandboxOriginDatabase::OriginRecord> origins;
 
   InitOriginDatabase(GURL(), false);
   return new ObfuscatedOriginEnumerator(
       origin_database_.get(), file_system_directory_);
 }
 
 bool ObfuscatedFileUtil::DestroyDirectoryDatabase(
@@ skipping 496 matching lines @@
   }
   return file.Pass();
 }
 
 bool ObfuscatedFileUtil::HasIsolatedStorage(const GURL& origin) {
   return special_storage_policy_.get() &&
       special_storage_policy_->HasIsolatedStorage(origin);
 }
 
 }  // namespace storage