scheduler: Don't access weak pointers on foreign threads

scheduler: Don't access weak pointers on foreign threads

The Task Queue Manager previously would give clients a
SingleThreadTaskRunner which held a weak pointer back to the manager in
order to post tasks. This is wrong because the task runner and hence
the weak pointer may be accessed from arbitrary threads, which is
not safe.

This patch corrects the problem by combining the task runner and the
incoming task queue into a single task queue object which owns both
the incoming task as well as the pointer back to the task queue
manager. This allows us to protect the incoming task queue and the
pointer using the same lock.

BUG=391005
Committed: https://crrev.com/b03818a3791ae90d3cf7014de69bfe44f831d192
Cr-Commit-Position: refs/heads/master@{#301085}

[Sami, 2014-10-23 17:45:52]

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

[Sami, 2014-10-23 17:46:41]

Ross, could you do the first pass here? I ended up having to shuffle things
around a bit so that we can protect both the incoming task queue as well as the
pointer to the QM with a single lock.

[rmcilroy, 2014-10-24 09:36:41]

Overall looks good, thanks.  A couple of comments.

https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/task_queue_manager.cc (right):

https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/task_queue_manager.cc:37: void PumpQueueLocked();
Nit - private

https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/task_queue_manager.cc:99:
Bind(&TaskQueue::EnqueueTask, task_queue_weak_ptr_, pending_task),
Could you just use a standard refcnt here (and avoid the weak_ptr entirely)? I
don't think holding a strong ref here would be an issue - the messageQueue
deletes or drains pending tasks when it shuts down, so while a strong ref may
keep the TaskQueue around for slightly longer, it shouldn't ever cause it to
leak.  WDYT?

https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/task_queue_manager_unittest.cc:331: void
PostTaskToRunner(scoped_refptr<base::SingleThreadTaskRunner> runner,
Thanks for adding a test!

[Sami, 2014-10-24 09:59:12]

Thanks for the review!

https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/task_queue_manager.cc (right):

https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/task_queue_manager.cc:37: void PumpQueueLocked();
On 2014/10/24 09:36:41, rmcilroy wrote:
> Nit - private

Done.

https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/task_queue_manager.cc:99:
Bind(&TaskQueue::EnqueueTask, task_queue_weak_ptr_, pending_task),
On 2014/10/24 09:36:41, rmcilroy wrote:
> Could you just use a standard refcnt here (and avoid the weak_ptr entirely)? I
> don't think holding a strong ref here would be an issue - the messageQueue
> deletes or drains pending tasks when it shuts down, so while a strong ref may
> keep the TaskQueue around for slightly longer, it shouldn't ever cause it to
> leak.  WDYT?

Great point, since the parent is already refcounted we can do exactly what you
said.

https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/task_queue_manager_unittest.cc:331: void
PostTaskToRunner(scoped_refptr<base::SingleThreadTaskRunner> runner,
On 2014/10/24 09:36:41, rmcilroy wrote:
> Thanks for adding a test!

My pleasure :)

[rmcilroy, 2014-10-24 11:08:51]

On 2014/10/24 09:59:12, Sami wrote:
> Thanks for the review!
> 
>
https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
> File content/renderer/scheduler/task_queue_manager.cc (right):
> 
>
https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
> content/renderer/scheduler/task_queue_manager.cc:37: void PumpQueueLocked();
> On 2014/10/24 09:36:41, rmcilroy wrote:
> > Nit - private
> 
> Done.
> 
>
https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
> content/renderer/scheduler/task_queue_manager.cc:99:
> Bind(&TaskQueue::EnqueueTask, task_queue_weak_ptr_, pending_task),
> On 2014/10/24 09:36:41, rmcilroy wrote:
> > Could you just use a standard refcnt here (and avoid the weak_ptr entirely)?
I
> > don't think holding a strong ref here would be an issue - the messageQueue
> > deletes or drains pending tasks when it shuts down, so while a strong ref
may
> > keep the TaskQueue around for slightly longer, it shouldn't ever cause it to
> > leak.  WDYT?
> 
> Great point, since the parent is already refcounted we can do exactly what you
> said.
> 
>
https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
> File content/renderer/scheduler/task_queue_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/673833002/diff/20001/content/renderer/schedul...
> content/renderer/scheduler/task_queue_manager_unittest.cc:331: void
> PostTaskToRunner(scoped_refptr<base::SingleThreadTaskRunner> runner,
> On 2014/10/24 09:36:41, rmcilroy wrote:
> > Thanks for adding a test!
> 
> My pleasure :)

LGTM, thanks!

[Sami, 2014-10-24 11:10:07]

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

[Sami, 2014-10-24 11:11:37]

Here's another scheduler patch. It looks scarier than it is -- honest! :) It
effectively moves code around so we can get everything behind a single
(per-queue) lock. No changes to the client interface or the task managing
algorithm. PTAL.

[jochen (gone - plz use gerrit), 2014-10-24 11:42:51]

rubberstamp lgtm

[Sami, 2014-10-24 11:52:54]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-10-24 11:53:47]

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

[commit-bot: I haz the power, 2014-10-24 12:39:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-24 12:39:56]

Patchset 3 (id:??) landed as
https://crrev.com/b03818a3791ae90d3cf7014de69bfe44f831d192
Cr-Commit-Position: refs/heads/master@{#301085}