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 started or we risk UAF bugs. This patch adds the concept of
a SHUTDOWN_TASK_QUEUE to the SchedulerHelper (which is always on) and a
PreShutdown API which turns off all other queues except for the shutdown and
control ones.

Original patch by Alex Clarke <alexclarke@chromium.org>; (https://codereview.chromium.org/1101703003/).

BUG=463143
TBR=sky@chromium.org

[Sami, 2015-04-27 16:54:28]

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

[Sami, 2015-04-27 16:57:47]

Ross, mind having a look? The main difference to before is that the selector no
longer knows about the preshutdown stage.

TBR sky@ because the html_viewer part was already reviewed in
https://codereview.chromium.org/1101703003/.

[rmcilroy, 2015-04-28 09:51:10]

A couple of questions / suggestions but lgtm - although I really wish we didn't
need to do this :(.

https://codereview.chromium.org/1106213002/diff/1/components/scheduler/child/...
File components/scheduler/child/scheduler_helper.cc (right):

https://codereview.chromium.org/1106213002/diff/1/components/scheduler/child/...
components/scheduler/child/scheduler_helper.cc:108: if (i == CONTROL_TASK_QUEUE
|| i == SHUTDOWN_TASK_QUEUE)
I'm wondering whether we even need the control task queue here? I don't think we
need anything on that queue after preshutdown, but it might be safer to keep it
running. I'll leave it up to you.

https://codereview.chromium.org/1106213002/diff/1/components/scheduler/child/...
File components/scheduler/child/webthread_impl_for_worker_scheduler_unittest.cc
(right):

https://codereview.chromium.org/1106213002/diff/1/components/scheduler/child/...
components/scheduler/child/webthread_impl_for_worker_scheduler_unittest.cc:129:
base::Lock lock;
Can you not just use a WaitableEvent and check !completion.IsSignaled() in
TestThreadShutdownAfterPreShutdown?

[Sami, 2015-04-28 11:52:47]

Thanks Ross!

> A couple of questions / suggestions but lgtm - although I really wish we
didn't
> need to do this :(.

Likewise :P However given the way worker threads are set up on the Blink side I
don't see a simpler option. I guess one option would be to wrap all worker tasks
-- a little how is being done at the moment but maybe more at the WebThread
level. I think I'll see how that would look before landing this.

https://codereview.chromium.org/1106213002/diff/1/components/scheduler/child/...
File components/scheduler/child/scheduler_helper.cc (right):

https://codereview.chromium.org/1106213002/diff/1/components/scheduler/child/...
components/scheduler/child/scheduler_helper.cc:108: if (i == CONTROL_TASK_QUEUE
|| i == SHUTDOWN_TASK_QUEUE)
On 2015/04/28 09:51:09, rmcilroy wrote:
> I'm wondering whether we even need the control task queue here? I don't think
we
> need anything on that queue after preshutdown, but it might be safer to keep
it
> running. I'll leave it up to you.

I thought Alex originally made kept the control queue stay alive because the
worker shutdown task was posted through there, but looks like that actually goes
through the shutdown queue. I think I agree that just having one queue be
special is better, so I'll do that. I'll run the layout tests to see if anything
breaks before landing though.

https://codereview.chromium.org/1106213002/diff/1/components/scheduler/child/...
File components/scheduler/child/scheduler_helper_unittest.cc (right):

https://codereview.chromium.org/1106213002/diff/1/components/scheduler/child/...
components/scheduler/child/scheduler_helper_unittest.cc:932:
PostTestTasks(&run_order, "D1 I1 T1");
I noticed we weren't actually posting a shutdown task. Fixed.

https://codereview.chromium.org/1106213002/diff/1/components/scheduler/child/...
File components/scheduler/child/webthread_impl_for_worker_scheduler_unittest.cc
(right):

https://codereview.chromium.org/1106213002/diff/1/components/scheduler/child/...
components/scheduler/child/webthread_impl_for_worker_scheduler_unittest.cc:129:
base::Lock lock;
On 2015/04/28 09:51:10, rmcilroy wrote:
> Can you not just use a WaitableEvent and check !completion.IsSignaled() in
> TestThreadShutdownAfterPreShutdown?

Good idea, that's cleaner.

Also found out that these tests weren't actually being run anymore:
https://codereview.chromium.org/1112523003/ :P