[MessageLoop] Clear all pending tasks in TriageQueue::Clear
This is a precursor to moving the |incoming_queue_| multi-threaded part
of IncomingTaskQueue to MessageLoopTaskRunner to allow mocking the
source of incoming tasks (i.e. for ScopedTaskEnvironment::MOCK_TIME on
top of MLForUI/etc.).
Such an interface will not have a notion of pre-triaged versus
still-on-multi-threaded-queue tasks. What matters here is that
TriageQueue::Clear() doesn't get stuck in a loop, the new logic clears
all pending tasks while still backing out if destructors post more
tasks.
R=danakj@chromium.org, kylechar@chromium.org
Bug: 708584
Change-Id: I8cd4f244718e4f5f74d5cb84138083ee37833238
Reviewed-on: https://chromium-review.googlesource.com/1078932
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563846}diff --git a/base/message_loop/incoming_task_queue.cc b/base/message_loop/incoming_task_queue.cc
index c495f14..d6f4d64 100644
--- a/base/message_loop/incoming_task_queue.cc
+++ b/base/message_loop/incoming_task_queue.cc
@@ -7,6 +7,8 @@
#include <limits>
#include <utility>
+#include "base/bind.h"
+#include "base/callback_helpers.h"
#include "base/location.h"
#include "base/message_loop/message_loop.h"
#include "base/synchronization/waitable_event.h"
@@ -157,18 +159,31 @@
void IncomingTaskQueue::TriageQueue::Clear() {
DCHECK_CALLED_ON_VALID_SEQUENCE(outer_->sequence_checker_);
- // Previously, MessageLoop would delete all tasks including delayed and
- // deferred tasks in a single round before attempting to reload from the
- // incoming queue to see if more tasks remained. This gave it a chance to
- // assess whether or not clearing should continue. As a result, while
- // reloading is automatic for getting and seeing if tasks exist, it is not
- // automatic for Clear().
- while (!queue_.empty()) {
- PendingTask pending_task = std::move(queue_.front());
- queue_.pop();
- if (pending_task.is_high_res)
- --outer_->pending_high_res_tasks_;
+ // Clear() should be invoked before WillDestroyCurrentMessageLoop().
+ DCHECK(outer_->accept_new_tasks_);
+
+ // Delete all currently pending tasks but not tasks potentially posted from
+ // their destructors. See ~MessageLoop() for the full logic mitigating against
+ // infite loops when clearing pending tasks. The ScopedClosureRunner below
+ // will be bound to a task posted at the end of the queue. After it is posted,
+ // tasks will be deleted one by one, when the bound ScopedClosureRunner is
+ // deleted and sets |deleted_all_originally_pending|, we know we've deleted
+ // all originally pending tasks.
+ bool deleted_all_originally_pending = false;
+ ScopedClosureRunner capture_deleted_all_originally_pending(BindOnce(
+ [](bool* deleted_all_originally_pending) {
+ *deleted_all_originally_pending = true;
+ },
+ Unretained(&deleted_all_originally_pending)));
+ outer_->AddToIncomingQueue(
+ FROM_HERE,
+ BindOnce([](ScopedClosureRunner) {},
+ std::move(capture_deleted_all_originally_pending)),
+ TimeDelta(), Nestable::kNestable);
+
+ while (!deleted_all_originally_pending) {
+ PendingTask pending_task = Pop();
if (!pending_task.delayed_run_time.is_null()) {
outer_->delayed_tasks().Push(std::move(pending_task));
diff --git a/base/message_loop/incoming_task_queue.h b/base/message_loop/incoming_task_queue.h
index f158d2a..9810cfa7 100644
--- a/base/message_loop/incoming_task_queue.h
+++ b/base/message_loop/incoming_task_queue.h
@@ -129,9 +129,9 @@
~TriageQueue() override;
// ReadAndRemoveOnlyQueue:
- // In general, the methods below will attempt to reload from the incoming
- // queue if the queue itself is empty except for Clear(). See Clear() for
- // why it doesn't reload.
+ // The methods below will attempt to reload from the incoming queue if the
+ // queue itself is empty (Clear() has special logic to reload only once
+ // should destructors post more tasks).
const PendingTask& Peek() override;
PendingTask Pop() override;
// Whether this queue has tasks after reloading from the incoming queue.
diff --git a/base/message_loop/message_loop_unittest.cc b/base/message_loop/message_loop_unittest.cc
index 8525366..3ddce801 100644
--- a/base/message_loop/message_loop_unittest.cc
+++ b/base/message_loop/message_loop_unittest.cc
@@ -23,6 +23,7 @@
#include "base/single_thread_task_runner.h"
#include "base/synchronization/waitable_event.h"
#include "base/task_scheduler/task_scheduler.h"
+#include "base/test/gtest_util.h"
#include "base/test/test_simple_task_runner.h"
#include "base/test/test_timeouts.h"
#include "base/threading/platform_thread.h"
@@ -2174,6 +2175,51 @@
EXPECT_NE(slot.Get(), 11);
}
+namespace {
+
+class PostTaskOnDestroy {
+ public:
+ PostTaskOnDestroy(int times) : times_remaining_(times) {}
+ ~PostTaskOnDestroy() { PostTaskWithPostingDestructor(times_remaining_); }
+
+ // Post a task that will repost itself on destruction |times| times.
+ static void PostTaskWithPostingDestructor(int times) {
+ if (times > 0) {
+ ThreadTaskRunnerHandle::Get()->PostTask(
+ FROM_HERE, BindOnce([](std::unique_ptr<PostTaskOnDestroy>) {},
+ std::make_unique<PostTaskOnDestroy>(times - 1)));
+ }
+ }
+
+ private:
+ const int times_remaining_;
+
+ DISALLOW_COPY_AND_ASSIGN(PostTaskOnDestroy);
+};
+
+} // namespace
+
+// Test that MessageLoop destruction handles a task's destructor posting another
+// task by:
+// 1) Not getting stuck clearing its task queue.
+// 2) DCHECKing when clearing pending tasks many times still doesn't yield an
+// empty queue.
+TEST_P(MessageLoopTest, ExpectDeathWithStubbornPostTaskOnDestroy) {
+ std::unique_ptr<MessageLoop> loop = std::make_unique<MessageLoop>();
+
+ EXPECT_DCHECK_DEATH({
+ PostTaskOnDestroy::PostTaskWithPostingDestructor(1000);
+ loop.reset();
+ });
+}
+
+TEST_P(MessageLoopTest, DestroysFineWithReasonablePostTaskOnDestroy) {
+ std::unique_ptr<MessageLoop> loop = std::make_unique<MessageLoop>();
+
+ PostTaskOnDestroy::PostTaskWithPostingDestructor(10);
+ loop.reset();
+}
+
INSTANTIATE_TEST_CASE_P(
,
MessageLoopTest,
