Adds a SHUTDOWN_TASK_QUEUE and a PreShutdown api to the scheduler.

Adds a SHUTDOWN_TASK_QUEUE and a PreShutdown api to the scheduler.

Unfortunately webworkers have a complicated shutdown process and we need
to be very careful to prevent execution of timers once the shutdown
process has strated or we risk UAF bugs. This patch adds the concept of
a SHUTDOWN_TASK_QUEUE to the SchedulerHelper (which is always on) and a
(thread safe) PreShutdown API which turns off all other queues except
for the control queue.

BUG=463143

[alex clarke (OOO till 29th), 2015-04-22 15:49:24]

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

[alex clarke (OOO till 29th), 2015-04-22 16:25:34]

alexclarke@chromium.org changed reviewers:
+ sky@chromium.org

[alex clarke (OOO till 29th), 2015-04-22 16:25:34]

+sky@ please review the mojo change, thanks!

[Sami, 2015-04-23 13:14:03]

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
File components/scheduler/child/child_scheduler.h (right):

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/child_scheduler.h:60: // Also ends any idle periods.
If posted from a different thread, it blocks
Do we need to mention idle periods here? If we disallow all queues except
shutdown and control, that already implies that nothing else will run, right?

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
File components/scheduler/child/scheduler_helper.cc (right):

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:104: void
SchedulerHelper::PreShutdown() {
Can we make the client ensure that this is called on the right thread? We don't
really have this pattern for any other method here. It seems scary if unrelated
threads are trying to initiate shutdown on the target thread.

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:120: EndIdlePeriod();
Do we actually need to end the idle period? The idle tasks won't get run because
the queue is disabled?

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:123: for (size_t i =
TASK_QUEUE_COUNT; i < total_task_queue_count_; i++) {
Can we get a policy update that undoes all of this?

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
File components/scheduler/child/web_scheduler_impl.cc (right):

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/web_scheduler_impl.cc:117:
shutdown_task_runner_->PostNonNestableDelayedTask(
Why non-nestable?

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
File components/scheduler/child/web_scheduler_impl.h (right):

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/web_scheduler_impl.h:51: virtual void preShutdown();
minor nit about naming: the pre/post kind of clash here but I don't have a
better suggestion.

Also, please move this before the post* APIs to keep the two separate.

[sky, 2015-04-23 16:35:37]

LGTM

https://codereview.chromium.org/1101703003/diff/20001/mojo/services/html_view...
File mojo/services/html_viewer/web_scheduler_impl.cc (right):

https://codereview.chromium.org/1101703003/diff/20001/mojo/services/html_view...
mojo/services/html_viewer/web_scheduler_impl.cc:69: // TODO(alexclarke)
Implement pre shutdown for mojo?
Add a NOTIMPLEMENTED here.

[alex clarke (OOO till 29th), 2015-04-23 17:34:33]

PTAL

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
File components/scheduler/child/child_scheduler.h (right):

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/child_scheduler.h:60: // Also ends any idle periods.
If posted from a different thread, it blocks
On 2015/04/23 13:14:02, Sami wrote:
> Do we need to mention idle periods here? If we disallow all queues except
> shutdown and control, that already implies that nothing else will run, right?

I guess not.

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
File components/scheduler/child/scheduler_helper.cc (right):

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:104: void
SchedulerHelper::PreShutdown() {
On 2015/04/23 13:14:03, Sami wrote:
> Can we make the client ensure that this is called on the right thread? We
don't
> really have this pattern for any other method here. It seems scary if
unrelated
> threads are trying to initiate shutdown on the target thread.

I was able to do this blink side instead.

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:120: EndIdlePeriod();
On 2015/04/23 13:14:02, Sami wrote:
> Do we actually need to end the idle period? The idle tasks won't get run
because
> the queue is disabled?

Yes I think so because enable_next_long_idle_period_closure_ gets posted on the
control_task_runner_ as does end_idle_period_closure_ (but that's not such a big
deal).

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:123: for (size_t i =
TASK_QUEUE_COUNT; i < total_task_queue_count_; i++) {
On 2015/04/23 13:14:02, Sami wrote:
> Can we get a policy update that undoes all of this?

Yes, but only on the render thread where this isn't intended for use. I think
there is an argument that this should be specific to the worker scheduler, but
that's awkward to surface in blink since there's no worker specific WebThread.

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
File components/scheduler/child/web_scheduler_impl.cc (right):

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/web_scheduler_impl.cc:117:
shutdown_task_runner_->PostNonNestableDelayedTask(
On 2015/04/23 13:14:03, Sami wrote:
> Why non-nestable?

Looks like this wasn't needed after all.

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
File components/scheduler/child/web_scheduler_impl.h (right):

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/web_scheduler_impl.h:51: virtual void preShutdown();
On 2015/04/23 13:14:03, Sami wrote:
> minor nit about naming: the pre/post kind of clash here but I don't have a
> better suggestion.
> 
> Also, please move this before the post* APIs to keep the two separate.

could call it disableAllButShutdownTasks()?

https://codereview.chromium.org/1101703003/diff/20001/mojo/services/html_view...
File mojo/services/html_viewer/web_scheduler_impl.cc (right):

https://codereview.chromium.org/1101703003/diff/20001/mojo/services/html_view...
mojo/services/html_viewer/web_scheduler_impl.cc:69: // TODO(alexclarke)
Implement pre shutdown for mojo?
On 2015/04/23 16:35:37, sky wrote:
> Add a NOTIMPLEMENTED here.

Done.

[alex clarke (OOO till 29th), 2015-04-23 17:48:02]

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

[alex clarke (OOO till 29th), 2015-04-23 17:48:02]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1101703003/#ps40001
(title: "Responding to feedback")

[commit-bot: I haz the power, 2015-04-23 17:48:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1101703003/40001

[Sami, 2015-04-23 18:47:49]

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
File components/scheduler/child/scheduler_helper.cc (right):

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:104: void
SchedulerHelper::PreShutdown() {
On 2015/04/23 17:34:32, alexclarke1 wrote:
> On 2015/04/23 13:14:03, Sami wrote:
> > Can we make the client ensure that this is called on the right thread? We
> don't
> > really have this pattern for any other method here. It seems scary if
> unrelated
> > threads are trying to initiate shutdown on the target thread.
> 
> I was able to do this blink side instead.

Great, thanks.

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:120: EndIdlePeriod();
On 2015/04/23 17:34:32, alexclarke1 wrote:
> On 2015/04/23 13:14:02, Sami wrote:
> > Do we actually need to end the idle period? The idle tasks won't get run
> because
> > the queue is disabled?
> 
> Yes I think so because enable_next_long_idle_period_closure_ gets posted on
the
> control_task_runner_ as does end_idle_period_closure_ (but that's not such a
big
> deal).

We could just let those run. I don't think there should be any bad side effects.

https://codereview.chromium.org/1101703003/diff/40001/components/scheduler/ch...
File components/scheduler/child/scheduler_helper.cc (right):

https://codereview.chromium.org/1101703003/diff/40001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:109:
task_queue_manager_->FlushQueue(QueueId::CONTROL_TASK_AFTER_WAKEUP_QUEUE);
Why do we need to flush again? Won't the selector ensure that no tasks will get
executed from these queues?

[commit-bot: I haz the power, 2015-04-23 20:43:00]

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

[commit-bot: I haz the power, 2015-04-23 20:43:01]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2015-04-24 09:32:36]

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
File components/scheduler/child/scheduler_helper.cc (right):

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:120: EndIdlePeriod();
On 2015/04/23 18:47:49, Sami wrote:
> On 2015/04/23 17:34:32, alexclarke1 wrote:
> > On 2015/04/23 13:14:02, Sami wrote:
> > > Do we actually need to end the idle period? The idle tasks won't get run
> > because
> > > the queue is disabled?
> > 
> > Yes I think so because enable_next_long_idle_period_closure_ gets posted on
> the
> > control_task_runner_ as does end_idle_period_closure_ (but that's not such a
> big
> > deal).
> 
> We could just let those run. I don't think there should be any bad side
effects.

The reason I wrote this patch in the first place is because I got a segfault in
a GC idle task running in the gap between WorkerThread::stop and the thread
actually getting shut down.  We need to stop idle tasks.  Simply disabling the
idle queue isn't enough because there often will be a
enable_next_long_idle_period_closure_ posted on the control queue which we don't
disable.

https://codereview.chromium.org/1101703003/diff/40001/components/scheduler/ch...
File components/scheduler/child/scheduler_helper.cc (right):

https://codereview.chromium.org/1101703003/diff/40001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:109:
task_queue_manager_->FlushQueue(QueueId::CONTROL_TASK_AFTER_WAKEUP_QUEUE);
On 2015/04/23 18:47:49, Sami wrote:
> Why do we need to flush again? Won't the selector ensure that no tasks will
get
> executed from these queues?

I was worried about crashes on shutdown.  I was able to fix these another way. 
We don't need this anymore.

[rmcilroy, 2015-04-24 09:41:01]

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

[rmcilroy, 2015-04-24 09:41:02]

https://codereview.chromium.org/1101703003/diff/60001/components/scheduler/ch...
File components/scheduler/child/child_scheduler.h (right):

https://codereview.chromium.org/1101703003/diff/60001/components/scheduler/ch...
components/scheduler/child/child_scheduler.h:61: // was created on has completed
the pre-shutdown.
I'm not sure what the second sentence means. As far as I can tell this doesn't
block on a different thread?   It looks like this should only be called on the
main thread.

[Sami, 2015-04-24 10:38:40]

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
File components/scheduler/child/scheduler_helper.cc (right):

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:120: EndIdlePeriod();
On 2015/04/24 09:32:35, alexclarke1 wrote:
> On 2015/04/23 18:47:49, Sami wrote:
> > On 2015/04/23 17:34:32, alexclarke1 wrote:
> > > On 2015/04/23 13:14:02, Sami wrote:
> > > > Do we actually need to end the idle period? The idle tasks won't get run
> > > because
> > > > the queue is disabled?
> > > 
> > > Yes I think so because enable_next_long_idle_period_closure_ gets posted
on
> > the
> > > control_task_runner_ as does end_idle_period_closure_ (but that's not such
a
> > big
> > > deal).
> > 
> > We could just let those run. I don't think there should be any bad side
> effects.
> 
> The reason I wrote this patch in the first place is because I got a segfault
in
> a GC idle task running in the gap between WorkerThread::stop and the thread
> actually getting shut down.  We need to stop idle tasks.  Simply disabling the
> idle queue isn't enough because there often will be a
> enable_next_long_idle_period_closure_ posted on the control queue which we
don't
> disable.

That sounds like an instance of the bug I was worried about: something goes and
changes the set of enabled queues after this task has run. I think we need to
guard against that in a more sound way. As we add new scheduling primitives it's
very easy to forget to account for them here.

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:123: for (size_t i =
TASK_QUEUE_COUNT; i < total_task_queue_count_; i++) {
On 2015/04/23 17:34:32, alexclarke1 wrote:
> On 2015/04/23 13:14:02, Sami wrote:
> > Can we get a policy update that undoes all of this?
> 
> Yes, but only on the render thread where this isn't intended for use. I think
> there is an argument that this should be specific to the worker scheduler, but
> that's awkward to surface in blink since there's no worker specific WebThread.

Looks like my reply here got lost somehow: I don't think we can rely on the fact
that this only happens in a Worker here. Maybe we can isolate the selector so
that it becomes impossible to enable any additional queues after this function
has been called?

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
File components/scheduler/child/web_scheduler_impl.h (right):

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/web_scheduler_impl.h:51: virtual void preShutdown();
On 2015/04/23 17:34:32, alexclarke1 wrote:
> On 2015/04/23 13:14:03, Sami wrote:
> > minor nit about naming: the pre/post kind of clash here but I don't have a
> > better suggestion.
> > 
> > Also, please move this before the post* APIs to keep the two separate.
> 
> could call it disableAllButShutdownTasks()?

I think it's the fact that it's a less like a verb compared to the other
functions here. disableAllButShutdownTasks() sounds better.

[alex clarke (OOO till 29th), 2015-04-24 14:10:05]

PTAL

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
File components/scheduler/child/scheduler_helper.cc (right):

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:120: EndIdlePeriod();
On 2015/04/24 10:38:40, Sami wrote:
> On 2015/04/24 09:32:35, alexclarke1 wrote:
> > On 2015/04/23 18:47:49, Sami wrote:
> > > On 2015/04/23 17:34:32, alexclarke1 wrote:
> > > > On 2015/04/23 13:14:02, Sami wrote:
> > > > > Do we actually need to end the idle period? The idle tasks won't get
run
> > > > because
> > > > > the queue is disabled?
> > > > 
> > > > Yes I think so because enable_next_long_idle_period_closure_ gets posted
> on
> > > the
> > > > control_task_runner_ as does end_idle_period_closure_ (but that's not
such
> a
> > > big
> > > > deal).
> > > 
> > > We could just let those run. I don't think there should be any bad side
> > effects.
> > 
> > The reason I wrote this patch in the first place is because I got a segfault
> in
> > a GC idle task running in the gap between WorkerThread::stop and the thread
> > actually getting shut down.  We need to stop idle tasks.  Simply disabling
the
> > idle queue isn't enough because there often will be a
> > enable_next_long_idle_period_closure_ posted on the control queue which we
> don't
> > disable.
> 
> That sounds like an instance of the bug I was worried about: something goes
and
> changes the set of enabled queues after this task has run. I think we need to
> guard against that in a more sound way. As we add new scheduling primitives
it's
> very easy to forget to account for them here.

Done.

https://codereview.chromium.org/1101703003/diff/20001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.cc:123: for (size_t i =
TASK_QUEUE_COUNT; i < total_task_queue_count_; i++) {
On 2015/04/24 10:38:40, Sami wrote:
> On 2015/04/23 17:34:32, alexclarke1 wrote:
> > On 2015/04/23 13:14:02, Sami wrote:
> > > Can we get a policy update that undoes all of this?
> > 
> > Yes, but only on the render thread where this isn't intended for use. I
think
> > there is an argument that this should be specific to the worker scheduler,
but
> > that's awkward to surface in blink since there's no worker specific
WebThread.
> 
> Looks like my reply here got lost somehow: I don't think we can rely on the
fact
> that this only happens in a Worker here. Maybe we can isolate the selector so
> that it becomes impossible to enable any additional queues after this function
> has been called?

Done.

https://codereview.chromium.org/1101703003/diff/60001/components/scheduler/ch...
File components/scheduler/child/child_scheduler.h (right):

https://codereview.chromium.org/1101703003/diff/60001/components/scheduler/ch...
components/scheduler/child/child_scheduler.h:61: // was created on has completed
the pre-shutdown.
On 2015/04/24 09:41:02, rmcilroy wrote:
> I'm not sure what the second sentence means. As far as I can tell this doesn't
> block on a different thread?   It looks like this should only be called on the
> main thread.

This comment was duplicated and I forgot to change this one, thanks.

[rmcilroy, 2015-04-24 14:17:28]

https://codereview.chromium.org/1101703003/diff/80001/components/scheduler/ch...
File components/scheduler/child/scheduler_helper.h (right):

https://codereview.chromium.org/1101703003/diff/80001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.h:91: // until the thread this class
was created on has completed the pre-shutdown.
Same comment here.

[alex clarke (OOO till 29th), 2015-04-24 14:30:57]

https://codereview.chromium.org/1101703003/diff/80001/components/scheduler/ch...
File components/scheduler/child/scheduler_helper.h (right):

https://codereview.chromium.org/1101703003/diff/80001/components/scheduler/ch...
components/scheduler/child/scheduler_helper.h:91: // until the thread this class
was created on has completed the pre-shutdown.
On 2015/04/24 14:17:27, rmcilroy wrote:
> Same comment here.

Done.

[alex clarke (OOO till 29th), 2015-04-24 14:38:52]

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

[alex clarke (OOO till 29th), 2015-04-24 14:38:53]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1101703003/#ps100001
(title: "Fix comment")

[commit-bot: I haz the power, 2015-04-24 14:39:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1101703003/100001

[Sami, 2015-04-24 15:39:28]

Like we discussed, let's make the necessary parts of the selector part of the
helper's interface so we can verify the shutdown state there.

[commit-bot: I haz the power, 2015-04-24 15:40:47]

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

[commit-bot: I haz the power, 2015-04-24 15:40:48]

Dry run: This issue passed the CQ dry run.

[Sami, 2015-04-27 16:53:34]

FYI, I took over this patch in https://codereview.chromium.org/1106213002.