Fix a bug with throttling and timer queue suspension

Fix a bug with throttling and timer queue suspension

Both the ThrottlingHelper and timer queue suspension (via
SuspendTimerQueue/ResumeTimerQueue and
SuspendTimerQueueWhenBackgrounded / ResumeTimerQueueWhenForegrounded)
call TaskQueue::SetEnabled and it was possible for the ThrottlingHelper
to re-enable a suspended queue by mistake.

This patch prevents that from happening.

BUG=616052
Committed: https://crrev.com/22ca5b9bce1453364fdb32979af7175ccf234e15
Cr-Commit-Position: refs/heads/master@{#397437}

[alex clarke (OOO till 29th), 2016-05-31 15:37:02]

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

[alex clarke (OOO till 29th), 2016-05-31 15:37:15]

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

[commit-bot: I haz the power, 2016-05-31 15:37:16]

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

[alex clarke (OOO till 29th), 2016-05-31 15:50:45]

Description was changed from

==========
Fix a bug where throttling and timer queue suspension

Both the ThrottlingHelper and timer queue suspension (via
SuspendTimerQueue/ResumeTimerQueue and
SuspendTimerQueueWhenBackgrounded / ResumeTimerQueueWhenForegrounded)
call TaskQueue::SetEnabled and it was possible for the ThrottlingHelper
to re-enable a suspended queue by mistake.

This patch prevents that from happening.

BUG=616052
==========

to

==========
Fix a bug where throttling and timer queue suspension

Both the ThrottlingHelper and timer queue suspension (via
SuspendTimerQueue/ResumeTimerQueue and
SuspendTimerQueueWhenBackgrounded / ResumeTimerQueueWhenForegrounded)
call TaskQueue::SetEnabled and it was possible for the ThrottlingHelper
to re-enable a suspended queue by mistake.

This patch prevents that from happening.

BUG=616052
==========

[alex clarke (OOO till 29th), 2016-05-31 15:50:58]

Description was changed from

==========
Fix a bug where throttling and timer queue suspension

Both the ThrottlingHelper and timer queue suspension (via
SuspendTimerQueue/ResumeTimerQueue and
SuspendTimerQueueWhenBackgrounded / ResumeTimerQueueWhenForegrounded)
call TaskQueue::SetEnabled and it was possible for the ThrottlingHelper
to re-enable a suspended queue by mistake.

This patch prevents that from happening.

BUG=616052
==========

to

==========
Fix a bug with throttling and timer queue suspension

Both the ThrottlingHelper and timer queue suspension (via
SuspendTimerQueue/ResumeTimerQueue and
SuspendTimerQueueWhenBackgrounded / ResumeTimerQueueWhenForegrounded)
call TaskQueue::SetEnabled and it was possible for the ThrottlingHelper
to re-enable a suspended queue by mistake.

This patch prevents that from happening.

BUG=616052
==========

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

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

[commit-bot: I haz the power, 2016-05-31 17:09:04]

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

[commit-bot: I haz the power, 2016-05-31 17:09:05]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-05-31 18:11:05]

lgtm % comments.

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:225:
throttling_helper_->IncreaseThrottleRefCount(timer_task_queue.get());
Good catch with these two.

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:910: // if
(new_task_queue_policy.time_domain_type != TimeDomainType::THROTTLED)
Debugging left in by mistake?

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:1036: //
DCHECK(!default_timer_task_runner_->IsQueueEnabled());
Ditto.

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
File components/scheduler/renderer/throttling_helper.cc (right):

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
components/scheduler/renderer/throttling_helper.cc:56: if (!enabled)
Maybe add a comment why we don't want to enable the queue directly here? (it
could defeat throttling).

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
components/scheduler/renderer/throttling_helper.cc:140:
task_queue->SetQueueEnabled(map_entry.second.enabled);
Mind adding a DCHECK here that makes sure the enabled status is what we expect?

[alex clarke (OOO till 29th), 2016-06-02 13:51:05]

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:225:
throttling_helper_->IncreaseThrottleRefCount(timer_task_queue.get());
On 2016/05/31 18:11:04, Sami wrote:
> Good catch with these two.

Acknowledged.

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:910: // if
(new_task_queue_policy.time_domain_type != TimeDomainType::THROTTLED)
On 2016/05/31 18:11:04, Sami wrote:
> Debugging left in by mistake?

Done.

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:1036: //
DCHECK(!default_timer_task_runner_->IsQueueEnabled());
On 2016/05/31 18:11:04, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
File components/scheduler/renderer/throttling_helper.cc (right):

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
components/scheduler/renderer/throttling_helper.cc:56: if (!enabled)
On 2016/05/31 18:11:04, Sami wrote:
> Maybe add a comment why we don't want to enable the queue directly here? (it
> could defeat throttling).

Done.

https://codereview.chromium.org/2028433004/diff/20001/components/scheduler/re...
components/scheduler/renderer/throttling_helper.cc:140:
task_queue->SetQueueEnabled(map_entry.second.enabled);
On 2016/05/31 18:11:04, Sami wrote:
> Mind adding a DCHECK here that makes sure the enabled status is what we
expect?

This in principle is a good idea, but due to the way
ThrottlingHelper::SetQueueEnabled operates it's a bit complex.  I think we can
get away without doing it.

[alex clarke (OOO till 29th), 2016-06-02 13:51:09]

The CQ bit was checked by alexclarke@chromium.org

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

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/2028433004/#ps40001
(title: "Sami's comments")

[commit-bot: I haz the power, 2016-06-02 13:51:33]

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

[commit-bot: I haz the power, 2016-06-02 16:14:57]

Description was changed from

==========
Fix a bug with throttling and timer queue suspension

Both the ThrottlingHelper and timer queue suspension (via
SuspendTimerQueue/ResumeTimerQueue and
SuspendTimerQueueWhenBackgrounded / ResumeTimerQueueWhenForegrounded)
call TaskQueue::SetEnabled and it was possible for the ThrottlingHelper
to re-enable a suspended queue by mistake.

This patch prevents that from happening.

BUG=616052
==========

to

==========
Fix a bug with throttling and timer queue suspension

Both the ThrottlingHelper and timer queue suspension (via
SuspendTimerQueue/ResumeTimerQueue and
SuspendTimerQueueWhenBackgrounded / ResumeTimerQueueWhenForegrounded)
call TaskQueue::SetEnabled and it was possible for the ThrottlingHelper
to re-enable a suspended queue by mistake.

This patch prevents that from happening.

BUG=616052
==========

[commit-bot: I haz the power, 2016-06-02 16:14:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-02 16:16:50]

Description was changed from

==========
Fix a bug with throttling and timer queue suspension

Both the ThrottlingHelper and timer queue suspension (via
SuspendTimerQueue/ResumeTimerQueue and
SuspendTimerQueueWhenBackgrounded / ResumeTimerQueueWhenForegrounded)
call TaskQueue::SetEnabled and it was possible for the ThrottlingHelper
to re-enable a suspended queue by mistake.

This patch prevents that from happening.

BUG=616052
==========

to

==========
Fix a bug with throttling and timer queue suspension

Both the ThrottlingHelper and timer queue suspension (via
SuspendTimerQueue/ResumeTimerQueue and
SuspendTimerQueueWhenBackgrounded / ResumeTimerQueueWhenForegrounded)
call TaskQueue::SetEnabled and it was possible for the ThrottlingHelper
to re-enable a suspended queue by mistake.

This patch prevents that from happening.

BUG=616052

Committed: https://crrev.com/22ca5b9bce1453364fdb32979af7175ccf234e15
Cr-Commit-Position: refs/heads/master@{#397437}
==========

[commit-bot: I haz the power, 2016-06-02 16:16:52]

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