DCHECK tasks posted by TaskRunner::PostTask

DCHECK tasks posted by TaskRunner::PostTask

This CL adds DCHECKs to detect null task posted to TaskRunner::PostTask
implementations, so that we can detect it earlier with meaningful stack
trace.

BUG=706854
Review-Url: https://codereview.chromium.org/2785943004
Cr-Commit-Position: refs/heads/master@{#461766}
Committed: https://chromium.googlesource.com/chromium/src/+/c69769616ffc1d266a8c4af8be6b957e2f305da8

[tzik, 2017-03-31 09:17:28]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 09:17:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-03-31 09:22:50]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 09:23:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-03-31 09:32:44]

Description was changed from

==========
CHECK tasks posted by TaskRunner::PostTask

BUG=
==========

to

==========
CHECK tasks posted by TaskRunner::PostTask

This CL adds CHECKs to detect null task posted to TaskRunner::PostTask
implementations, so that we can detect it earlier with meaningful stack
trace.

A null task posted to a TaskRunner eventually causes a crash, but its
stack trace is less useful to debug on it. Though the minidump contains
FROM_HERE where it posted, it's not easy to see it if the crash happened
on other platform than a developer works on.

BUG=
==========

[tzik, 2017-03-31 09:33:31]

Description was changed from

==========
CHECK tasks posted by TaskRunner::PostTask

This CL adds CHECKs to detect null task posted to TaskRunner::PostTask
implementations, so that we can detect it earlier with meaningful stack
trace.

A null task posted to a TaskRunner eventually causes a crash, but its
stack trace is less useful to debug on it. Though the minidump contains
FROM_HERE where it posted, it's not easy to see it if the crash happened
on other platform than a developer works on.

BUG=
==========

to

==========
CHECK tasks posted by TaskRunner::PostTask

This CL adds CHECKs to detect null task posted to TaskRunner::PostTask
implementations, so that we can detect it earlier with meaningful stack
trace.

A null task posted to a TaskRunner eventually causes a crash, but its
stack trace is less useful to debug on it. Though the minidump contains
FROM_HERE where it posted, it's not easy to see it if the crash happened
on other platform than a developer works on.

BUG=706854
==========

[tzik, 2017-03-31 09:33:32]

tzik@chromium.org changed reviewers:
+ gab@chromium.org

[tzik, 2017-03-31 09:36:27]

tzik@chromium.org changed reviewers:
+ alexclarke@chromium.org, kinuko@chromium.org, sky@chromium.org

[commit-bot: I haz the power, 2017-03-31 09:50:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 09:50:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tzik, 2017-03-31 09:52:44]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 09:52:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-03-31 10:24:28]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 10:24:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-03-31 11:23:59]

PTAL to:
gab: //base
sky: //chrome
kinuko: //content
alexclarke: task_queue_impl.cc

[commit-bot: I haz the power, 2017-03-31 12:09:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 12:09:11]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-03-31 14:06:56]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2017-03-31 14:06:57]

Would a DCHECK() be sufficient? That should help catch most of these at
development time, right? However given that the try bots are all green maybe
this mostly happens in the wild?

[Sami, 2017-03-31 14:08:55]

https://codereview.chromium.org/2785943004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
(right):

https://codereview.chromium.org/2785943004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:221:
base::AutoLock lock(any_thread_lock_);
I think we just need to instrument PostImmediateTaskImpl and PostDelayedTaskImpl
-- everything else is downstream from them.

[sky, 2017-03-31 14:30:56]

I prefer DCHECK too.

On Fri, Mar 31, 2017 at 4:23 AM, <tzik@chromium.org> wrote:

> Reviewers: gab, sky, kinuko, alex clarke
> CL: https://codereview.chromium.org/2785943004/
>
> Message:
> PTAL to:
> gab: //base
> sky: //chrome
> kinuko: //content
> alexclarke: task_queue_impl.cc
>
> Description:
> CHECK tasks posted by TaskRunner::PostTask
>
> This CL adds CHECKs to detect null task posted to TaskRunner::PostTask
> implementations, so that we can detect it earlier with meaningful stack
> trace.
>
> A null task posted to a TaskRunner eventually causes a crash, but its
> stack trace is less useful to debug on it. Though the minidump contains
> FROM_HERE where it posted, it's not easy to see it if the crash happened
> on other platform than a developer works on.
>
> BUG=706854
>
> Affected files (+34, -10 lines):
> M base/deferred_sequenced_task_runner.cc
> M base/message_loop/incoming_task_queue.cc
> M base/task_scheduler/delayed_task_manager.cc
> M base/task_scheduler/priority_queue_unittest.cc
> M base/task_scheduler/scheduler_worker_pool_impl.cc
> M base/task_scheduler/sequence.cc
> M base/task_scheduler/sequence_unittest.cc
> M base/threading/sequenced_worker_pool.cc
> M base/threading/worker_pool_posix.cc
> M base/threading/worker_pool_win.cc
> M chrome/browser/after_startup_task_utils.cc
> M content/renderer/categorized_worker_pool.cc
> M third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
>
>
> Index: base/deferred_sequenced_task_runner.cc
> diff --git a/base/deferred_sequenced_task_runner.cc
> b/base/deferred_sequenced_task_runner.cc
> index 3811bdaa82103d648b15910fbcd0a6db93a83517..
> c066018847895437d79471b371ca057e55b26356 100644
> --- a/base/deferred_sequenced_task_runner.cc
> +++ b/base/deferred_sequenced_task_runner.cc
> @@ -72,6 +72,8 @@ void DeferredSequencedTaskRunner::QueueDeferredTask(
> Closure task,
> TimeDelta delay,
> bool is_non_nestable) {
> + CHECK(task);
> +
> DeferredTask deferred_task;
> deferred_task.posted_from = from_here;
> deferred_task.task = std::move(task);
> Index: base/message_loop/incoming_task_queue.cc
> diff --git a/base/message_loop/incoming_task_queue.cc
> b/base/message_loop/incoming_task_queue.cc
> index f0df650adc49f1a506a0fc03ee58cb987f51d64c..
> 51540f0b40483658377ec10eaa49650999d86be8 100644
> --- a/base/message_loop/incoming_task_queue.cc
> +++ b/base/message_loop/incoming_task_queue.cc
> @@ -63,6 +63,7 @@ bool IncomingTaskQueue::AddToIncomingQueue(
> Closure task,
> TimeDelta delay,
> bool nestable) {
> + CHECK(task);
> DLOG_IF(WARNING,
> delay.InSeconds() > kTaskDelayWarningThresholdInSeconds)
> << "Requesting super-long task delay period of " << delay.InSeconds()
> Index: base/task_scheduler/delayed_task_manager.cc
> diff --git a/base/task_scheduler/delayed_task_manager.cc
> b/base/task_scheduler/delayed_task_manager.cc
> index 1921364bd1e6154f644c06d0ed1a3b26af57e6ad..
> dfdab391548dcdaef9ab90cd32da0f667b9e8052 100644
> --- a/base/task_scheduler/delayed_task_manager.cc
> +++ b/base/task_scheduler/delayed_task_manager.cc
> @@ -26,6 +26,7 @@ void DelayedTaskManager::AddDelayedTask(
> std::unique_ptr<Task> task,
> const PostTaskNowCallback& post_task_now_callback) {
> DCHECK(task);
> + CHECK(task->task);
>
> const TimeDelta delay = task->delay;
> DCHECK(!delay.is_zero());
> Index: base/task_scheduler/priority_queue_unittest.cc
> diff --git a/base/task_scheduler/priority_queue_unittest.cc
> b/base/task_scheduler/priority_queue_unittest.cc
> index 221532b60306057cf6a2b64a2d547bfb8c142e55..
> 2ec9ac51ff01a3fb4fa9717d7a2bed2b57cca066 100644
> --- a/base/task_scheduler/priority_queue_unittest.cc
> +++ b/base/task_scheduler/priority_queue_unittest.cc
> @@ -6,6 +6,7 @@
>
> #include <memory>
>
> +#include "base/bind.h"
> #include "base/macros.h"
> #include "base/memory/ptr_util.h"
> #include "base/memory/ref_counted.h"
> @@ -59,26 +60,26 @@ TEST(TaskSchedulerPriorityQueueTest, PushPopPeek) {
> // Create test sequences.
> scoped_refptr<Sequence> sequence_a(new Sequence);
> sequence_a->PushTask(MakeUnique<Task>(
> - FROM_HERE, Closure(),
> + FROM_HERE, Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::USER_VISIBLE), TimeDelta()));
> SequenceSortKey sort_key_a = sequence_a->GetSortKey();
>
> scoped_refptr<Sequence> sequence_b(new Sequence);
> sequence_b->PushTask(MakeUnique<Task>(
> - FROM_HERE, Closure(),
> + FROM_HERE, Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::USER_BLOCKING), TimeDelta()));
> SequenceSortKey sort_key_b = sequence_b->GetSortKey();
>
> scoped_refptr<Sequence> sequence_c(new Sequence);
> sequence_c->PushTask(MakeUnique<Task>(
> - FROM_HERE, Closure(),
> + FROM_HERE, Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::USER_BLOCKING), TimeDelta()));
> SequenceSortKey sort_key_c = sequence_c->GetSortKey();
>
> scoped_refptr<Sequence> sequence_d(new Sequence);
> sequence_d->PushTask(MakeUnique<Task>(
> - FROM_HERE, Closure(), TaskTraits().WithPriority(
> TaskPriority::BACKGROUND),
> - TimeDelta()));
> + FROM_HERE, Bind([] {}),
> + TaskTraits().WithPriority(TaskPriority::BACKGROUND), TimeDelta()));
> SequenceSortKey sort_key_d = sequence_d->GetSortKey();
>
> // Create a PriorityQueue and a Transaction.
> Index: base/task_scheduler/scheduler_worker_pool_impl.cc
> diff --git a/base/task_scheduler/scheduler_worker_pool_impl.cc
> b/base/task_scheduler/scheduler_worker_pool_impl.cc
> index 9e61bd7323cf8bcf507f9277de9017e6acc9498c..
> 9ee9ba0445c5712793b6a90042508f4c2692c37f 100644
> --- a/base/task_scheduler/scheduler_worker_pool_impl.cc
> +++ b/base/task_scheduler/scheduler_worker_pool_impl.cc
> @@ -252,6 +252,7 @@ bool SchedulerWorkerPoolImpl::PostTaskWithSequence(
> if (task->delayed_run_time.is_null()) {
> PostTaskWithSequenceNow(std::move(task), std::move(sequence));
> } else {
> + CHECK(task->task);
> delayed_task_manager_->AddDelayedTask(
> std::move(task),
> Bind(
> Index: base/task_scheduler/sequence.cc
> diff --git a/base/task_scheduler/sequence.cc b/base/task_scheduler/
> sequence.cc
> index 601b5402d069b489b70f960a04931e9875c8d520..
> 381d5cf108db8ae1f0f60071c655d4139c646ba5 100644
> --- a/base/task_scheduler/sequence.cc
> +++ b/base/task_scheduler/sequence.cc
> @@ -15,6 +15,8 @@ namespace internal {
> Sequence::Sequence() = default;
>
> bool Sequence::PushTask(std::unique_ptr<Task> task) {
> + DCHECK(task);
> + CHECK(task->task);
> DCHECK(task->sequenced_time.is_null());
> task->sequenced_time = base::TimeTicks::Now();
>
> Index: base/task_scheduler/sequence_unittest.cc
> diff --git a/base/task_scheduler/sequence_unittest.cc
> b/base/task_scheduler/sequence_unittest.cc
> index c45d8a87d010054872e5861824f5e9911e92f15d..
> 3f792e3a7346d375623ec5917fbbb47688e8561e 100644
> --- a/base/task_scheduler/sequence_unittest.cc
> +++ b/base/task_scheduler/sequence_unittest.cc
> @@ -24,27 +24,27 @@ class TaskSchedulerSequenceTest : public testing::Test
> {
> TaskSchedulerSequenceTest()
> : task_a_owned_(
> new Task(FROM_HERE,
> - Closure(),
> + Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::BACKGROUND),
> TimeDelta())),
> task_b_owned_(
> new Task(FROM_HERE,
> - Closure(),
> + Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::USER_VISIBLE),
> TimeDelta())),
> task_c_owned_(
> new Task(FROM_HERE,
> - Closure(),
> + Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::USER_BLOCKING),
> TimeDelta())),
> task_d_owned_(
> new Task(FROM_HERE,
> - Closure(),
> + Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::USER_BLOCKING),
> TimeDelta())),
> task_e_owned_(
> new Task(FROM_HERE,
> - Closure(),
> + Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::BACKGROUND),
> TimeDelta())),
> task_a_(task_a_owned_.get()),
> Index: base/threading/sequenced_worker_pool.cc
> diff --git a/base/threading/sequenced_worker_pool.cc
> b/base/threading/sequenced_worker_pool.cc
> index a5f997fd34dcdc4d1cbc35021d6f8025fc22bee6..
> 090e1041699000cb29f48ec5da1cff365323ef45 100644
> --- a/base/threading/sequenced_worker_pool.cc
> +++ b/base/threading/sequenced_worker_pool.cc
> @@ -695,6 +695,8 @@ bool SequencedWorkerPool::Inner::PostTask(
> const tracked_objects::Location& from_here,
> Closure task,
> TimeDelta delay) {
> + CHECK(task);
> +
> // TODO(fdoray): Uncomment this DCHECK. It is initially commented to avoid
> a
> // revert of the CL that adds debug::DumpWithoutCrashing() if it fails on
> the
> // waterfall. https://crbug.com/622400
> Index: base/threading/worker_pool_posix.cc
> diff --git a/base/threading/worker_pool_posix.cc
> b/base/threading/worker_pool_posix.cc
> index e0efa463f75dcbbaa0ccf20dfc6be5c0ab73f6fe..
> 0ca7062c0571bc60e4d13ffc9d15797c4785d611 100644
> --- a/base/threading/worker_pool_posix.cc
> +++ b/base/threading/worker_pool_posix.cc
> @@ -146,6 +146,8 @@ void PosixDynamicThreadPool::PostTask(
> }
>
> void PosixDynamicThreadPool::AddTask(PendingTask* pending_task) {
> + DCHECK(pending_task);
> + CHECK(pending_task->task);
> AutoLock locked(lock_);
>
> pending_tasks_.push(std::move(*pending_task));
> Index: base/threading/worker_pool_win.cc
> diff --git a/base/threading/worker_pool_win.cc
> b/base/threading/worker_pool_win.cc
> index ab64f7160f4deb61e9d013b28e6cfb8c3f336203..
> 4c0e42ab83eabc86c77c2c6d9af16385c4230896 100644
> --- a/base/threading/worker_pool_win.cc
> +++ b/base/threading/worker_pool_win.cc
> @@ -45,6 +45,8 @@ DWORD CALLBACK WorkItemCallback(void* param) {
>
> // Takes ownership of |pending_task|
> bool PostTaskInternal(PendingTask* pending_task, bool task_is_slow) {
> + CHECK(pending_task->task);
> +
> ULONG flags = 0;
> if (task_is_slow)
> flags |= WT_EXECUTELONGFUNCTION;
> Index: chrome/browser/after_startup_task_utils.cc
> diff --git a/chrome/browser/after_startup_task_utils.cc
> b/chrome/browser/after_startup_task_utils.cc
> index e024a945968ccb901ceabca657df8747f36288c3..
> bf39464e8eb3c74aa393e215aef3d59a7740300c 100644
> --- a/chrome/browser/after_startup_task_utils.cc
> +++ b/chrome/browser/after_startup_task_utils.cc
> @@ -75,6 +75,9 @@ void ScheduleTask(std::unique_ptr<AfterStartupTask>
> queued_task) {
> }
>
> void QueueTask(std::unique_ptr<AfterStartupTask> queued_task) {
> + DCHECK(queued_task);
> + CHECK(queued_task->task);
> +
> if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
> BrowserThread::PostTask(
> BrowserThread::UI, FROM_HERE,
> Index: content/renderer/categorized_worker_pool.cc
> diff --git a/content/renderer/categorized_worker_pool.cc
> b/content/renderer/categorized_worker_pool.cc
> index fc89b888fa1ef71f10bb62e0f910b75c6446a342..
> 22e7756385bd60d7d12160e82776e42401118bd4 100644
> --- a/content/renderer/categorized_worker_pool.cc
> +++ b/content/renderer/categorized_worker_pool.cc
> @@ -63,6 +63,7 @@ class CategorizedWorkerPool::
> CategorizedWorkerPoolSequencedTaskRunner
> bool PostNonNestableDelayedTask(const tracked_objects::Location&
> from_here,
> base::Closure task,
> base::TimeDelta delay) override {
> + CHECK(task);
> base::AutoLock lock(lock_);
>
> // Remove completed tasks.
> Index: third_party/WebKit/Source/platform/scheduler/base/task_
> queue_impl.cc
> diff --git
a/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
> b/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
> index b2546fc574d45fd0ff580e57d6bc6fc6c4015f9b..
> 7ee64d740f77713b9bb1b9484387d0698358c4c0 100644
> --- a/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
> +++ b/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
> @@ -274,6 +274,8 @@ bool TaskQueueImpl::PostDelayedTaskImpl(
>
> void TaskQueueImpl::PushOntoDelayedIncomingQueueFromMainThread(
> Task pending_task, base::TimeTicks now) {
> + CHECK(pending_task.task);
> +
> base::TimeTicks delayed_run_time = pending_task.delayed_run_time;
> main_thread_only().task_queue_manager->DidQueueTask(pending_task);
> main_thread_only().delayed_incoming_queue.push(std::move(pending_task));
> @@ -293,6 +295,8 @@ void TaskQueueImpl::PushOntoDelayedIncomingQueueFr
> omMainThread(
> }
>
> void TaskQueueImpl::PushOntoDelayedIncomingQueueLocked(Task pending_task)
> {
> + CHECK(pending_task.task);
> +
> any_thread().task_queue_manager->DidQueueTask(pending_task);
>
> int thread_hop_task_sequence_number =
> @@ -333,6 +337,8 @@ void TaskQueueImpl::PushOntoImmediateIncomingQueue
> Locked(
> base::TimeTicks desired_run_time,
> EnqueueOrder sequence_number,
> bool nestable) {
> + CHECK(task);
> +
> // If the |immediate_incoming_queue| is empty we need a DoWork posted to
> make
> // it run.
> if (any_thread().immediate_incoming_queue.empty()) {
>
>
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sky, 2017-03-31 14:30:56]

I prefer DCHECK too.

On Fri, Mar 31, 2017 at 4:23 AM, <tzik@chromium.org> wrote:

> Reviewers: gab, sky, kinuko, alex clarke
> CL: https://codereview.chromium.org/2785943004/
>
> Message:
> PTAL to:
> gab: //base
> sky: //chrome
> kinuko: //content
> alexclarke: task_queue_impl.cc
>
> Description:
> CHECK tasks posted by TaskRunner::PostTask
>
> This CL adds CHECKs to detect null task posted to TaskRunner::PostTask
> implementations, so that we can detect it earlier with meaningful stack
> trace.
>
> A null task posted to a TaskRunner eventually causes a crash, but its
> stack trace is less useful to debug on it. Though the minidump contains
> FROM_HERE where it posted, it's not easy to see it if the crash happened
> on other platform than a developer works on.
>
> BUG=706854
>
> Affected files (+34, -10 lines):
> M base/deferred_sequenced_task_runner.cc
> M base/message_loop/incoming_task_queue.cc
> M base/task_scheduler/delayed_task_manager.cc
> M base/task_scheduler/priority_queue_unittest.cc
> M base/task_scheduler/scheduler_worker_pool_impl.cc
> M base/task_scheduler/sequence.cc
> M base/task_scheduler/sequence_unittest.cc
> M base/threading/sequenced_worker_pool.cc
> M base/threading/worker_pool_posix.cc
> M base/threading/worker_pool_win.cc
> M chrome/browser/after_startup_task_utils.cc
> M content/renderer/categorized_worker_pool.cc
> M third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
>
>
> Index: base/deferred_sequenced_task_runner.cc
> diff --git a/base/deferred_sequenced_task_runner.cc
> b/base/deferred_sequenced_task_runner.cc
> index 3811bdaa82103d648b15910fbcd0a6db93a83517..
> c066018847895437d79471b371ca057e55b26356 100644
> --- a/base/deferred_sequenced_task_runner.cc
> +++ b/base/deferred_sequenced_task_runner.cc
> @@ -72,6 +72,8 @@ void DeferredSequencedTaskRunner::QueueDeferredTask(
> Closure task,
> TimeDelta delay,
> bool is_non_nestable) {
> + CHECK(task);
> +
> DeferredTask deferred_task;
> deferred_task.posted_from = from_here;
> deferred_task.task = std::move(task);
> Index: base/message_loop/incoming_task_queue.cc
> diff --git a/base/message_loop/incoming_task_queue.cc
> b/base/message_loop/incoming_task_queue.cc
> index f0df650adc49f1a506a0fc03ee58cb987f51d64c..
> 51540f0b40483658377ec10eaa49650999d86be8 100644
> --- a/base/message_loop/incoming_task_queue.cc
> +++ b/base/message_loop/incoming_task_queue.cc
> @@ -63,6 +63,7 @@ bool IncomingTaskQueue::AddToIncomingQueue(
> Closure task,
> TimeDelta delay,
> bool nestable) {
> + CHECK(task);
> DLOG_IF(WARNING,
> delay.InSeconds() > kTaskDelayWarningThresholdInSeconds)
> << "Requesting super-long task delay period of " << delay.InSeconds()
> Index: base/task_scheduler/delayed_task_manager.cc
> diff --git a/base/task_scheduler/delayed_task_manager.cc
> b/base/task_scheduler/delayed_task_manager.cc
> index 1921364bd1e6154f644c06d0ed1a3b26af57e6ad..
> dfdab391548dcdaef9ab90cd32da0f667b9e8052 100644
> --- a/base/task_scheduler/delayed_task_manager.cc
> +++ b/base/task_scheduler/delayed_task_manager.cc
> @@ -26,6 +26,7 @@ void DelayedTaskManager::AddDelayedTask(
> std::unique_ptr<Task> task,
> const PostTaskNowCallback& post_task_now_callback) {
> DCHECK(task);
> + CHECK(task->task);
>
> const TimeDelta delay = task->delay;
> DCHECK(!delay.is_zero());
> Index: base/task_scheduler/priority_queue_unittest.cc
> diff --git a/base/task_scheduler/priority_queue_unittest.cc
> b/base/task_scheduler/priority_queue_unittest.cc
> index 221532b60306057cf6a2b64a2d547bfb8c142e55..
> 2ec9ac51ff01a3fb4fa9717d7a2bed2b57cca066 100644
> --- a/base/task_scheduler/priority_queue_unittest.cc
> +++ b/base/task_scheduler/priority_queue_unittest.cc
> @@ -6,6 +6,7 @@
>
> #include <memory>
>
> +#include "base/bind.h"
> #include "base/macros.h"
> #include "base/memory/ptr_util.h"
> #include "base/memory/ref_counted.h"
> @@ -59,26 +60,26 @@ TEST(TaskSchedulerPriorityQueueTest, PushPopPeek) {
> // Create test sequences.
> scoped_refptr<Sequence> sequence_a(new Sequence);
> sequence_a->PushTask(MakeUnique<Task>(
> - FROM_HERE, Closure(),
> + FROM_HERE, Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::USER_VISIBLE), TimeDelta()));
> SequenceSortKey sort_key_a = sequence_a->GetSortKey();
>
> scoped_refptr<Sequence> sequence_b(new Sequence);
> sequence_b->PushTask(MakeUnique<Task>(
> - FROM_HERE, Closure(),
> + FROM_HERE, Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::USER_BLOCKING), TimeDelta()));
> SequenceSortKey sort_key_b = sequence_b->GetSortKey();
>
> scoped_refptr<Sequence> sequence_c(new Sequence);
> sequence_c->PushTask(MakeUnique<Task>(
> - FROM_HERE, Closure(),
> + FROM_HERE, Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::USER_BLOCKING), TimeDelta()));
> SequenceSortKey sort_key_c = sequence_c->GetSortKey();
>
> scoped_refptr<Sequence> sequence_d(new Sequence);
> sequence_d->PushTask(MakeUnique<Task>(
> - FROM_HERE, Closure(), TaskTraits().WithPriority(
> TaskPriority::BACKGROUND),
> - TimeDelta()));
> + FROM_HERE, Bind([] {}),
> + TaskTraits().WithPriority(TaskPriority::BACKGROUND), TimeDelta()));
> SequenceSortKey sort_key_d = sequence_d->GetSortKey();
>
> // Create a PriorityQueue and a Transaction.
> Index: base/task_scheduler/scheduler_worker_pool_impl.cc
> diff --git a/base/task_scheduler/scheduler_worker_pool_impl.cc
> b/base/task_scheduler/scheduler_worker_pool_impl.cc
> index 9e61bd7323cf8bcf507f9277de9017e6acc9498c..
> 9ee9ba0445c5712793b6a90042508f4c2692c37f 100644
> --- a/base/task_scheduler/scheduler_worker_pool_impl.cc
> +++ b/base/task_scheduler/scheduler_worker_pool_impl.cc
> @@ -252,6 +252,7 @@ bool SchedulerWorkerPoolImpl::PostTaskWithSequence(
> if (task->delayed_run_time.is_null()) {
> PostTaskWithSequenceNow(std::move(task), std::move(sequence));
> } else {
> + CHECK(task->task);
> delayed_task_manager_->AddDelayedTask(
> std::move(task),
> Bind(
> Index: base/task_scheduler/sequence.cc
> diff --git a/base/task_scheduler/sequence.cc b/base/task_scheduler/
> sequence.cc
> index 601b5402d069b489b70f960a04931e9875c8d520..
> 381d5cf108db8ae1f0f60071c655d4139c646ba5 100644
> --- a/base/task_scheduler/sequence.cc
> +++ b/base/task_scheduler/sequence.cc
> @@ -15,6 +15,8 @@ namespace internal {
> Sequence::Sequence() = default;
>
> bool Sequence::PushTask(std::unique_ptr<Task> task) {
> + DCHECK(task);
> + CHECK(task->task);
> DCHECK(task->sequenced_time.is_null());
> task->sequenced_time = base::TimeTicks::Now();
>
> Index: base/task_scheduler/sequence_unittest.cc
> diff --git a/base/task_scheduler/sequence_unittest.cc
> b/base/task_scheduler/sequence_unittest.cc
> index c45d8a87d010054872e5861824f5e9911e92f15d..
> 3f792e3a7346d375623ec5917fbbb47688e8561e 100644
> --- a/base/task_scheduler/sequence_unittest.cc
> +++ b/base/task_scheduler/sequence_unittest.cc
> @@ -24,27 +24,27 @@ class TaskSchedulerSequenceTest : public testing::Test
> {
> TaskSchedulerSequenceTest()
> : task_a_owned_(
> new Task(FROM_HERE,
> - Closure(),
> + Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::BACKGROUND),
> TimeDelta())),
> task_b_owned_(
> new Task(FROM_HERE,
> - Closure(),
> + Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::USER_VISIBLE),
> TimeDelta())),
> task_c_owned_(
> new Task(FROM_HERE,
> - Closure(),
> + Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::USER_BLOCKING),
> TimeDelta())),
> task_d_owned_(
> new Task(FROM_HERE,
> - Closure(),
> + Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::USER_BLOCKING),
> TimeDelta())),
> task_e_owned_(
> new Task(FROM_HERE,
> - Closure(),
> + Bind([] {}),
> TaskTraits().WithPriority(TaskPriority::BACKGROUND),
> TimeDelta())),
> task_a_(task_a_owned_.get()),
> Index: base/threading/sequenced_worker_pool.cc
> diff --git a/base/threading/sequenced_worker_pool.cc
> b/base/threading/sequenced_worker_pool.cc
> index a5f997fd34dcdc4d1cbc35021d6f8025fc22bee6..
> 090e1041699000cb29f48ec5da1cff365323ef45 100644
> --- a/base/threading/sequenced_worker_pool.cc
> +++ b/base/threading/sequenced_worker_pool.cc
> @@ -695,6 +695,8 @@ bool SequencedWorkerPool::Inner::PostTask(
> const tracked_objects::Location& from_here,
> Closure task,
> TimeDelta delay) {
> + CHECK(task);
> +
> // TODO(fdoray): Uncomment this DCHECK. It is initially commented to avoid
> a
> // revert of the CL that adds debug::DumpWithoutCrashing() if it fails on
> the
> // waterfall. https://crbug.com/622400
> Index: base/threading/worker_pool_posix.cc
> diff --git a/base/threading/worker_pool_posix.cc
> b/base/threading/worker_pool_posix.cc
> index e0efa463f75dcbbaa0ccf20dfc6be5c0ab73f6fe..
> 0ca7062c0571bc60e4d13ffc9d15797c4785d611 100644
> --- a/base/threading/worker_pool_posix.cc
> +++ b/base/threading/worker_pool_posix.cc
> @@ -146,6 +146,8 @@ void PosixDynamicThreadPool::PostTask(
> }
>
> void PosixDynamicThreadPool::AddTask(PendingTask* pending_task) {
> + DCHECK(pending_task);
> + CHECK(pending_task->task);
> AutoLock locked(lock_);
>
> pending_tasks_.push(std::move(*pending_task));
> Index: base/threading/worker_pool_win.cc
> diff --git a/base/threading/worker_pool_win.cc
> b/base/threading/worker_pool_win.cc
> index ab64f7160f4deb61e9d013b28e6cfb8c3f336203..
> 4c0e42ab83eabc86c77c2c6d9af16385c4230896 100644
> --- a/base/threading/worker_pool_win.cc
> +++ b/base/threading/worker_pool_win.cc
> @@ -45,6 +45,8 @@ DWORD CALLBACK WorkItemCallback(void* param) {
>
> // Takes ownership of |pending_task|
> bool PostTaskInternal(PendingTask* pending_task, bool task_is_slow) {
> + CHECK(pending_task->task);
> +
> ULONG flags = 0;
> if (task_is_slow)
> flags |= WT_EXECUTELONGFUNCTION;
> Index: chrome/browser/after_startup_task_utils.cc
> diff --git a/chrome/browser/after_startup_task_utils.cc
> b/chrome/browser/after_startup_task_utils.cc
> index e024a945968ccb901ceabca657df8747f36288c3..
> bf39464e8eb3c74aa393e215aef3d59a7740300c 100644
> --- a/chrome/browser/after_startup_task_utils.cc
> +++ b/chrome/browser/after_startup_task_utils.cc
> @@ -75,6 +75,9 @@ void ScheduleTask(std::unique_ptr<AfterStartupTask>
> queued_task) {
> }
>
> void QueueTask(std::unique_ptr<AfterStartupTask> queued_task) {
> + DCHECK(queued_task);
> + CHECK(queued_task->task);
> +
> if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
> BrowserThread::PostTask(
> BrowserThread::UI, FROM_HERE,
> Index: content/renderer/categorized_worker_pool.cc
> diff --git a/content/renderer/categorized_worker_pool.cc
> b/content/renderer/categorized_worker_pool.cc
> index fc89b888fa1ef71f10bb62e0f910b75c6446a342..
> 22e7756385bd60d7d12160e82776e42401118bd4 100644
> --- a/content/renderer/categorized_worker_pool.cc
> +++ b/content/renderer/categorized_worker_pool.cc
> @@ -63,6 +63,7 @@ class CategorizedWorkerPool::
> CategorizedWorkerPoolSequencedTaskRunner
> bool PostNonNestableDelayedTask(const tracked_objects::Location&
> from_here,
> base::Closure task,
> base::TimeDelta delay) override {
> + CHECK(task);
> base::AutoLock lock(lock_);
>
> // Remove completed tasks.
> Index: third_party/WebKit/Source/platform/scheduler/base/task_
> queue_impl.cc
> diff --git
a/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
> b/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
> index b2546fc574d45fd0ff580e57d6bc6fc6c4015f9b..
> 7ee64d740f77713b9bb1b9484387d0698358c4c0 100644
> --- a/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
> +++ b/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
> @@ -274,6 +274,8 @@ bool TaskQueueImpl::PostDelayedTaskImpl(
>
> void TaskQueueImpl::PushOntoDelayedIncomingQueueFromMainThread(
> Task pending_task, base::TimeTicks now) {
> + CHECK(pending_task.task);
> +
> base::TimeTicks delayed_run_time = pending_task.delayed_run_time;
> main_thread_only().task_queue_manager->DidQueueTask(pending_task);
> main_thread_only().delayed_incoming_queue.push(std::move(pending_task));
> @@ -293,6 +295,8 @@ void TaskQueueImpl::PushOntoDelayedIncomingQueueFr
> omMainThread(
> }
>
> void TaskQueueImpl::PushOntoDelayedIncomingQueueLocked(Task pending_task)
> {
> + CHECK(pending_task.task);
> +
> any_thread().task_queue_manager->DidQueueTask(pending_task);
>
> int thread_hop_task_sequence_number =
> @@ -333,6 +337,8 @@ void TaskQueueImpl::PushOntoImmediateIncomingQueue
> Locked(
> base::TimeTicks desired_run_time,
> EnqueueOrder sequence_number,
> bool nestable) {
> + CHECK(task);
> +
> // If the |immediate_incoming_queue| is empty we need a DoWork posted to
> make
> // it run.
> if (any_thread().immediate_incoming_queue.empty()) {
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tzik, 2017-03-31 17:02:33]

On 2017/03/31 14:06:57, Sami wrote:
> Would a DCHECK() be sufficient? That should help catch most of these at
> development time, right? However given that the try bots are all green maybe
> this mostly happens in the wild?

That's currently happen in the wild.
IIRC, it's third time to me, which is not so frequent, but it's the first
mac-only one.
The reported minidumps should have FROM_HERE info in principle as a local
variable, but it's annoying to debug on a minidump on a platform where I
regularly work on.

[kinuko, 2017-04-03 07:19:13]

On 2017/03/31 17:02:33, tzik wrote:
> On 2017/03/31 14:06:57, Sami wrote:
> > Would a DCHECK() be sufficient? That should help catch most of these at
> > development time, right? However given that the try bots are all green maybe
> > this mostly happens in the wild?
> 
> That's currently happen in the wild.
> IIRC, it's third time to me, which is not so frequent, but it's the first
> mac-only one.
> The reported minidumps should have FROM_HERE info in principle as a local
> variable, but it's annoying to debug on a minidump on a platform where I
> regularly work on.

For debugging purposes our common practice (that's written in our coding
guideline) is that we're ok with turning DCHECKs to CHECKs 'temporarily', but
not having them forever.  Could we do the same here?

[Sami, 2017-04-03 10:12:53]

Landing something like this temporarily sounds fine to me.

[tzik, 2017-04-03 13:55:25]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 13:55:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-04-03 13:57:12]

On 2017/04/03 07:19:13, kinuko wrote:
> On 2017/03/31 17:02:33, tzik wrote:
> > On 2017/03/31 14:06:57, Sami wrote:
> > > Would a DCHECK() be sufficient? That should help catch most of these at
> > > development time, right? However given that the try bots are all green
maybe
> > > this mostly happens in the wild?
> > 
> > That's currently happen in the wild.
> > IIRC, it's third time to me, which is not so frequent, but it's the first
> > mac-only one.
> > The reported minidumps should have FROM_HERE info in principle as a local
> > variable, but it's annoying to debug on a minidump on a platform where I
> > regularly work on.
> 
> For debugging purposes our common practice (that's written in our coding
> guideline) is that we're ok with turning DCHECKs to CHECKs 'temporarily', but
> not having them forever.  Could we do the same here?

OK. Updated the CL to use DCHECK all but TaskQueueImpl. Observed failure seems
to happen only there.

[tzik, 2017-04-03 13:57:22]

https://codereview.chromium.org/2785943004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc
(right):

https://codereview.chromium.org/2785943004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:221:
base::AutoLock lock(any_thread_lock_);
On 2017/03/31 14:08:55, Sami wrote:
> I think we just need to instrument PostImmediateTaskImpl and
PostDelayedTaskImpl
> -- everything else is downstream from them.

Done.

[Sami, 2017-04-03 15:04:38]

Thanks, third_party/WebKit/Source/platform/scheduler/ lgtm

[commit-bot: I haz the power, 2017-04-03 16:16:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 16:16:37]

Dry run: This issue passed the CQ dry run.

[gab, 2017-04-03 17:06:17]

lgtm w/ comment (and promise to not lift ReleaseBlock-Beta on
http://crbug.com/706854 so that this CHECK doesn't go beyond Dev).

https://codereview.chromium.org/2785943004/diff/80001/base/task_scheduler/pri...
File base/task_scheduler/priority_queue_unittest.cc (right):

https://codereview.chromium.org/2785943004/diff/80001/base/task_scheduler/pri...
base/task_scheduler/priority_queue_unittest.cc:63: FROM_HERE, Bind([] {}),
Use DoNothing() from bind_helpers.h instead for all of these

[gab, 2017-04-03 17:06:46]

On 2017/04/03 17:06:17, gab wrote:
> lgtm w/ comment (and promise to not lift ReleaseBlock-Beta on
> http://crbug.com/706854 so that this CHECK doesn't go beyond Dev).
> 
>
https://codereview.chromium.org/2785943004/diff/80001/base/task_scheduler/pri...
> File base/task_scheduler/priority_queue_unittest.cc (right):
> 
>
https://codereview.chromium.org/2785943004/diff/80001/base/task_scheduler/pri...
> base/task_scheduler/priority_queue_unittest.cc:63: FROM_HERE, Bind([] {}),
> Use DoNothing() from bind_helpers.h instead for all of these

Also, please update CL description to reflect addition of DCHECKs not CHECKs
(with one temporary CHECK)

[kinuko, 2017-04-03 20:53:15]

Thanks! content/ lgtm

[tzik, 2017-04-04 01:28:43]

Description was changed from

==========
CHECK tasks posted by TaskRunner::PostTask

This CL adds CHECKs to detect null task posted to TaskRunner::PostTask
implementations, so that we can detect it earlier with meaningful stack
trace.

A null task posted to a TaskRunner eventually causes a crash, but its
stack trace is less useful to debug on it. Though the minidump contains
FROM_HERE where it posted, it's not easy to see it if the crash happened
on other platform than a developer works on.

BUG=706854
==========

to

==========
DCHECK tasks posted by TaskRunner::PostTask

This CL adds DCHECKs to detect null task posted to TaskRunner::PostTask
implementations, so that we can detect it earlier with meaningful stack
trace. Plus, this adds CHECK instead of DCHECK to TaskQueueImpl temporarily,
for debugging a crash report in the wild.

BUG=706854
==========

[tzik, 2017-04-04 11:33:44]

Description was changed from

==========
DCHECK tasks posted by TaskRunner::PostTask

This CL adds DCHECKs to detect null task posted to TaskRunner::PostTask
implementations, so that we can detect it earlier with meaningful stack
trace. Plus, this adds CHECK instead of DCHECK to TaskQueueImpl temporarily,
for debugging a crash report in the wild.

BUG=706854
==========

to

==========
DCHECK tasks posted by TaskRunner::PostTask

This CL adds DCHECKs to detect null task posted to TaskRunner::PostTask
implementations, so that we can detect it earlier with meaningful stack
trace.

BUG=706854
==========

[tzik, 2017-04-04 11:33:55]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 11:34:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-04-04 11:36:26]

I identified the culprit in another minidump on Windows. And I'm fixing it
separately. Let me address the TODO I was about to add now.

https://codereview.chromium.org/2785943004/diff/80001/base/task_scheduler/pri...
File base/task_scheduler/priority_queue_unittest.cc (right):

https://codereview.chromium.org/2785943004/diff/80001/base/task_scheduler/pri...
base/task_scheduler/priority_queue_unittest.cc:63: FROM_HERE, Bind([] {}),
On 2017/04/03 17:06:17, gab wrote:
> Use DoNothing() from bind_helpers.h instead for all of these

Done.

[commit-bot: I haz the power, 2017-04-04 11:41:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 11:41:25]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tzik, 2017-04-04 12:03:54]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 12:04:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 13:30:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 13:30:51]

Dry run: This issue passed the CQ dry run.

[gab, 2017-04-04 15:16:05]

Thanks, lgtm++

[sky, 2017-04-04 17:05:22]

LGTM

[tzik, 2017-04-04 17:20:41]

The CQ bit was checked by tzik@chromium.org

[tzik, 2017-04-04 17:20:42]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2785943004/#ps120001
(title: "fix")

[commit-bot: I haz the power, 2017-04-04 17:21:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 17:27:38]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491326441844400,
"parent_rev": "68765ef28c91339c82a2644376aa1018d14b52bd", "commit_rev":
"c69769616ffc1d266a8c4af8be6b957e2f305da8"}

[commit-bot: I haz the power, 2017-04-04 17:28:37]

Description was changed from

==========
DCHECK tasks posted by TaskRunner::PostTask

This CL adds DCHECKs to detect null task posted to TaskRunner::PostTask
implementations, so that we can detect it earlier with meaningful stack
trace.

BUG=706854
==========

to

==========
DCHECK tasks posted by TaskRunner::PostTask

This CL adds DCHECKs to detect null task posted to TaskRunner::PostTask
implementations, so that we can detect it earlier with meaningful stack
trace.

BUG=706854

Review-Url: https://codereview.chromium.org/2785943004
Cr-Commit-Position: refs/heads/master@{#461766}
Committed:
https://chromium.googlesource.com/chromium/src/+/c69769616ffc1d266a8c4af8be6b...
==========

[commit-bot: I haz the power, 2017-04-04 17:28:40]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/c69769616ffc1d266a8c4af8be6b...