Fix bug with TaskQueueSelector and blocked queues

Fix bug with TaskQueueSelector and blocked queues

The TaskQueueSelector is only supposed to touch the blocked_selector_
iff queue->should_report_when_execution_blocked() is true. Unfortunately
TaskQueueSelector::EnableQueue unconditionally added queues to the
blocked_selector_ leading to a potential UAF.

BUG=581973, 584544, 582712, 585744
Committed: https://crrev.com/fae98b1d855879a11105a17be6b095fa4446f2da
Cr-Commit-Position: refs/heads/master@{#374692}

[alex clarke (OOO till 29th), 2016-02-10 12:21:53]

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

[commit-bot: I haz the power, 2016-02-10 12:22:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1685093002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1685093002/1

[alex clarke (OOO till 29th), 2016-02-10 12:25:56]

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

[alex clarke (OOO till 29th), 2016-02-10 12:26:12]

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

[alex clarke (OOO till 29th), 2016-02-10 12:26:13]

PTAL

[commit-bot: I haz the power, 2016-02-10 12:26:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1685093002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1685093002/20001

[commit-bot: I haz the power, 2016-02-10 12:40:55]

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

[commit-bot: I haz the power, 2016-02-10 12:40:56]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Sami, 2016-02-10 13:15:48]

Thanks for tracking this down -- great find!

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
File components/scheduler/base/task_queue_selector.cc (right):

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
components/scheduler/base/task_queue_selector.cc:35:
DCHECK(!blocked_selector_.immediate_work_queue_sets()
How about a helper on the selector that combines these two checks into one? We
could also use that in
TaskQueueSelector::PrioritizingSelector::AddQueue/ChangeSetIndex.

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
components/scheduler/base/task_queue_selector.cc:84:
queue->delayed_work_queue()->AssignSetIndex(priority);
Mind adding a comment here why we need to do this assignment here?

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
components/scheduler/base/task_queue_selector.cc:127: // The #if DCHECK_IS_ON()
shouldn't be necessary but this doesn't compile on
nit: might not need to repeat this comment since it's still on the same screen.

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
File components/scheduler/base/work_queue_sets.cc (right):

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
components/scheduler/base/work_queue_sets.cc:27:
enqueue_order_to_work_queue_maps_[set_index].insert(
Should we call OnPushQueue instead? If we ever add anything there we might
forget to do it here too.

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
components/scheduler/base/work_queue_sets.cc:133: if
(work_queue->work_queue_sets() == this) {
When would the work queue be assigned to a set but not actually be part of it?
Could we avoid this situation?

[alex clarke (OOO till 29th), 2016-02-10 14:35:01]

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

[commit-bot: I haz the power, 2016-02-10 14:35:55]

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

[alex clarke (OOO till 29th), 2016-02-10 14:37:13]

PTAL

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
File components/scheduler/base/task_queue_selector.cc (right):

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
components/scheduler/base/task_queue_selector.cc:35:
DCHECK(!blocked_selector_.immediate_work_queue_sets()
On 2016/02/10 13:15:47, Sami wrote:
> How about a helper on the selector that combines these two checks into one? We
> could also use that in
> TaskQueueSelector::PrioritizingSelector::AddQueue/ChangeSetIndex.

Done.

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
components/scheduler/base/task_queue_selector.cc:84:
queue->delayed_work_queue()->AssignSetIndex(priority);
On 2016/02/10 13:15:47, Sami wrote:
> Mind adding a comment here why we need to do this assignment here?

Done.

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
File components/scheduler/base/work_queue_sets.cc (right):

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
components/scheduler/base/work_queue_sets.cc:27:
enqueue_order_to_work_queue_maps_[set_index].insert(
On 2016/02/10 13:15:47, Sami wrote:
> Should we call OnPushQueue instead? If we ever add anything there we might
> forget to do it here too.

We'd end up calling work_queue->GetFrontTaskEnqueueOrder twice which feels a bit
unfortunate :(

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
components/scheduler/base/work_queue_sets.cc:133: if
(work_queue->work_queue_sets() == this) {
On 2016/02/10 13:15:47, Sami wrote:
> When would the work queue be assigned to a set but not actually be part of it?
> Could we avoid this situation?

Consider:

WorkQueueSets a;
WorkQueueSets b;
WorkQueue q;

a.AddQueue(&q, 1);
DCHECK(a.ContainsWorkQueueForTest(&q));
DCHECK(!b.ContainsWorkQueueForTest(&q));

|q| must only be in one WorkQueueSets at a time.

if |has_enqueue_order| is not null (ie. its not empty) then it must have an
entry in enqueue_order_to_work_queue_maps_.

That's what it's guarding against.

[commit-bot: I haz the power, 2016-02-10 14:52:39]

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

[commit-bot: I haz the power, 2016-02-10 14:52:40]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[alex clarke (OOO till 29th), 2016-02-10 15:01:15]

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

[commit-bot: I haz the power, 2016-02-10 15:03:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1685093002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1685093002/60001

[Sami, 2016-02-10 15:30:38]

lgtm with one suggestion.

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
File components/scheduler/base/work_queue_sets.cc (right):

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
components/scheduler/base/work_queue_sets.cc:27:
enqueue_order_to_work_queue_maps_[set_index].insert(
On 2016/02/10 14:37:12, alexclarke1 wrote:
> On 2016/02/10 13:15:47, Sami wrote:
> > Should we call OnPushQueue instead? If we ever add anything there we might
> > forget to do it here too.
> 
> We'd end up calling work_queue->GetFrontTaskEnqueueOrder twice which feels a
bit
> unfortunate :(

Yeah, I was hoping the compiler would noticed that. Let's add a comment to
OnPushQueue that points here instead?

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
components/scheduler/base/work_queue_sets.cc:133: if
(work_queue->work_queue_sets() == this) {
On 2016/02/10 14:37:12, alexclarke1 wrote:
> On 2016/02/10 13:15:47, Sami wrote:
> > When would the work queue be assigned to a set but not actually be part of
it?
> > Could we avoid this situation?
> 
> Consider:
> 
> WorkQueueSets a;
> WorkQueueSets b;
> WorkQueue q;
> 
> a.AddQueue(&q, 1);
> DCHECK(a.ContainsWorkQueueForTest(&q));
> DCHECK(!b.ContainsWorkQueueForTest(&q));
> 
> |q| must only be in one WorkQueueSets at a time.
> 
> if |has_enqueue_order| is not null (ie. its not empty) then it must have an
> entry in enqueue_order_to_work_queue_maps_.
> 
> That's what it's guarding against.
> 
> 
> 

Ah, the case of an empty queue. Makes sense, thanks.

[commit-bot: I haz the power, 2016-02-10 15:48:13]

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

[commit-bot: I haz the power, 2016-02-10 15:48:13]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[alex clarke (OOO till 29th), 2016-02-10 17:25:35]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2016-02-10 17:25:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1685093002/#ps80001
(title: "Fix UAF")

[alex clarke (OOO till 29th), 2016-02-10 17:26:08]

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
File components/scheduler/base/work_queue_sets.cc (right):

https://codereview.chromium.org/1685093002/diff/20001/components/scheduler/ba...
components/scheduler/base/work_queue_sets.cc:27:
enqueue_order_to_work_queue_maps_[set_index].insert(
On 2016/02/10 15:30:38, Sami wrote:
> On 2016/02/10 14:37:12, alexclarke1 wrote:
> > On 2016/02/10 13:15:47, Sami wrote:
> > > Should we call OnPushQueue instead? If we ever add anything there we might
> > > forget to do it here too.
> > 
> > We'd end up calling work_queue->GetFrontTaskEnqueueOrder twice which feels a
> bit
> > unfortunate :(
> 
> Yeah, I was hoping the compiler would noticed that. Let's add a comment to
> OnPushQueue that points here instead?

Done.

[commit-bot: I haz the power, 2016-02-10 17:28:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1685093002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1685093002/80001

[commit-bot: I haz the power, 2016-02-10 18:48:31]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-10 18:49:39]

Description was changed from

==========
Fix bug with TaskQueueSelector and blocked queues

The TaskQueueSelector is only supposed to touch the blocked_selector_
iff queue->should_report_when_execution_blocked() is true. Unfortunately
TaskQueueSelector::EnableQueue unconditionally added queues to the
blocked_selector_ leading to a potential UAF.

BUG=581973, 584544, 582712, 585744
==========

to

==========
Fix bug with TaskQueueSelector and blocked queues

The TaskQueueSelector is only supposed to touch the blocked_selector_
iff queue->should_report_when_execution_blocked() is true. Unfortunately
TaskQueueSelector::EnableQueue unconditionally added queues to the
blocked_selector_ leading to a potential UAF.

BUG=581973, 584544, 582712, 585744

Committed: https://crrev.com/fae98b1d855879a11105a17be6b095fa4446f2da
Cr-Commit-Position: refs/heads/master@{#374692}
==========

[commit-bot: I haz the power, 2016-02-10 18:49:39]

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