Garbage Collect the Storage directory on next profile start after an extension uninstall.

content/browser/storage_partition_impl_map.cc

Index: content/browser/storage_partition_impl_map.cc
diff --git a/content/browser/storage_partition_impl_map.cc b/content/browser/storage_partition_impl_map.cc
index 06f3dc432c7b73310544acca214c5fa1f66d2c58..fab116a51d0ccdfc8205787859c381082ce42742 100644
--- a/content/browser/storage_partition_impl_map.cc
+++ b/content/browser/storage_partition_impl_map.cc
@@ -9,9 +9,10 @@
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/stl_util.h"
-#include "base/string_util.h"
 #include "base/string_number_conversions.h"
-#include "base/threading/worker_pool.h"
+#include "base/string_util.h"
+#include "base/stringprintf.h"
+#include "base/threading/sequenced_worker_pool.h"
 #include "content/browser/appcache/chrome_appcache_service.h"
 #include "content/browser/fileapi/browser_file_system_helper.h"
 #include "content/browser/fileapi/chrome_blob_storage_context.h"
@@ -210,6 +211,8 @@ const FilePath::CharType kExtensionsDirname[] =
     FILE_PATH_LITERAL("ext");
 const FilePath::CharType kDefaultPartitionDirname[] =
     FILE_PATH_LITERAL("def");
+const FilePath::CharType kTrashDirname[] =
+    FILE_PATH_LITERAL("trash");
 
 // Because partition names are user specified, they can be arbitrarily long
 // which makes them unsuitable for paths names. We use a truncation of a
@@ -267,6 +270,8 @@ FilePath GetStoragePartitionDomainPath(
 void ObliterateOneDirectory(const FilePath& current_dir,
                             const std::vector<FilePath>& paths_to_keep,
                             std::vector<FilePath>* paths_to_consider) {
+  CHECK(current_dir.IsAbsolute());
+
   file_util::FileEnumerator enumerator(current_dir, false, kAllFileTypes);
   for (FilePath to_delete = enumerator.Next(); !to_delete.empty();
        to_delete = enumerator.Next()) {
@@ -305,8 +310,13 @@ void ObliterateOneDirectory(const FilePath& current_dir,
 // Synchronously attempts to delete |root|, preserving only entries in
 // |paths_to_keep|. If there are no entries in |paths_to_keep| on disk, then it
 // completely removes |root|. All paths must be absolute paths.
-void BlockingObliteratePath(const FilePath& root,
-                            const std::vector<FilePath>& paths_to_keep) {
+void BlockingObliteratePath(
+    const FilePath& root,
+    const std::vector<FilePath>& paths_to_keep,
+    const scoped_refptr<base::TaskRunner>& closure_runner,
+    const base::Closure& on_gc_required) {
+  CHECK(root.IsAbsolute());
+
   // Reduce |paths_to_keep| set to those under the root and actually on disk.
   std::vector<FilePath> valid_paths_to_keep;
   for (std::vector<FilePath>::const_iterator it = paths_to_keep.begin();
@@ -321,6 +331,8 @@ void BlockingObliteratePath(const FilePath& root,
   if (valid_paths_to_keep.empty()) {
     file_util::Delete(root, true);
     return;
+  } else {

[Charlie Reis, 2012/12/10 23:22:28]

nit: Don't need the else, since we return early above.

[awong, 2012/12/11 01:18:32]

Done.

+    closure_runner->PostTask(FROM_HERE, on_gc_required);
   }
 
   // Otherwise, start at the root and delete everything that is not in
@@ -334,6 +346,49 @@ void BlockingObliteratePath(const FilePath& root,
   }
 }
 
+// Deletes all entries inside the |storage_root| that are not in the
+// |active_paths|.  Deletion is done in 2 steps:
+//
+//   (1) Moving all garbage collected paths into a trash directory.
+//   (2) Asynchronously deleting the trash directory.
+//
+// The deletion is asynchronously because after (1) completes, calling code can

[Charlie Reis, 2012/12/10 23:22:28]

nit: asynchronous
Also, "calling code can safely continue..."

[awong, 2012/12/11 01:18:32]

Done.

+// continue to use the paths that had just been garbage collected without
+// fear of race conditions.
+//
+// This code also ignores failed moves rather than attempting a smarter retry.
+// Moves shouldn't fail here unless there is some out-of-band error (eg.,
+// FS corruption). Retry logic is dangerous in the general case because
+// there is not necessarily a guaranteed case where the logic may succeed.
+void BlockingGarbageCollect(

[Charlie Reis, 2012/12/10 23:22:28]

What does the "Blocking" part of the name refer to?  Just the moving of
directories into the trash directory?  Unclear, given how much emphasis there is
on the asynchronous behavior.

[awong, 2012/12/11 01:18:32]

Yeah, it's referring to the move to trash.

[awong, 2012/12/11 01:18:32]

Yep. It's referring to the moves and the enumeration. I added a comment.

+    const FilePath& storage_root,
+    const scoped_refptr<base::TaskRunner>& file_access_runner,
+    scoped_ptr<base::hash_set<FilePath> > active_paths) {
+  CHECK(storage_root.IsAbsolute());
+
+  file_util::FileEnumerator enumerator(storage_root, false, kAllFileTypes);
+  FilePath trash_directory;
+  if (!file_util::CreateTemporaryDirInDir(storage_root, kTrashDirname,
+                                          &trash_directory)) {
+    // Unable to continue without creating the trash directory so give up.
+    return;
+  }
+  for (FilePath path = enumerator.Next(); !path.empty();
+       path = enumerator.Next()) {
+    if (active_paths->find(path) == active_paths->end() &&
+        path != trash_directory) {
+      // Since |trash_directory| is unique for each run of this function there
+      // can be no colllisions on the move.
+      file_util::Move(path, trash_directory.Append(path.BaseName()));
+    }
+  }
+
+  file_access_runner->PostTask(
+      FROM_HERE,
+      base::Bind(base::IgnoreResult(&file_util::Delete), trash_directory,
+                 true));
+}
+
 }  // namespace
 
 // static
@@ -365,6 +420,10 @@ StoragePartitionImplMap::StoragePartitionImplMap(
     BrowserContext* browser_context)
     : browser_context_(browser_context),
       resource_context_initialized_(false) {
+  // Doing here instead of initializer list cause it's just too ugly to read.
+  base::SequencedWorkerPool* blocking_pool = BrowserThread::GetBlockingPool();
+  file_access_runner_ =
+      blocking_pool->GetSequencedTaskRunner(blocking_pool->GetSequenceToken());
 }
 
 StoragePartitionImplMap::~StoragePartitionImplMap() {
@@ -418,7 +477,9 @@ StoragePartitionImpl* StoragePartitionImplMap::Get(
   return partition;
 }
 
-void StoragePartitionImplMap::AsyncObliterate(const GURL& site) {
+void StoragePartitionImplMap::AsyncObliterate(
+    const GURL& site,
+    const base::Closure& on_gc_required) {
   // This method should avoid creating any StoragePartition (which would
   // create more open file handles) so that it can delete as much of the
   // data off disk as possible.
@@ -429,10 +490,6 @@ void StoragePartitionImplMap::AsyncObliterate(const GURL& site) {
       browser_context_, site, false, &partition_domain,
       &partition_name, &in_memory);
 
-  // The default partition is the whole profile. We can't obliterate that and
-  // should never even try.
-  CHECK(!partition_domain.empty()) << site;
-
   // Find the active partitions for the domain. Because these partitions are
   // active, it is not possible to just delete the directories that contain
   // the backing data structures without causing the browser to crash. Instead,
@@ -459,13 +516,39 @@ void StoragePartitionImplMap::AsyncObliterate(const GURL& site) {
   // run of the browser.
   FilePath domain_root = browser_context_->GetPath().Append(
       GetStoragePartitionDomainPath(partition_domain));
-  base::WorkerPool::PostTask(
+
+  // Never try to obliterate the browser context root. Die hard.
+  CHECK(domain_root != browser_context_->GetPath()) << site;

[Charlie Reis, 2012/12/10 23:22:28]

Can we easily check that domain_root is within browser_context_->GetPath() as
well?  Maybe that's overkill, but I wouldn't want something like "../" to be
able to cause larger parts of the user's filesystem to be deleted.

[awong, 2012/12/11 01:18:32]

Made this check more robust.

+
+  BrowserThread::PostBlockingPoolTask(
       FROM_HERE,
-      base::Bind(&BlockingObliteratePath, domain_root, paths_to_keep),
-      true);
+      base::Bind(&BlockingObliteratePath, domain_root, paths_to_keep,
+                 base::MessageLoopProxy::current(), on_gc_required));
+}
 
-  // TODO(ajwong): Schedule a final AsyncObliterate of the whole directory on
-  // the next browser start. http://crbug.com/85127.
+void StoragePartitionImplMap::GarbageCollect(
+    scoped_ptr<base::hash_set<FilePath> > active_paths,
+    const base::Closure& done) {
+  // Include all paths for current StoragePartitions in the active_paths since
+  // they cannot be deleted safely.
+  for (PartitionMap::const_iterator it = partitions_.begin();
+       it != partitions_.end();
+       ++it) {
+    const StoragePartitionConfig& config = it->first;
+    if (!config.in_memory)
+      active_paths->insert(it->second->GetPath());
+  }
+
+  // Find the directory holding the StoragePartitions and delete everything in
+  // there that isn't considered active.
+  FilePath storage_root = browser_context_->GetPath().Append(
+      GetStoragePartitionDomainPath(std::string()));
+  file_access_runner_->PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&BlockingGarbageCollect, storage_root,
+                 file_access_runner_,
+                 base::Passed(&active_paths)),
+      done);
 }
 
 void StoragePartitionImplMap::ForEach(