Fix aborting in throttled file system (FSP).

The previous logic was causing a memory corruption when aborting was not
invoked with the callback, but due to unmounting the file system.

As there are multiple paths for aborting, this CL adds a method IsAborted on
the Queue class so it's possible to check if a task has been aborted before.

TEST=unit_tests: *FileSystemProviderQueue*
BUG=454310

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

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

4 files changed, 50 insertions, 1 deletions

diff --git a/chrome/browser/chromeos/file_system_provider/queue.cc b/chrome/browser/chromeos/file_system_provider/queue.cc
index d0826c7..bd32c11 100644
--- a/chrome/browser/chromeos/file_system_provider/queue.cc
+++ b/chrome/browser/chromeos/file_system_provider/queue.cc
@@ -125,5 +125,21 @@ void Queue::Abort(size_t token) {
   NOTREACHED();
 }
 
+bool Queue::IsAborted(size_t token) {
+#if !NDEBUG
+  bool in_queue = executed_.find(token) != executed_.end() ||
+                  completed_.find(token) != completed_.end() ||
+                  aborted_.find(token) != aborted_.end();
+  for (auto& task : pending_) {
+    if (token == task.token) {
+      in_queue = true;
+      break;
+    }
+  }
+  DCHECK(in_queue);
+#endif
+  return aborted_.find(token) != aborted_.end();
+}
+
 }  // namespace file_system_provider
 }  // namespace chromeos
diff --git a/chrome/browser/chromeos/file_system_provider/queue.h b/chrome/browser/chromeos/file_system_provider/queue.h
index 7eca4f9..559dcd6 100644
--- a/chrome/browser/chromeos/file_system_provider/queue.h
+++ b/chrome/browser/chromeos/file_system_provider/queue.h
@@ -63,6 +63,11 @@ class Queue {
   // abort callback is NULL).
   void Abort(size_t token);
 
+  // Returns true if the task which is in the queue with |token| has been
+  // aborted. This method must not be called for tasks which are not in the
+  // queue.
+  bool IsAborted(size_t token);
+
   // Marks the previously enqueued task as complete. Must be called for each
   // enqueued task (unless aborted). Note, that Remove() must be called in order
   // to remove the task from the queue if it hasn't been aborted earlier.
diff --git a/chrome/browser/chromeos/file_system_provider/queue_unittest.cc b/chrome/browser/chromeos/file_system_provider/queue_unittest.cc
index 62edba9..a5bab45 100644
--- a/chrome/browser/chromeos/file_system_provider/queue_unittest.cc
+++ b/chrome/browser/chromeos/file_system_provider/queue_unittest.cc
@@ -282,6 +282,26 @@ TEST_F(FileSystemProviderQueueTest, InvalidUsage_AbortTwice) {
   EXPECT_DEATH(queue.Abort(first_token), "");
 }
 
+TEST_F(FileSystemProviderQueueTest, InvalidUsage_IsAbortedWhileNotInQueue) {
+  Queue queue(1);
+  EXPECT_DEATH(queue.IsAborted(1234), "");
+}
+
+TEST_F(FileSystemProviderQueueTest, InvalidUsage_IsAbortedAfterRemoved) {
+  Queue queue(1);
+  const size_t first_token = queue.NewToken();
+  int first_counter = 0;
+  int first_abort_counter = 0;
+  queue.Enqueue(first_token,
+                base::Bind(&OnRun, &first_counter, &first_abort_counter));
+
+  base::RunLoop().RunUntilIdle();
+
+  queue.Abort(first_token);
+  queue.Remove(first_token);
+  EXPECT_DEATH(queue.IsAborted(first_token), "");
+}
+
 TEST_F(FileSystemProviderQueueTest, InvalidUsage_RemoveTwice) {
   Queue queue(1);
   const size_t first_token = queue.NewToken();
@@ -363,12 +383,16 @@ TEST_F(FileSystemProviderQueueTest, Enqueue_Abort) {
   EXPECT_EQ(0, second_abort_counter);
 
   // Abort the first task while it's being executed.
+  EXPECT_FALSE(queue.IsAborted(first_token));
   queue.Abort(first_token);
+  EXPECT_TRUE(queue.IsAborted(first_token));
   queue.Remove(first_token);
 
   // Abort the second task, before it's started.
   EXPECT_EQ(0, second_counter);
+  EXPECT_FALSE(queue.IsAborted(second_token));
   queue.Abort(second_token);
+  EXPECT_TRUE(queue.IsAborted(second_token));
   queue.Remove(second_token);
 
   base::RunLoop().RunUntilIdle();
diff --git a/chrome/browser/chromeos/file_system_provider/throttled_file_system.cc b/chrome/browser/chromeos/file_system_provider/throttled_file_system.cc
index df4e424..dc01372 100644
--- a/chrome/browser/chromeos/file_system_provider/throttled_file_system.cc
+++ b/chrome/browser/chromeos/file_system_provider/throttled_file_system.cc
@@ -195,7 +195,11 @@ void ThrottledFileSystem::OnOpenFileCompleted(int queue_token,
                                               const OpenFileCallback& callback,
                                               int file_handle,
                                               base::File::Error result) {
-  if (result != base::File::FILE_ERROR_ABORT)
+  // The task may be aborted either via the callback, or by the operation, eg.
+  // because of destroying the request manager or unmounting the file system
+  // during the operation. Mark the task as completed only if it hasn't been
+  // aborted before.
+  if (!open_queue_->IsAborted(queue_token))
     open_queue_->Complete(queue_token);
 
   // If the file is opened successfully then hold the queue token until the file