[fsp] Make the reading operation abortable.

Reading may be slow. We should be able to abort it to avoid unnecessary
consumption of resources such as network, cpu and memory.

TEST=browser_test: *FileSystemProvider*ReadFile*
BUG=400574

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

Cr-Commit-Position: refs/heads/master@{#291645}

chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc

@@ -30,59 +30,54 @@ void Int64ToIntCompletionCallback(net::CompletionCallback callback,
   callback.Run(static_cast<int>(result));
 }
 
-// Opens a file for reading and calls the completion callback. Must be called
-// on UI thread.
-void OpenFileOnUIThread(
+}  // namespace
+
+class FileStreamReader::OperationRunner
+    : public base::RefCountedThreadSafe<FileStreamReader::OperationRunner> {
+ public:
+  OperationRunner() : file_handle_(-1) {}
+
+  // Opens a file for reading and calls the completion callback. Must be called
+  // on UI thread.
+  void OpenFileOnUIThread(
       const storage::FileSystemURL& url,
-    const FileStreamReader::OpenFileCompletedCallback& callback) {
+      const storage::AsyncFileUtil::StatusCallback& callback) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
     util::FileSystemURLParser parser(url);
     if (!parser.Parse()) {
-    callback.Run(base::WeakPtr<ProvidedFileSystemInterface>(),
-                 base::FilePath(),
-                 0 /* file_handle */,
-                 base::File::FILE_ERROR_SECURITY);
+      BrowserThread::PostTask(
+          BrowserThread::IO,
+          FROM_HERE,
+          base::Bind(callback, base::File::FILE_ERROR_SECURITY));
       return;
     }
 
-  parser.file_system()->OpenFile(
-      parser.file_path(),
+    file_system_ = parser.file_system()->GetWeakPtr();
+    file_path_ = parser.file_path();
+    abort_callback_ = parser.file_system()->OpenFile(
+        file_path_,
         ProvidedFileSystemInterface::OPEN_FILE_MODE_READ,
         base::Bind(
-          callback, parser.file_system()->GetWeakPtr(), parser.file_path()));
-}
-
-// Forwards results of calling OpenFileOnUIThread back to the IO thread.
-void OnOpenFileCompletedOnUIThread(
-    const FileStreamReader::OpenFileCompletedCallback& callback,
-    base::WeakPtr<ProvidedFileSystemInterface> file_system,
-    const base::FilePath& file_path,
-    int file_handle,
-    base::File::Error result) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  BrowserThread::PostTask(
-      BrowserThread::IO,
-      FROM_HERE,
-      base::Bind(callback, file_system, file_path, file_handle, result));
-}
+            &OperationRunner::OnOpenFileCompletedOnUIThread, this, callback));
+  }
 
-// Closes a file. Ignores result, since it is called from a constructor.
-// Must be called on UI thread.
-void CloseFileOnUIThread(base::WeakPtr<ProvidedFileSystemInterface> file_system,
-                         int file_handle) {
+  // Closes a file. Ignores result, since it is called from a constructor.
+  // Must be called on UI thread.
+  void CloseFileOnUIThread() {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (file_system.get())
-    file_system->CloseFile(file_handle, base::Bind(&EmptyStatusCallback));
-}
+    if (file_system_.get() && file_handle_ != -1) {
+      // Closing a file must not be aborted, since we could end up on files
+      // which are never closed.
+      file_system_->CloseFile(file_handle_, base::Bind(&EmptyStatusCallback));
+    }
+  }
 
-// Requests reading contents of a file. In case of either success or a failure
-// |callback| is executed. It can be called many times, until |has_more| is set
-// to false. This function guarantees that it will succeed only if the file has
-// not been changed while reading. Must be called on UI thread.
-void ReadFileOnUIThread(
-    base::WeakPtr<ProvidedFileSystemInterface> file_system,
-    int file_handle,
+  // Requests reading contents of a file. In case of either success or a failure
+  // |callback| is executed. It can be called many times, until |has_more| is
+  // set to false. This function guarantees that it will succeed only if the
+  // file has not been changed while reading. Must be called on UI thread.
+  void ReadFileOnUIThread(
       scoped_refptr<net::IOBuffer> buffer,
       int64 offset,
       int length,
@@ -90,56 +85,124 @@ void ReadFileOnUIThread(
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
     // If the file system got unmounted, then abort the reading operation.
-  if (!file_system.get()) {
-    callback.Run(0, false /* has_more */, base::File::FILE_ERROR_ABORT);
+    if (!file_system_.get()) {
+      BrowserThread::PostTask(
+          BrowserThread::IO,
+          FROM_HERE,
+          base::Bind(
+              callback, 0, false /* has_more */, base::File::FILE_ERROR_ABORT));
       return;
     }
 
-  file_system->ReadFile(file_handle, buffer, offset, length, callback);
-}
+    abort_callback_ = file_system_->ReadFile(
+        file_handle_,
+        buffer,
+        offset,
+        length,
+        base::Bind(
+            &OperationRunner::OnReadFileCompletedOnUIThread, this, callback));
+  }
 
-// Forward the completion callback to IO thread.
-void OnReadChunkReceivedOnUIThread(
-    const ProvidedFileSystemInterface::ReadChunkReceivedCallback&
-        chunk_received_callback,
-    int chunk_length,
-    bool has_more,
-    base::File::Error result) {
+  // Requests metadata of a file. In case of either succes or a failure,
+  // |callback| is executed. Must be called on UI thread.
+  void GetMetadataOnUIThread(
+      const ProvidedFileSystemInterface::GetMetadataCallback& callback) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+    // If the file system got unmounted, then abort the get length operation.
+    if (!file_system_.get()) {
       BrowserThread::PostTask(
           BrowserThread::IO,
           FROM_HERE,
-      base::Bind(chunk_received_callback, chunk_length, has_more, result));
-}
+          base::Bind(callback, EntryMetadata(), base::File::FILE_ERROR_ABORT));
+      return;
+    }
 
-// Requests metadata of a file. In case of either succes or a failure,
-// |callback is executed. Must be called on UI thread.
-void GetMetadataOnUIThread(
-    base::WeakPtr<ProvidedFileSystemInterface> file_system,
-    const base::FilePath& file_path,
-    const ProvidedFileSystemInterface::GetMetadataCallback& callback) {
+    abort_callback_ = file_system_->GetMetadata(
+        file_path_,
+        base::Bind(&OperationRunner::OnGetMetadataCompletedOnUIThread,
+                   this,
+                   callback));
+  }
+
+  // Aborts the most recent operation (if exists), and calls the callback.
+  void AbortOnUIThread(const storage::AsyncFileUtil::StatusCallback& callback) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  // If the file system got unmounted, then abort the get length operation.
-  if (!file_system.get()) {
-    callback.Run(EntryMetadata(), base::File::FILE_ERROR_ABORT);
+    if (abort_callback_.is_null()) {
+      // No operation to be cancelled. At most a callback call, which will be
+      // discarded.
+      BrowserThread::PostTask(BrowserThread::IO,
+                              FROM_HERE,
+                              base::Bind(callback, base::File::FILE_OK));
       return;
     }
 
-  file_system->GetMetadata(file_path, callback);
-}
+    const ProvidedFileSystemInterface::AbortCallback abort_callback =
+        abort_callback_;
+    abort_callback_ = ProvidedFileSystemInterface::AbortCallback();
+    abort_callback.Run(base::Bind(
+        &OperationRunner::OnAbortCompletedOnUIThread, this, callback));
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<OperationRunner>;
+
+  virtual ~OperationRunner() {}
+
+  // Remembers a file handle for further operations and forwards the result to
+  // the IO thread.
+  void OnOpenFileCompletedOnUIThread(
+      const storage::AsyncFileUtil::StatusCallback& callback,
+      int file_handle,
+      base::File::Error result) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+    file_handle_ = file_handle;
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE, base::Bind(callback, result));
+  }
 
-// Forward the completion callback to IO thread.
-void OnGetMetadataReceivedOnUIThread(
+  // Forwards a metadata to the IO thread.
+  void OnGetMetadataCompletedOnUIThread(
       const ProvidedFileSystemInterface::GetMetadataCallback& callback,
       const EntryMetadata& metadata,
       base::File::Error result) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE, base::Bind(callback, metadata, result));
-}
+  }
 
-}  // namespace
+  // Forwards a response of reading from a file to the IO thread.
+  void OnReadFileCompletedOnUIThread(
+      const ProvidedFileSystemInterface::ReadChunkReceivedCallback&
+          chunk_received_callback,
+      int chunk_length,
+      bool has_more,
+      base::File::Error result) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(chunk_received_callback, chunk_length, has_more, result));
+  }
+
+  // Forwards a response of aborting an operation to the IO thread.
+  void OnAbortCompletedOnUIThread(
+      const storage::AsyncFileUtil::StatusCallback& callback,
+      base::File::Error result) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE, base::Bind(callback, result));
+  }
+
+  ProvidedFileSystemInterface::AbortCallback abort_callback_;
+  base::WeakPtr<ProvidedFileSystemInterface> file_system_;
+  base::FilePath file_path_;
+  int file_handle_;
+
+  DISALLOW_COPY_AND_ASSIGN(OperationRunner);
+};
 
 FileStreamReader::FileStreamReader(storage::FileSystemContext* context,
                                    const storage::FileSystemURL& url,
@@ -149,16 +212,24 @@ FileStreamReader::FileStreamReader(storage::FileSystemContext* context,
       current_offset_(initial_offset),
       current_length_(0),
       expected_modification_time_(expected_modification_time),
+      runner_(new OperationRunner),
       state_(NOT_INITIALIZED),
-      file_handle_(0),
       weak_ptr_factory_(this) {
 }
 
 FileStreamReader::~FileStreamReader() {
+  // FileStreamReader doesn't have a Cancel() method like in FileStreamWriter.
+  // Therefore, aborting is done from the destructor.
+  BrowserThread::PostTask(BrowserThread::UI,
+                          FROM_HERE,
+                          base::Bind(&OperationRunner::AbortOnUIThread,
+                                     runner_,
+                                     base::Bind(&EmptyStatusCallback)));
+
   BrowserThread::PostTask(
       BrowserThread::UI,
       FROM_HERE,
-      base::Bind(&CloseFileOnUIThread, file_system_, file_handle_));
+      base::Bind(&OperationRunner::CloseFileOnUIThread, runner_));
 }
 
 void FileStreamReader::Initialize(
@@ -170,21 +241,18 @@ void FileStreamReader::Initialize(
   BrowserThread::PostTask(
       BrowserThread::UI,
       FROM_HERE,
-      base::Bind(&OpenFileOnUIThread,
+      base::Bind(&OperationRunner::OpenFileOnUIThread,
+                 runner_,
                  url_,
-                 base::Bind(&OnOpenFileCompletedOnUIThread,
                  base::Bind(&FileStreamReader::OnOpenFileCompleted,
                             weak_ptr_factory_.GetWeakPtr(),
                             pending_closure,
-                                       error_callback))));
+                            error_callback)));
 }
 
 void FileStreamReader::OnOpenFileCompleted(
     const base::Closure& pending_closure,
     const net::Int64CompletionCallback& error_callback,
-    base::WeakPtr<ProvidedFileSystemInterface> file_system,
-    const base::FilePath& file_path,
-    int file_handle,
     base::File::Error result) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK_EQ(INITIALIZING, state_);
@@ -197,23 +265,18 @@ void FileStreamReader::OnOpenFileCompleted(
     return;
   }
 
-  file_system_ = file_system;
-  file_path_ = file_path;
-  file_handle_ = file_handle;
-  DCHECK_LT(0, file_handle);
+  DCHECK_EQ(base::File::FILE_OK, result);
 
   // Verify the last modification time.
   BrowserThread::PostTask(
       BrowserThread::UI,
       FROM_HERE,
-      base::Bind(&GetMetadataOnUIThread,
-                 file_system_,
-                 file_path_,
-                 base::Bind(&OnGetMetadataReceivedOnUIThread,
+      base::Bind(&OperationRunner::GetMetadataOnUIThread,
+                 runner_,
                  base::Bind(&FileStreamReader::OnInitializeCompleted,
                             weak_ptr_factory_.GetWeakPtr(),
                             pending_closure,
-                                       error_callback))));
+                            error_callback)));
 }
 
 void FileStreamReader::OnInitializeCompleted(
@@ -341,16 +404,14 @@ void FileStreamReader::ReadAfterInitialized(
   BrowserThread::PostTask(
       BrowserThread::UI,
       FROM_HERE,
-      base::Bind(&ReadFileOnUIThread,
-                 file_system_,
-                 file_handle_,
+      base::Bind(&OperationRunner::ReadFileOnUIThread,
+                 runner_,
                  buffer,
                  current_offset_,
                  buffer_length,
-                 base::Bind(&OnReadChunkReceivedOnUIThread,
                  base::Bind(&FileStreamReader::OnReadChunkReceived,
                             weak_ptr_factory_.GetWeakPtr(),
-                                       callback))));
+                            callback)));
 }
 
 void FileStreamReader::GetLengthAfterInitialized(
@@ -362,14 +423,11 @@ void FileStreamReader::GetLengthAfterInitialized(
       BrowserThread::UI,
       FROM_HERE,
       base::Bind(
-          &GetMetadataOnUIThread,
-          file_system_,
-          file_path_,
-          base::Bind(
-              &OnGetMetadataReceivedOnUIThread,
+          &OperationRunner::GetMetadataOnUIThread,
+          runner_,
           base::Bind(&FileStreamReader::OnGetMetadataForGetLengthReceived,
                      weak_ptr_factory_.GetWeakPtr(),
-                         callback))));
+                     callback)));
 }
 
 void FileStreamReader::OnReadChunkReceived(

chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.h

@@ -43,6 +43,11 @@ class FileStreamReader : public storage::FileStreamReader {
       const net::Int64CompletionCallback& callback) OVERRIDE;
 
  private:
+  // Helper class for executing operations on the provided file system. All
+  // of its methods must be called on UI thread. Callbacks are called on IO
+  // thread.
+  class OperationRunner;
+
   // State of the file stream reader.
   enum State { NOT_INITIALIZED, INITIALIZING, INITIALIZED, FAILED };
 
@@ -59,9 +64,6 @@ class FileStreamReader : public storage::FileStreamReader {
   void OnOpenFileCompleted(
       const base::Closure& pending_closure,
       const net::Int64CompletionCallback& error_callback,
-      base::WeakPtr<ProvidedFileSystemInterface> file_system,
-      const base::FilePath& file_path,
-      int file_handle,
       base::File::Error result);
 
   // Called when initialization is completed with either a success or an error.
@@ -98,13 +100,9 @@ class FileStreamReader : public storage::FileStreamReader {
   int64 current_offset_;
   int64 current_length_;
   base::Time expected_modification_time_;
+  scoped_refptr<OperationRunner> runner_;
   State state_;
 
-  // Set during initialization (in case of a success).
-  base::WeakPtr<ProvidedFileSystemInterface> file_system_;
-  base::FilePath file_path_;
-  int file_handle_;
-
   base::WeakPtrFactory<FileStreamReader> weak_ptr_factory_;
   DISALLOW_COPY_AND_ASSIGN(FileStreamReader);
 };

chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js

@@ -42,6 +42,19 @@ var TESTING_BROKEN_TIRAMISU_FILE = Object.freeze({
   modificationTime: new Date(2014, 1, 25, 7, 36, 12)
 });
 
+/**
+ * Metadata of a broken file used to read contents from, but it simulates
+ * a very long read, in order to verify the aborting mechanism.
+ * @type {Object}
+ * @const
+ */
+var TESTING_VANILLA_FOR_ABORT_FILE = Object.freeze({
+  isDirectory: false,
+  name: 'vanilla.txt',
+  size: TESTING_TEXT.length,
+  modificationTime: new Date(2014, 1, 25, 7, 36, 12)
+});
+
 /**
  * Requests reading contents of a file, previously opened with <code>
  * openRequestId</code>.
@@ -77,6 +90,11 @@ function onReadFileRequested(options, onSuccess, onError) {
     return;
   }
 
+  if (filePath == '/' + TESTING_VANILLA_FOR_ABORT_FILE.name) {
+    // Do nothing. This simulates a very slow read.
+    return;
+  }
+
   if (filePath == '/' + TESTING_BROKEN_TIRAMISU_FILE.name) {
     onError('ACCESS_DENIED');  // enum ProviderError.
     return;
@@ -103,6 +121,8 @@ function setUp(callback) {
       TESTING_TIRAMISU_FILE;
   test_util.defaultMetadata['/' + TESTING_BROKEN_TIRAMISU_FILE.name] =
       TESTING_BROKEN_TIRAMISU_FILE;
+  test_util.defaultMetadata['/' + TESTING_VANILLA_FOR_ABORT_FILE.name] =
+      TESTING_VANILLA_FOR_ABORT_FILE;
 
   chrome.fileSystemProvider.onReadFileRequested.addListener(
       onReadFileRequested);
@@ -143,7 +163,8 @@ function runTests() {
             chrome.test.fail(error.name);
           });
     },
-    // Read contents of a file file, but with an error on the way. This should
+
+    // Read contents of a file,  but with an error on the way. This should
     // result in an error.
     function readEntriesError() {
       var onTestSuccess = chrome.test.callbackPass();
@@ -163,7 +184,56 @@ function runTests() {
               fileReader.readAsText(file);
             },
             function(error) {
+              chrome.test.fail(error.name);
+            });
+          },
+          function(error) {
+            chrome.test.fail(error.name);
+          });
+    },
+
+    // Abort reading a file with a registered abort handler. Should result in a
+    // gracefully terminated reading operation.
+    function abortReadingSuccess() {
+      var onTestSuccess = chrome.test.callbackPass();
+
+      var onAbortRequested = function(options, onSuccess, onError) {
+        chrome.fileSystemProvider.onAbortRequested.removeListener(
+            onAbortRequested);
+        onSuccess();
+        onTestSuccess();
+      };
+
+      chrome.fileSystemProvider.onAbortRequested.addListener(
+          onAbortRequested);
+
+      test_util.fileSystem.root.getFile(
+          TESTING_VANILLA_FOR_ABORT_FILE.name,
+          {create: false, exclusive: false},
+          function(fileEntry) {
+            fileEntry.file(function(file) {
+              var hadAbort = false;
+              var fileReader = new FileReader();
+              fileReader.onload = function(e) {
+                if (!hadAbort) {
+                  chrome.test.fail(
+                      'Unexpectedly finished writing, despite aborting.');
+                  return;
+                }
                 chrome.test.fail();
+              };
+              fileReader.onerror = function(e) {
+                chrome.test.assertEq(
+                    'AbortError', fileReader.error.name);
+              };
+              fileReader.readAsText(file);
+              setTimeout(function() {
+                // Abort the operation after it's started.
+                fileReader.abort();
+              }, 0);
+            },
+            function(error) {
+              chrome.test.fail(error.name);
             });
           },
           function(error) {

chrome/test/data/extensions/api_test/file_system_provider/write_file/test.js

@@ -316,7 +316,7 @@ function runTests() {
     },
 
     // Abort writing to a valid file with a registered abort handler. Should
-    // resurt in a gracefully terminated writing operation.
+    // result in a gracefully terminated writing operation.
     function abortWritingSuccess() {
       var onTestSuccess = chrome.test.callbackPass();