Fix a bug that could occasionaly cause setInterval to stop

Fix a bug that could occasionaly cause setInterval to stop

There was a bad interaction between throttled task queues and
logic in TimerBase::setNextFireTime that could cause setInterval
to stop firing.

If a timer was posted to a throttled task queue after running on
a normal task queue, it was possible for NOW to be in the past.

Logic existed in TimeDomain::ComputeDelayedRunTime to compensate
for this. Usually that worked fine, however the anti-drift logic
in TimerBase for repeating tasks could cause the scheduled run
time (as visible to TimerBase) to fall on the exact same time as
the previous run.  This would cause TimerBase::setNextFireTime
to nop out, which would cause the setInterval to stop firing.

This patch prevents that by changing ThrottledTimeDomain to be based
on RealTimeDomain.  This means Now() is the real time rather than a
periodically updated virtual time.  I.e. it's monotonically increasing.

At the same time I've remove the now useless
TimeDomain::ComputeDelayedRunTime

BUG=625041
Committed: https://crrev.com/d8ef45f5f3b2d1cdfe863d346acda5da03ba6716
Cr-Commit-Position: refs/heads/master@{#406017}

[alex clarke (OOO till 29th), 2016-07-18 12:20:57]

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

[commit-bot: I haz the power, 2016-07-18 12:21:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alex clarke (OOO till 29th), 2016-07-18 12:21:17]

alexclarke@chromium.org changed reviewers:
+ jochen@chromium.org, rmcilroy@chromium.org

[alex clarke (OOO till 29th), 2016-07-18 12:37:50]

Description was changed from

==========
Fix a bug that could occasionaly cause setInterval to stop

There was a bad interaction between throttled task queues and
logic in TimerBase::setNextFireTime that could cause setInterval
to stop firing.

If a timer was posted to a throttled task queue after running on
a normal task queue, it was possible for NOW to be in the past.

Logic existed in TimeDomain::ComputeDelayedRunTime to compensate
for this. Usually that worked fine, however the anti-drift logic
in TimerBase for repeating tasks could cause the scheduled run
time (as visible to TimerBase) to fall on the exact same time as
the previous run.  This would cause TimerBase::setNextFireTime
to nop out, which would cause the setInterval to stop firing.

This patch prevents that in two ways.  Firstly the throttled
time domain now reports the real time to blink.  Secondly the
optimization in TimerBase::setNextFireTime has been removed.

BUG=625041
==========

to

==========
Fix a bug that could occasionaly cause setInterval to stop

There was a bad interaction between throttled task queues and
logic in TimerBase::setNextFireTime that could cause setInterval
to stop firing.

If a timer was posted to a throttled task queue after running on
a normal task queue, it was possible for NOW to be in the past.

Logic existed in TimeDomain::ComputeDelayedRunTime to compensate
for this. Usually that worked fine, however the anti-drift logic
in TimerBase for repeating tasks could cause the scheduled run
time (as visible to TimerBase) to fall on the exact same time as
the previous run.  This would cause TimerBase::setNextFireTime
to nop out, which would cause the setInterval to stop firing.

This patch prevents that in two ways.  Firstly the throttled
time domain now reports the real time to blink.  Secondly the
optimization in TimerBase::setNextFireTime has been removed.

BUG=625041
==========

[rmcilroy, 2016-07-18 13:03:03]

https://codereview.chromium.org/2155143002/diff/1/components/scheduler/render...
File components/scheduler/renderer/throttled_time_domain.cc (right):

https://codereview.chromium.org/2155143002/diff/1/components/scheduler/render...
components/scheduler/renderer/throttled_time_domain.cc:18: base::TimeTicks
ThrottledTimeDomain::BlinkNow() const {
As discussed offline, could we see whether it is possible to use the real time
always for throttled task queues to avoid having to add this new API?

[commit-bot: I haz the power, 2016-07-18 13:30:05]

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

[commit-bot: I haz the power, 2016-07-18 13:30:05]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2016-07-18 14:52:42]

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

[commit-bot: I haz the power, 2016-07-18 14:53:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alex clarke (OOO till 29th), 2016-07-18 14:56:37]

Description was changed from

==========
Fix a bug that could occasionaly cause setInterval to stop

There was a bad interaction between throttled task queues and
logic in TimerBase::setNextFireTime that could cause setInterval
to stop firing.

If a timer was posted to a throttled task queue after running on
a normal task queue, it was possible for NOW to be in the past.

Logic existed in TimeDomain::ComputeDelayedRunTime to compensate
for this. Usually that worked fine, however the anti-drift logic
in TimerBase for repeating tasks could cause the scheduled run
time (as visible to TimerBase) to fall on the exact same time as
the previous run.  This would cause TimerBase::setNextFireTime
to nop out, which would cause the setInterval to stop firing.

This patch prevents that in two ways.  Firstly the throttled
time domain now reports the real time to blink.  Secondly the
optimization in TimerBase::setNextFireTime has been removed.

BUG=625041
==========

to

==========
Fix a bug that could occasionaly cause setInterval to stop

There was a bad interaction between throttled task queues and
logic in TimerBase::setNextFireTime that could cause setInterval
to stop firing.

If a timer was posted to a throttled task queue after running on
a normal task queue, it was possible for NOW to be in the past.

Logic existed in TimeDomain::ComputeDelayedRunTime to compensate
for this. Usually that worked fine, however the anti-drift logic
in TimerBase for repeating tasks could cause the scheduled run
time (as visible to TimerBase) to fall on the exact same time as
the previous run.  This would cause TimerBase::setNextFireTime
to nop out, which would cause the setInterval to stop firing.

This patch prevents that by changing ThrottledTimeDomain to be based
on RealTimeDomain.  This means Now() is the real now rather than a
periodically updated virtual time.  I.e. it's monotonically increasing.

At the same time I've remove the now useless TimeDomain::ComputeDelayedRunTime

BUG=625041
==========

[alex clarke (OOO till 29th), 2016-07-18 14:58:20]

alexclarke@chromium.org changed reviewers:
- jochen@chromium.org

[alex clarke (OOO till 29th), 2016-07-18 14:58:20]

I'll make a Blink change separately, TimerBase::runInternal is calling
timerMonotonicallyIncreasingTime twice which it doesn't need to.

[alex clarke (OOO till 29th), 2016-07-18 14:59:21]

Description was changed from

==========
Fix a bug that could occasionaly cause setInterval to stop

There was a bad interaction between throttled task queues and
logic in TimerBase::setNextFireTime that could cause setInterval
to stop firing.

If a timer was posted to a throttled task queue after running on
a normal task queue, it was possible for NOW to be in the past.

Logic existed in TimeDomain::ComputeDelayedRunTime to compensate
for this. Usually that worked fine, however the anti-drift logic
in TimerBase for repeating tasks could cause the scheduled run
time (as visible to TimerBase) to fall on the exact same time as
the previous run.  This would cause TimerBase::setNextFireTime
to nop out, which would cause the setInterval to stop firing.

This patch prevents that by changing ThrottledTimeDomain to be based
on RealTimeDomain.  This means Now() is the real now rather than a
periodically updated virtual time.  I.e. it's monotonically increasing.

At the same time I've remove the now useless TimeDomain::ComputeDelayedRunTime

BUG=625041
==========

to

==========
Fix a bug that could occasionaly cause setInterval to stop

There was a bad interaction between throttled task queues and
logic in TimerBase::setNextFireTime that could cause setInterval
to stop firing.

If a timer was posted to a throttled task queue after running on
a normal task queue, it was possible for NOW to be in the past.

Logic existed in TimeDomain::ComputeDelayedRunTime to compensate
for this. Usually that worked fine, however the anti-drift logic
in TimerBase for repeating tasks could cause the scheduled run
time (as visible to TimerBase) to fall on the exact same time as
the previous run.  This would cause TimerBase::setNextFireTime
to nop out, which would cause the setInterval to stop firing.

This patch prevents that by changing ThrottledTimeDomain to be based
on RealTimeDomain.  This means Now() is the real time rather than a
periodically updated virtual time.  I.e. it's monotonically increasing.

At the same time I've remove the now useless 
TimeDomain::ComputeDelayedRunTime

BUG=625041
==========

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

https://codereview.chromium.org/2155143002/diff/1/components/scheduler/render...
File components/scheduler/renderer/throttled_time_domain.cc (right):

https://codereview.chromium.org/2155143002/diff/1/components/scheduler/render...
components/scheduler/renderer/throttled_time_domain.cc:18: base::TimeTicks
ThrottledTimeDomain::BlinkNow() const {
On 2016/07/18 13:03:03, rmcilroy wrote:
> As discussed offline, could we see whether it is possible to use the real time
> always for throttled task queues to avoid having to add this new API?

Done.

[rmcilroy, 2016-07-18 15:22:57]

I like this approach better, thanks! I'm not sure I totally understand the
intricacies of time domains, but overall LGTM.

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

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/ba...
components/scheduler/base/task_queue_impl.cc:450: LazyNow
lazy_now(main_thread_only().time_domain->CreateLazyNow());
Just out of interest, wondering why this LazyNow creation was pulled out of
PumpQueueLocked?

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/ba...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/ba...
components/scheduler/base/time_domain.h:67: void ClearExpiredWakeups();
Did this need to be moved? I don't see it being used anywhere it wasn't
previously.

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

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/re...
components/scheduler/renderer/throttled_time_domain.cc:20: base::TimeDelta
delay) {}
Comment on why nothing is done here.

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/re...
File components/scheduler/renderer/throttled_time_domain.h (right):

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/re...
components/scheduler/renderer/throttled_time_domain.h:13: // A time domein for
throttled tasks. behaves like an RealTimeDomain except it
/s/domein/domain

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/re...
components/scheduler/renderer/throttled_time_domain.h:14: // relies the owner
(ThrottlingHelper) to schedule wakeups.
s/relies the/relies on the

[alex clarke (OOO till 29th), 2016-07-18 15:35:53]

Thanks Ross.

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

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/ba...
components/scheduler/base/task_queue_impl.cc:450: LazyNow
lazy_now(main_thread_only().time_domain->CreateLazyNow());
On 2016/07/18 15:22:57, rmcilroy wrote:
> Just out of interest, wondering why this LazyNow creation was pulled out of
> PumpQueueLocked?

There could be many throttled task queues, and
ThrottlingHelper::PumpThrottledTasks pumps each in turn.  Without this change,
each queue would run delayed tasks up to a different point which is undesirable
- in degenerate cases the throttling possibly wouldn't throttle at all.

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/ba...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/ba...
components/scheduler/base/time_domain.h:67: void ClearExpiredWakeups();
On 2016/07/18 15:22:57, rmcilroy wrote:
> Did this need to be moved? I don't see it being used anywhere it wasn't
> previously.

I made it public.

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

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/re...
components/scheduler/renderer/throttled_time_domain.cc:20: base::TimeDelta
delay) {}
On 2016/07/18 15:22:57, rmcilroy wrote:
> Comment on why nothing is done here.

Good point, thanks!

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/re...
File components/scheduler/renderer/throttled_time_domain.h (right):

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/re...
components/scheduler/renderer/throttled_time_domain.h:13: // A time domein for
throttled tasks. behaves like an RealTimeDomain except it
On 2016/07/18 15:22:57, rmcilroy wrote:
> /s/domein/domain

Done.

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/re...
components/scheduler/renderer/throttled_time_domain.h:14: // relies the owner
(ThrottlingHelper) to schedule wakeups.
On 2016/07/18 15:22:57, rmcilroy wrote:
> s/relies the/relies on the

Done.

[alex clarke (OOO till 29th), 2016-07-18 15:36:13]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2016-07-18 15:36:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2155143002/#ps40001
(title: "Couple of comments")

[commit-bot: I haz the power, 2016-07-18 15:36:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rmcilroy, 2016-07-18 16:01:42]

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

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/ba...
components/scheduler/base/task_queue_impl.cc:450: LazyNow
lazy_now(main_thread_only().time_domain->CreateLazyNow());
On 2016/07/18 15:35:52, alex clarke wrote:
> On 2016/07/18 15:22:57, rmcilroy wrote:
> > Just out of interest, wondering why this LazyNow creation was pulled out of
> > PumpQueueLocked?
> 
> There could be many throttled task queues, and
> ThrottlingHelper::PumpThrottledTasks pumps each in turn.  Without this change,
> each queue would run delayed tasks up to a different point which is
undesirable
> - in degenerate cases the throttling possibly wouldn't throttle at all.

I see, makes sense, thanks.

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/ba...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/ba...
components/scheduler/base/time_domain.h:67: void ClearExpiredWakeups();
On 2016/07/18 15:35:52, alex clarke wrote:
> On 2016/07/18 15:22:57, rmcilroy wrote:
> > Did this need to be moved? I don't see it being used anywhere it wasn't
> > previously.
> 
> I made it public.

Right, my point was why did you need to make it Public?

[alex clarke (OOO till 29th), 2016-07-18 16:06:38]

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/ba...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/ba...
components/scheduler/base/time_domain.h:67: void ClearExpiredWakeups();
On 2016/07/18 16:01:42, rmcilroy wrote:
> On 2016/07/18 15:35:52, alex clarke wrote:
> > On 2016/07/18 15:22:57, rmcilroy wrote:
> > > Did this need to be moved? I don't see it being used anywhere it wasn't
> > > previously.
> > 
> > I made it public.
> 
> Right, my point was why did you need to make it Public?

It's used by ThrottlingHelper::PumpThrottledTasks.  Note the VirtualTimeDomain
made it public with "using TimeDomain::ClearExpiredWakeups;"

I suppose we could do something similar for ThrottledTimeDomain instead.

[alex clarke (OOO till 29th), 2016-07-18 16:06:46]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2016-07-18 16:06:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2155143002/#ps60001
(title: "ClearExpiredWakeups tweak.")

[commit-bot: I haz the power, 2016-07-18 16:07:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rmcilroy, 2016-07-18 16:10:44]

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/ba...
File components/scheduler/base/time_domain.h (right):

https://codereview.chromium.org/2155143002/diff/20001/components/scheduler/ba...
components/scheduler/base/time_domain.h:67: void ClearExpiredWakeups();
On 2016/07/18 16:06:38, alex clarke wrote:
> On 2016/07/18 16:01:42, rmcilroy wrote:
> > On 2016/07/18 15:35:52, alex clarke wrote:
> > > On 2016/07/18 15:22:57, rmcilroy wrote:
> > > > Did this need to be moved? I don't see it being used anywhere it wasn't
> > > > previously.
> > > 
> > > I made it public.
> > 
> > Right, my point was why did you need to make it Public?
> 
> It's used by ThrottlingHelper::PumpThrottledTasks.  Note the VirtualTimeDomain
> made it public with "using TimeDomain::ClearExpiredWakeups;"
> 
> I suppose we could do something similar for ThrottledTimeDomain instead. 

Ahh I see. I think it's better now that it's explicitly public rather than being
exposed in a subclass.

[commit-bot: I haz the power, 2016-07-18 16:57:05]

Description was changed from

==========
Fix a bug that could occasionaly cause setInterval to stop

There was a bad interaction between throttled task queues and
logic in TimerBase::setNextFireTime that could cause setInterval
to stop firing.

If a timer was posted to a throttled task queue after running on
a normal task queue, it was possible for NOW to be in the past.

Logic existed in TimeDomain::ComputeDelayedRunTime to compensate
for this. Usually that worked fine, however the anti-drift logic
in TimerBase for repeating tasks could cause the scheduled run
time (as visible to TimerBase) to fall on the exact same time as
the previous run.  This would cause TimerBase::setNextFireTime
to nop out, which would cause the setInterval to stop firing.

This patch prevents that by changing ThrottledTimeDomain to be based
on RealTimeDomain.  This means Now() is the real time rather than a
periodically updated virtual time.  I.e. it's monotonically increasing.

At the same time I've remove the now useless 
TimeDomain::ComputeDelayedRunTime

BUG=625041
==========

to

==========
Fix a bug that could occasionaly cause setInterval to stop

There was a bad interaction between throttled task queues and
logic in TimerBase::setNextFireTime that could cause setInterval
to stop firing.

If a timer was posted to a throttled task queue after running on
a normal task queue, it was possible for NOW to be in the past.

Logic existed in TimeDomain::ComputeDelayedRunTime to compensate
for this. Usually that worked fine, however the anti-drift logic
in TimerBase for repeating tasks could cause the scheduled run
time (as visible to TimerBase) to fall on the exact same time as
the previous run.  This would cause TimerBase::setNextFireTime
to nop out, which would cause the setInterval to stop firing.

This patch prevents that by changing ThrottledTimeDomain to be based
on RealTimeDomain.  This means Now() is the real time rather than a
periodically updated virtual time.  I.e. it's monotonically increasing.

At the same time I've remove the now useless 
TimeDomain::ComputeDelayedRunTime

BUG=625041
==========

[commit-bot: I haz the power, 2016-07-18 16:57:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-18 16:57:16]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-18 16:58:52]

Description was changed from

==========
Fix a bug that could occasionaly cause setInterval to stop

There was a bad interaction between throttled task queues and
logic in TimerBase::setNextFireTime that could cause setInterval
to stop firing.

If a timer was posted to a throttled task queue after running on
a normal task queue, it was possible for NOW to be in the past.

Logic existed in TimeDomain::ComputeDelayedRunTime to compensate
for this. Usually that worked fine, however the anti-drift logic
in TimerBase for repeating tasks could cause the scheduled run
time (as visible to TimerBase) to fall on the exact same time as
the previous run.  This would cause TimerBase::setNextFireTime
to nop out, which would cause the setInterval to stop firing.

This patch prevents that by changing ThrottledTimeDomain to be based
on RealTimeDomain.  This means Now() is the real time rather than a
periodically updated virtual time.  I.e. it's monotonically increasing.

At the same time I've remove the now useless 
TimeDomain::ComputeDelayedRunTime

BUG=625041
==========

to

==========
Fix a bug that could occasionaly cause setInterval to stop

There was a bad interaction between throttled task queues and
logic in TimerBase::setNextFireTime that could cause setInterval
to stop firing.

If a timer was posted to a throttled task queue after running on
a normal task queue, it was possible for NOW to be in the past.

Logic existed in TimeDomain::ComputeDelayedRunTime to compensate
for this. Usually that worked fine, however the anti-drift logic
in TimerBase for repeating tasks could cause the scheduled run
time (as visible to TimerBase) to fall on the exact same time as
the previous run.  This would cause TimerBase::setNextFireTime
to nop out, which would cause the setInterval to stop firing.

This patch prevents that by changing ThrottledTimeDomain to be based
on RealTimeDomain.  This means Now() is the real time rather than a
periodically updated virtual time.  I.e. it's monotonically increasing.

At the same time I've remove the now useless
TimeDomain::ComputeDelayedRunTime

BUG=625041

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

[commit-bot: I haz the power, 2016-07-18 16:58:53]

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