Commit 9bbd4b92 authored by Sami Kyostila's avatar Sami Kyostila Committed by Chromium LUCI CQ

sequence_manager: Fix potential crash when sweeping away last task in queue

If the last delayed task in a queue is cancelled and swept away, we
ended up comparing an invalid iterator with the end of the queue.
Depending on where the new end of the queue ends up being, this can
cause us to try to cancel an invalid task, potentially triggering a
crash.

Bug: 1155905
Change-Id: I9df4a2802dbaf3da733f8bde72f394b8fe0b7fab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612970
Auto-Submit: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: Carlos Caballero <carlscab@google.com>
Reviewed-by: default avatarCarlos Caballero <carlscab@google.com>
Cr-Commit-Position: refs/heads/master@{#840984}
parent d24a515a
......@@ -2508,6 +2508,8 @@ class CancelableTask {
run_times->push_back(clock_->NowTicks());
}
void FailTask() { FAIL(); }
const TickClock* clock_;
WeakPtrFactory<CancelableTask> weak_factory_{this};
};
......@@ -2547,6 +2549,20 @@ TEST_P(SequenceManagerTest, TaskQueueObserver_SweepCanceledDelayedTasks) {
sequence_manager()->ReclaimMemory();
}
TEST_P(SequenceManagerTest, TaskQueueObserver_SweepLastTaskInQueue) {
auto queue = CreateTaskQueue();
CancelableTask task(mock_tick_clock());
queue->task_runner()->PostDelayedTask(
FROM_HERE,
BindOnce(&CancelableTask::FailTask, task.weak_factory_.GetWeakPtr()),
base::TimeDelta::FromSeconds(1));
// Make sure sweeping away the last task in the queue doesn't end up accessing
// invalid iterators.
task.weak_factory_.InvalidateWeakPtrs();
sequence_manager()->ReclaimMemory();
}
namespace {
void ChromiumRunloopInspectionTask(
......
......@@ -1407,7 +1407,7 @@ void TaskQueueImpl::DelayedIncomingQueue::SweepCancelledTasks(
// canceled tasks in place.
bool task_deleted = false;
auto it = queue_.c.begin();
while (it != queue_.c.end()) {
while (!queue_.c.empty() && it != queue_.c.end()) {
// TODO(crbug.com/1155905): Remove after figuring out the cause of the
// crash.
sequence_manager->RecordCrashKeys(*it);
......@@ -1417,6 +1417,8 @@ void TaskQueueImpl::DelayedIncomingQueue::SweepCancelledTasks(
*it = std::move(queue_.c.back());
queue_.c.pop_back();
task_deleted = true;
// Note: if we just removed the last task in the queue, |it| is now
// invalid and shouldn't be used.
} else {
it++;
}
......
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