Commit d3126d1f authored by mtomasz's avatar mtomasz Committed by Commit bot

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}
parent 5060425d
......@@ -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
......@@ -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.
......
......@@ -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();
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment