Issue 717380  has been merged into this issue.

Issue 728235 has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b2b72df4bdc0217e7ed7cc321648028a429edf61

commit b2b72df4bdc0217e7ed7cc321648028a429edf61
Author: gab <gab@chromium.org>
Date: Fri Jun 02 21:32:23 2017

Disable DCHECK for no BLOCK_SHUTDOWN posted after TaskScheduler shutdown.

Until it's possible for async observation services to post non-BLOCK_SHUTDOWN
tasks to otherwise BLOCK_SHUTDOWN sequences, this DCHECK will be flaky.

clang-format off is required or I couldn't find a way to make it happy with the
comment (either inside or outside the #if)

BUG=728235, 698140

Review-Url: https://codereview.chromium.org/2916673003
Cr-Commit-Position: refs/heads/master@{#476799}

[modify] https://crrev.com/b2b72df4bdc0217e7ed7cc321648028a429edf61/base/task_scheduler/task_tracker.cc
[modify] https://crrev.com/b2b72df4bdc0217e7ed7cc321648028a429edf61/base/task_scheduler/task_tracker_unittest.cc

Re. https://bugs.chromium.org/p/chromium/issues/detail?id=771701#c3, why can't we just stop the service thread in Shutdown/JoinForTesting()?

The last time we tried to do this was
https://codereview.chromium.org/2857103005/

The comment I left was...
Stopping the service thread requires some non-trivial task lifetime
fixes to do it correctly, so this was deemed the next best quick fix
for redirection.

IIRC, there was an interplay between the service thread and DelayedTaskManager on shutdown that caused bad things to happen. We punted on the issue then.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/adf845b5cb5e7d320c715256da11841ec43ed317

commit adf845b5cb5e7d320c715256da11841ec43ed317
Author: Scott Graham <scottmg@chromium.org>
Date: Tue Oct 17 01:38:09 2017

Stop TaskScheduler service thread before joining workers

If the service thread is stopped after workers are joined, the service
thread can post a task after join_called_for_testing_ is set which
causes a CHECK. Instead stop the service thread before joining the pool
workers in TaskSchedulerImpl::JoinForTesting().

This bug showed up on Fuchsia, but is cross-platform.

Bug:  771701 , 698140
Change-Id: I4a25ede2bceb700eeea17ae47f7402211856bef6
Reviewed-on: https://chromium-review.googlesource.com/721570
Reviewed-by: Robert Liao <robliao@chromium.org>
Commit-Queue: Scott Graham <scottmg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509208}
[modify] https://crrev.com/adf845b5cb5e7d320c715256da11841ec43ed317/base/task_scheduler/task_scheduler_impl.cc

I don't remember why this was deemed hard but it feels to me like we should just stop the service thread on Shutdown() and it shouldn't be hard to plumb the service thread to TaskTracker for this..?

Can BLOCK_SHUTDOWN tasks expect to be able to watch file descriptor during shutdown? What about a SKIP_ON_SHUTDOWN task that was scheduled before shutdown start?

Ideally, service thread should stop running tasks atomically when shutdown completes.

Good point. It'd be more robust to stop it "atomically" with the end of shutdown.

This is going to require a bit of a Callback dance I think.

@etienneb I think this is a good first bug to dip your toes in TaskScheduler while getting to use many core Chromium constructs.

Discussed offline with Francois, we'll just go ahead and stop the service thread at the beginning of TaskTracker::Shutdown() -- stopping at the end of Shutdown() is hard and not terribly useful (requires a tricky callback dance for pools and service thread to agree they're both at zero and will no longer accept tasks at the same time).

This will prevent delayed tasks and file descriptor watches bound to BLOCK_SHUTDOWN sequences from running during shutdown but those should all be async events which aren't guaranteed to fire in the first place so it doesn't matter. The only one which is unclear is file descriptor watchers that are effectively used as "synchronous" ends of a communication pipe and bound to a BLOCK_SHUTDOWN sequence. Those can technically assume the other end of the pipe will pickup the notification (e.g. WatchReadable()) before they return from their call (e.g. WriteFile()). We decided not to support this communication use case during shutdown as it's far-fetched and we do not believe this is how read-write pipes are used in practice in chrome today (ignoring IPCs because BrowserThread::IO is already stopped when TaskScheduler::Shutdown() kicks in).

Before making this change, we will need to update FileDescriptorWatcher's API to no longer take a MessageLoopForIO* but rather a scoped_refptr<SingleThreadTaskRunner> pointing to that loop. This is because invoking service_thread_->Stop() will delete the MessageLoopForIO bound to it and using the raw pointer is now unsafe. All FileDescriptorWatcher needs in practice is a SingleThreadTaskRunner, it merely used a MessageLoopForIO to enforce that the tasks are posted to an IO capable destination (we will address that by posting [](){DCHECK(MessageLoopForIOCurrent::IsSet());} in DCHECK_IS_ON() builds).

@etienneb: let us know when you're ready for your TaskScheduler crash course :) (I'd recommend not trying to make sense of the above paragraphs until then..!)