Dispatch FileSystemOperationRunner callbacks always synchronously

- Re-post callbacks only if they are called synchronously
- Always return a valid operation ID (even if it's finished immediately)
- Fix cancel callbacks ordering

BUG=279288
TEST=existing tests
TBR=hidehiko@chromium.org, tzik@chromium.org

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

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

content/content_tests.gypi

@@ -497,6 +497,7 @@
         '../webkit/browser/fileapi/file_system_file_stream_reader_unittest.cc',
         '../webkit/browser/fileapi/file_system_operation_impl_unittest.cc',
         '../webkit/browser/fileapi/file_system_operation_impl_write_unittest.cc',
+        '../webkit/browser/fileapi/file_system_operation_runner_unittest.cc',
         '../webkit/browser/fileapi/file_system_quota_client_unittest.cc',
         '../webkit/browser/fileapi/file_system_url_request_job_unittest.cc',
         '../webkit/browser/fileapi/file_system_url_unittest.cc',

webkit/browser/fileapi/file_system_operation_runner.h

@@ -43,8 +43,6 @@ class WEBKIT_STORAGE_BROWSER_EXPORT FileSystemOperationRunner
 
   typedef int OperationID;
 
-  static const OperationID kErrorOperationID;
-
   virtual ~FileSystemOperationRunner();
 
   // Cancels all inflight operations.
@@ -222,35 +220,45 @@ class WEBKIT_STORAGE_BROWSER_EXPORT FileSystemOperationRunner
                                               base::FilePath* platform_path);
 
  private:
+  class BeginOperationScoper;
+
+  struct OperationHandle {
+    OperationID id;
+    base::WeakPtr<BeginOperationScoper> scope;
+
+    OperationHandle();
+    ~OperationHandle();
+  };
+
   friend class FileSystemContext;
   explicit FileSystemOperationRunner(FileSystemContext* file_system_context);
 
-  void DidFinish(OperationID id,
+  void DidFinish(const OperationHandle& handle,
                  const StatusCallback& callback,
                  base::PlatformFileError rv);
-  void DidGetMetadata(OperationID id,
+  void DidGetMetadata(const OperationHandle& handle,
                       const GetMetadataCallback& callback,
                       base::PlatformFileError rv,
                       const base::PlatformFileInfo& file_info);
-  void DidReadDirectory(OperationID id,
+  void DidReadDirectory(const OperationHandle& handle,
                         const ReadDirectoryCallback& callback,
                         base::PlatformFileError rv,
                         const std::vector<DirectoryEntry>& entries,
                         bool has_more);
-  void DidWrite(OperationID id,
+  void DidWrite(const OperationHandle& handle,
                 const WriteCallback& callback,
                 base::PlatformFileError rv,
                 int64 bytes,
                 bool complete);
   void DidOpenFile(
-      OperationID id,
+      const OperationHandle& handle,
       const OpenFileCallback& callback,
       base::PlatformFileError rv,
       base::PlatformFile file,
       const base::Closure& on_close_callback,
       base::ProcessHandle peer_handle);
   void DidCreateSnapshot(
-      OperationID id,
+      const OperationHandle& handle,
       const SnapshotFileCallback& callback,
       base::PlatformFileError rv,
       const base::PlatformFileInfo& file_info,
@@ -260,7 +268,9 @@ class WEBKIT_STORAGE_BROWSER_EXPORT FileSystemOperationRunner
   void PrepareForWrite(OperationID id, const FileSystemURL& url);
   void PrepareForRead(OperationID id, const FileSystemURL& url);
 
-  // This must be called at the end of any async operations.
+  // These must be called at the beginning and end of any async operations.
+  OperationHandle BeginOperation(FileSystemOperation* operation,
+                                 base::WeakPtr<BeginOperationScoper> scope);
   void FinishOperation(OperationID id);
 
   // Not owned; file_system_context owns this.
@@ -274,6 +284,12 @@ class WEBKIT_STORAGE_BROWSER_EXPORT FileSystemOperationRunner
   typedef std::map<OperationID, FileSystemURLSet> OperationToURLSet;
   OperationToURLSet write_target_urls_;
 
+  // Operations that are finished but not yet fire their callbacks.
+  std::set<OperationID> finished_operations_;
+
+  // Callbacks for stray cancels whose target operation is already finished.
+  std::map<OperationID, StatusCallback> stray_cancel_callbacks_;
+
   DISALLOW_COPY_AND_ASSIGN(FileSystemOperationRunner);
 };
 

webkit/browser/fileapi/file_system_operation_runner_unittest.cc

+// Copyright 2013 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 "base/basictypes.h"
+#include "base/files/file_path.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/message_loop/message_loop.h"
+#include "base/platform_file.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "webkit/browser/fileapi/file_system_context.h"
+#include "webkit/browser/fileapi/file_system_operation_runner.h"
+#include "webkit/browser/fileapi/mock_file_system_context.h"
+
+namespace fileapi {
+
+void GetStatus(bool* done,
+               base::PlatformFileError *status_out,
+               base::PlatformFileError status) {
+  ASSERT_FALSE(*done);
+  *done = true;
+  *status_out = status;
+}
+
+void GetCancelStatus(bool* operation_done,
+                     bool* cancel_done,
+                     base::PlatformFileError *status_out,
+                     base::PlatformFileError status) {
+  // Cancel callback must be always called after the operation's callback.
+  ASSERT_TRUE(*operation_done);
+  ASSERT_FALSE(*cancel_done);
+  *cancel_done = true;
+  *status_out = status;
+}
+
+class FileSystemOperationRunnerTest : public testing::Test {
+ protected:
+  FileSystemOperationRunnerTest() {}
+  virtual ~FileSystemOperationRunnerTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    ASSERT_TRUE(base_.CreateUniqueTempDir());
+    base::FilePath base_dir = base_.path();
+    file_system_context_ =
+        CreateFileSystemContextForTesting(NULL, base_dir);
+  }
+
+  virtual void TearDown() OVERRIDE {
+    file_system_context_ = NULL;
+    base::MessageLoop::current()->RunUntilIdle();
+  }
+
+  FileSystemURL URL(const std::string& path) {
+    return file_system_context_->CreateCrackedFileSystemURL(
+        GURL("http://example.com"), kFileSystemTypeTemporary,
+        base::FilePath::FromUTF8Unsafe(path));
+  }
+
+  FileSystemOperationRunner* operation_runner() {
+    return file_system_context_->operation_runner();
+  }
+
+ private:
+  base::ScopedTempDir base_;
+  base::MessageLoop message_loop_;
+  scoped_refptr<FileSystemContext> file_system_context_;
+
+  DISALLOW_COPY_AND_ASSIGN(FileSystemOperationRunnerTest);
+};
+
+TEST_F(FileSystemOperationRunnerTest, NotFoundError) {
+  bool done = false;
+  base::PlatformFileError status = base::PLATFORM_FILE_ERROR_FAILED;
+
+  // Regular NOT_FOUND error, which is called asynchronously.
+  operation_runner()->Truncate(URL("foo"), 0,
+                               base::Bind(&GetStatus, &done, &status));
+  ASSERT_FALSE(done);
+  base::MessageLoop::current()->RunUntilIdle();
+  ASSERT_TRUE(done);
+  ASSERT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, status);
+}
+
+TEST_F(FileSystemOperationRunnerTest, InvalidURLError) {
+  bool done = false;
+  base::PlatformFileError status = base::PLATFORM_FILE_ERROR_FAILED;
+
+  // Invalid URL error, which calls DidFinish synchronously.
+  operation_runner()->Truncate(FileSystemURL(), 0,
+                               base::Bind(&GetStatus, &done, &status));
+  // The error call back shouldn't be fired synchronously.
+  ASSERT_FALSE(done);
+
+  base::MessageLoop::current()->RunUntilIdle();
+  ASSERT_TRUE(done);
+  ASSERT_EQ(base::PLATFORM_FILE_ERROR_INVALID_URL, status);
+}
+
+TEST_F(FileSystemOperationRunnerTest, NotFoundErrorAndCancel) {
+  bool done = false;
+  bool cancel_done = false;
+  base::PlatformFileError status = base::PLATFORM_FILE_ERROR_FAILED;
+  base::PlatformFileError cancel_status = base::PLATFORM_FILE_ERROR_FAILED;
+
+  // Call Truncate with non-existent URL, and try to cancel it immediately
+  // after that (before its callback is fired).
+  FileSystemOperationRunner::OperationID id =
+      operation_runner()->Truncate(URL("foo"), 0,
+                                   base::Bind(&GetStatus, &done, &status));
+  operation_runner()->Cancel(id, base::Bind(&GetCancelStatus,
+                                            &done, &cancel_done,
+                                            &cancel_status));
+
+  ASSERT_FALSE(done);
+  ASSERT_FALSE(cancel_done);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  ASSERT_TRUE(done);
+  ASSERT_TRUE(cancel_done);
+  ASSERT_EQ(base::PLATFORM_FILE_ERROR_ABORT, status);
+  ASSERT_EQ(base::PLATFORM_FILE_OK, cancel_status);
+}
+
+TEST_F(FileSystemOperationRunnerTest, InvalidURLErrorAndCancel) {
+  bool done = false;
+  bool cancel_done = false;
+  base::PlatformFileError status = base::PLATFORM_FILE_ERROR_FAILED;
+  base::PlatformFileError cancel_status = base::PLATFORM_FILE_ERROR_FAILED;
+
+  // Call Truncate with invalid URL, and try to cancel it immediately
+  // after that (before its callback is fired).
+  FileSystemOperationRunner::OperationID id =
+      operation_runner()->Truncate(FileSystemURL(), 0,
+                                  base::Bind(&GetStatus, &done, &status));
+  operation_runner()->Cancel(id, base::Bind(&GetCancelStatus,
+                                            &done, &cancel_done,
+                                            &cancel_status));
+
+  ASSERT_FALSE(done);
+  ASSERT_FALSE(cancel_done);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  ASSERT_TRUE(done);
+  ASSERT_TRUE(cancel_done);
+  ASSERT_EQ(base::PLATFORM_FILE_ERROR_INVALID_URL, status);
+  ASSERT_EQ(base::PLATFORM_FILE_ERROR_INVALID_OPERATION, cancel_status);
+}
+
+}  // namespace fileapi