Fix possible crash in RecursiveOperationDelegate and throttle # of parallel tasks

- RecursiveOperationDelegate::ProcessFile() could fail /w calling its
  callback in an synchronous way, so it must be always called
  asynchronously or at the end of a synchronous block.
- LocalFilesystemOperation::SetUp() has to be also called before calling into
  RecursiveOperationDelegate.
- This also introduces a constant kMaxInflightOperations to cap the max # of
  inflight operations at the same time.

BUG=146215
TEST=tested with locally patched test, http://crbug.com/172424 will add one
TEST=LocalFileSystemOperationTest.TestRemoveSuccessRecursive for throttle

Review URL: https://codereview.chromium.org/12177006

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

4 files changed, 59 insertions, 20 deletions

diff --git a/webkit/fileapi/local_file_system_operation.cc b/webkit/fileapi/local_file_system_operation.cc
index 4cda1f8..c2349dd 100644
--- a/webkit/fileapi/local_file_system_operation.cc
+++ b/webkit/fileapi/local_file_system_operation.cc
@@ -225,6 +225,14 @@ void LocalFileSystemOperation::Remove(const FileSystemURL& url,
                                       bool recursive,
                                       const StatusCallback& callback) {
   DCHECK(SetPendingOperationType(kOperationRemove));
+
+  base::PlatformFileError result = SetUp(url, SETUP_FOR_WRITE);
+  if (result != base::PLATFORM_FILE_OK) {
+    callback.Run(result);
+    delete this;
+    return;
+  }
+
   DCHECK(!recursive_operation_delegate_);
   recursive_operation_delegate_.reset(
       new RemoveOperationDelegate(
diff --git a/webkit/fileapi/local_file_system_operation_unittest.cc b/webkit/fileapi/local_file_system_operation_unittest.cc
index 37b76a5..8bc9972 100644
--- a/webkit/fileapi/local_file_system_operation_unittest.cc
+++ b/webkit/fileapi/local_file_system_operation_unittest.cc
@@ -1031,16 +1031,24 @@ TEST_F(LocalFileSystemOperationTest, TestRemoveSuccess) {
 
   EXPECT_EQ(1, change_observer()->get_and_reset_remove_directory_count());
   EXPECT_TRUE(change_observer()->HasNoChange());
+}
 
+TEST_F(LocalFileSystemOperationTest, TestRemoveSuccessRecursive) {
   // Removing a non-empty directory with recursive flag == true should be ok.
   //      parent_dir
   //       |       |
-  //  child_dir  child_file
+  //  child_dir  child_files
+  //       |
+  //  child_files
+  //
   // Verify deleting parent_dir.
   base::FilePath parent_dir_path(CreateUniqueDir());
-  base::FilePath child_file_path(CreateUniqueFileInDir(parent_dir_path));
+  for (int i = 0; i < 8; ++i)
+    CreateUniqueFileInDir(parent_dir_path);
   base::FilePath child_dir_path(CreateUniqueDirInDir(parent_dir_path));
   ASSERT_FALSE(child_dir_path.empty());
+  for (int i = 0; i < 8; ++i)
+    CreateUniqueFileInDir(child_dir_path);
 
   operation()->Remove(URLForPath(parent_dir_path), true /* recursive */,
                       RecordStatusCallback());
@@ -1049,7 +1057,7 @@ TEST_F(LocalFileSystemOperationTest, TestRemoveSuccess) {
   EXPECT_FALSE(DirectoryExists(parent_dir_path));
 
   EXPECT_EQ(2, change_observer()->get_and_reset_remove_directory_count());
-  EXPECT_EQ(1, change_observer()->get_and_reset_remove_file_count());
+  EXPECT_EQ(16, change_observer()->get_and_reset_remove_file_count());
   EXPECT_TRUE(change_observer()->HasNoChange());
 }
 
diff --git a/webkit/fileapi/recursive_operation_delegate.cc b/webkit/fileapi/recursive_operation_delegate.cc
index 32830db..4efdefc 100644
--- a/webkit/fileapi/recursive_operation_delegate.cc
+++ b/webkit/fileapi/recursive_operation_delegate.cc
@@ -11,6 +11,11 @@
 
 namespace fileapi {
 
+namespace {
+// Don't start too many inflight operations.
+const int kMaxInflightOperations = 5;
+}
+
 RecursiveOperationDelegate::RecursiveOperationDelegate(
     LocalFileSystemOperation* original_operation)
     : original_operation_(original_operation),
@@ -24,7 +29,7 @@ void RecursiveOperationDelegate::StartRecursiveOperation(
     const StatusCallback& callback) {
   callback_ = callback;
   pending_directories_.push(root);
-  ProcessNextDirectory(base::PLATFORM_FILE_OK);
+  ProcessNextDirectory();
 }
 
 LocalFileSystemOperation* RecursiveOperationDelegate::NewOperation(
@@ -54,16 +59,12 @@ FileSystemContext* RecursiveOperationDelegate::file_system_context() {
   return original_operation_->file_system_context();
 }
 
-void RecursiveOperationDelegate::ProcessNextDirectory(
-    base::PlatformFileError error) {
-  if (error != base::PLATFORM_FILE_OK) {
-    callback_.Run(error);
-    return;
-  }
+void RecursiveOperationDelegate::ProcessNextDirectory() {
+  DCHECK(pending_files_.empty());
   if (inflight_operations_ > 0)
     return;
   if (pending_directories_.empty()) {
-    callback_.Run(error);
+    callback_.Run(base::PLATFORM_FILE_OK);
     return;
   }
   FileSystemURL url = pending_directories_.front();
@@ -74,10 +75,33 @@ void RecursiveOperationDelegate::ProcessNextDirectory(
                       AsWeakPtr(), url));
 }
 
+void RecursiveOperationDelegate::ProcessPendingFiles() {
+  if (pending_files_.empty()) {
+    ProcessNextDirectory();
+    return;
+  }
+  while (!pending_files_.empty() &&
+         inflight_operations_ < kMaxInflightOperations) {
+    FileSystemURL url = pending_files_.front();
+    pending_files_.pop();
+    inflight_operations_++;
+    base::MessageLoopProxy::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&RecursiveOperationDelegate::ProcessFile,
+                   AsWeakPtr(), url,
+                   base::Bind(&RecursiveOperationDelegate::DidProcessFile,
+                              AsWeakPtr())));
+  }
+}
+
 void RecursiveOperationDelegate::DidProcessFile(base::PlatformFileError error) {
   inflight_operations_--;
   DCHECK_GE(inflight_operations_, 0);
-  ProcessNextDirectory(error);
+  if (error != base::PLATFORM_FILE_OK) {
+    callback_.Run(error);
+    return;
+  }
+  ProcessPendingFiles();
 }
 
 void RecursiveOperationDelegate::DidProcessDirectory(
@@ -115,20 +139,17 @@ void RecursiveOperationDelegate::DidReadDirectory(
   }
   for (size_t i = 0; i < entries.size(); i++) {
     FileSystemURL url = parent.WithPath(parent.path().Append(entries[i].name));
-    if (entries[i].is_directory) {
+    if (entries[i].is_directory)
       pending_directories_.push(url);
-      continue;
-    }
-    inflight_operations_++;
-    ProcessFile(url, base::Bind(&RecursiveOperationDelegate::DidProcessFile,
-                                AsWeakPtr()));
+    else
+      pending_files_.push(url);
   }
   if (has_more)
     return;
 
   inflight_operations_--;
   DCHECK_GE(inflight_operations_, 0);
-  ProcessNextDirectory(base::PLATFORM_FILE_OK);
+  ProcessPendingFiles();
 }
 
 void RecursiveOperationDelegate::DidTryProcessFile(
diff --git a/webkit/fileapi/recursive_operation_delegate.h b/webkit/fileapi/recursive_operation_delegate.h
index 1d214e4..8b0626f 100644
--- a/webkit/fileapi/recursive_operation_delegate.h
+++ b/webkit/fileapi/recursive_operation_delegate.h
@@ -74,7 +74,8 @@ class RecursiveOperationDelegate
   FileSystemContext* file_system_context();
 
  private:
-  void ProcessNextDirectory(base::PlatformFileError error);
+  void ProcessNextDirectory();
+  void ProcessPendingFiles();
   void DidProcessFile(base::PlatformFileError error);
   void DidProcessDirectory(const FileSystemURL& url,
                            base::PlatformFileError error);
@@ -89,6 +90,7 @@ class RecursiveOperationDelegate
   LocalFileSystemOperation* original_operation_;
   StatusCallback callback_;
   std::queue<FileSystemURL> pending_directories_;
+  std::queue<FileSystemURL> pending_files_;
   int inflight_operations_;
 
   DISALLOW_COPY_AND_ASSIGN(RecursiveOperationDelegate);