Don't keep file system directories around if they contain unrelated files.

webkit/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 "webkit/fileapi/obfuscated_file_util.h"
 
 #include <queue>
 #include <string>
 #include <vector>
 
@@ skipping 890 matching lines @@
     return FilePath();
   FilePath::StringType type_string = GetDirectoryNameForType(type);
   if (type_string.empty()) {
     LOG(WARNING) << "Unknown filesystem type requested:" << type;
 
     if (error_code)
       *error_code = base::PLATFORM_FILE_ERROR_INVALID_URL;
     return FilePath();
   }
   FilePath path = origin_dir.Append(type_string);
+  base::PlatformFileError error = base::PLATFORM_FILE_OK;
   if (!file_util::DirectoryExists(path) &&
       (!create || !file_util::CreateDirectory(path))) {
-    if (error_code) {
-      *error_code = create ?
+    error = create ?
           base::PLATFORM_FILE_ERROR_FAILED :
           base::PLATFORM_FILE_ERROR_NOT_FOUND;
-    }
-    return FilePath();
   }
 
   if (error_code)
-    *error_code = base::PLATFORM_FILE_OK;
+    *error_code = error;

[kinuko, 2012/07/09 11:47:50]

I'm afraid some of the callsites of this method rely on the fact that the
returned path is empty if the error code is not OK.

[Bernhard Bauer, 2012/07/09 15:20:28]

Good point. Would you prefer updating all callsites to look at the error code
instead, or adding a convenience method that returns an empty path if the error
code is not OK?

   return path;
 }
 
 bool ObfuscatedFileUtil::DeleteDirectoryForOriginAndType(
     const GURL& origin, FileSystemType type) {
-  FilePath origin_type_path = GetDirectoryForOriginAndType(origin, type, false);
-  if (!file_util::PathExists(origin_type_path))
+  base::PlatformFileError error = base::PLATFORM_FILE_OK;
+  FilePath origin_type_path = GetDirectoryForOriginAndType(origin, type, false,
+                                                           &error);
+  if (origin_type_path.empty())
     return true;
 
-  // 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.
-  DestroyDirectoryDatabase(origin, type);
-  if (!file_util::Delete(origin_type_path, true /* recursive */))
-    return false;
+  if (error != base::PLATFORM_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.
+    DestroyDirectoryDatabase(origin, type);
+    if (!file_util::Delete(origin_type_path, true /* recursive */))
+      return false;
+  }
 
   FilePath origin_path = origin_type_path.DirName();
   DCHECK_EQ(origin_path.value(),
             GetDirectoryForOrigin(origin, false, NULL).value());
 
   // Delete the origin directory if the deleted one was the last remaining
-  // type for the origin.
-  if (file_util::Delete(origin_path, false /* recursive */)) {
+  // type for the origin, i.e. if the *other* type doesn't exist.
+  FileSystemType other_type = kFileSystemTypeUnknown;
+  switch (type) {
+    case kFileSystemTypeTemporary:
+      other_type = kFileSystemTypePersistent;
+      break;
+    case kFileSystemTypePersistent:
+      other_type = kFileSystemTypeTemporary;
+      break;
+    // These types shouldn't be used.
+    case kFileSystemTypeUnknown:
+    case kFileSystemTypeIsolated:
+    case kFileSystemTypeExternal:
+    case kFileSystemTypeTest:
+      NOTREACHED();
+  }
+  if (!file_util::DirectoryExists(
+          origin_path.Append(GetDirectoryNameForType(other_type)))) {

[kinuko, 2012/07/09 07:23:33]

Could we instead check if the type directory is the only directory in the origin
directory?

[Bernhard Bauer, 2012/07/09 09:03:29]

Hmm, I think there can also be garbage directories (for example, ZIP files of
directories on HFS volumes created by Mac OS contain a __MACOSX directory when
extracted on other file systems).

[kinuko, 2012/07/09 11:47:50]

Ok...

     InitOriginDatabase(false);
     if (origin_database_.get())
       origin_database_->RemovePathForOrigin(GetOriginIdentifierFromURL(origin));
+    if (!file_util::Delete(origin_path, true /* recursive */))
+      return false;
   }
 
   // At this point we are sure we had successfully deleted the origin/type
   // directory, so just returning true here.
   return true;
 }
 
 bool ObfuscatedFileUtil::MigrateFromOldSandbox(
     const GURL& origin_url, FileSystemType type, const FilePath& src_root) {
   if (!DestroyDirectoryDatabase(origin_url, type))
@@ skipping 398 matching lines @@
   PlatformFileError error = NativeFileUtil::CreateDirectory(
       new_local_path, false /* exclusive */, false /* recursive */);
   if (error != base::PLATFORM_FILE_OK)
     return error;
 
   *local_path = new_local_path.AppendASCII(StringPrintf("%08" PRId64, number));
   return base::PLATFORM_FILE_OK;
 }
 
 }  // namespace fileapi