Device policy refresh fixed when the device is woken up.

Device policy refresh fixed when the device is woken up.

When the device wakes up, check and reschedule, if the policy refresh should happen earlier based on system time.

BUG=665343
TEST=Manually tested on lulu device.

Review-Url: https://codereview.chromium.org/2638923002
Cr-Commit-Position: refs/heads/master@{#453207}
Committed: https://chromium.googlesource.com/chromium/src/+/5d10b590e88508313c9e68d004cabc11bf6f55ce

[igorcov, 2017-01-17 15:52:29]

Description was changed from

==========
Device policy refresh fixed when the device is woken up.

When the device wakes up and the network is established, check if
policy refresh is required.

BUG=665343
TEST=Manually tested on lulu device.
==========

to

==========
Device policy refresh fixed when the device is woken up.

When the device wakes up and the network is established, check if
policy refresh is required.

BUG=665343
TEST=Manually tested on lulu device.
==========

[igorcov, 2017-01-17 15:52:29]

igorcov@chromium.org changed reviewers:
+ atwilson@chromium.org, tnagel@chromium.org

[igorcov, 2017-01-31 08:54:30]

Please take a look.

[Andrew T Wilson (Slow), 2017-01-31 09:29:26]

LGTM as a workaround, but I feel like we need to keep investigating the true
cause because if it's what I suspect (timers don't fire after sleep) there will
be other side-effects so we should fix the root issue.

https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
(right):

https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:182: +
base::TimeDelta::FromMilliseconds(GetActualRefreshDelay())
Are you guaranteed to get an IP address change after every sleep (i.e. what if
your DHCP lease time is long)?

https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:185:
RefreshSoon();
One thing I've never quite understood - is the problem a flaw in this logic in
this class (i.e. there are cases where we fail to setup a refresh timer)? Or is
the problem that after a sleep, pending timers get deleted and don't fire?

[igorcov, 2017-01-31 09:40:07]

https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
(right):

https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:185:
RefreshSoon();
On 2017/01/31 09:29:26, Andrew T Wilson (Slow) wrote:
> One thing I've never quite understood - is the problem a flaw in this logic in
> this class (i.e. there are cases where we fail to setup a refresh timer)? Or
is
> the problem that after a sleep, pending timers get deleted and don't fire?

We setup the times correctly. As far as I've reached with 
investigation, the problem is the function
SetInvalidationServiceAvailability is called but enters in
if (is_available == invalidations_available_) {
when is called for device policy. So, that is another place
where I could put my temporary fix.

As for IPAddress changed, I think if that is not triggered,
the policy is updated well and that is the reason the issue
is not reproduced with smaller timeouts.

[Andrew T Wilson (Slow), 2017-01-31 10:19:06]

On 2017/01/31 09:40:07, igorcov wrote:
>
https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
> File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
> (right):
> 
>
https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
> components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:185:
> RefreshSoon();
> On 2017/01/31 09:29:26, Andrew T Wilson (Slow) wrote:
> > One thing I've never quite understood - is the problem a flaw in this logic
in
> > this class (i.e. there are cases where we fail to setup a refresh timer)? Or
> is
> > the problem that after a sleep, pending timers get deleted and don't fire?
> 
> We setup the times correctly. As far as I've reached with 
> investigation, the problem is the function
> SetInvalidationServiceAvailability is called but enters in
> if (is_available == invalidations_available_) {
> when is called for device policy. So, that is another place
> where I could put my temporary fix.
> 
> As for IPAddress changed, I think if that is not triggered,
> the policy is updated well and that is the reason the issue
> is not reproduced with smaller timeouts.

I guess I don't get it. If is_available == invalidations_available_, all that
means is invalidations are still available and we don't have to change the
refresh timer interval (the old refresh timer is still valid). But the pending
refresh timer should still fire after the device wakes up, right? Is the refresh
timer firing after the device wakes up, if it's timed to fire while the device
is asleep?

[igorcov, 2017-01-31 12:09:38]

On 2017/01/31 10:19:06, Andrew T Wilson (Slow) wrote:
> On 2017/01/31 09:40:07, igorcov wrote:
> >
>
https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
> > File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
> > components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:185:
> > RefreshSoon();
> > On 2017/01/31 09:29:26, Andrew T Wilson (Slow) wrote:
> > > One thing I've never quite understood - is the problem a flaw in this
logic
> in
> > > this class (i.e. there are cases where we fail to setup a refresh timer)?
Or
> > is
> > > the problem that after a sleep, pending timers get deleted and don't fire?
> > 
> > We setup the times correctly. As far as I've reached with 
> > investigation, the problem is the function
> > SetInvalidationServiceAvailability is called but enters in
> > if (is_available == invalidations_available_) {
> > when is called for device policy. So, that is another place
> > where I could put my temporary fix.
> > 
> > As for IPAddress changed, I think if that is not triggered,
> > the policy is updated well and that is the reason the issue
> > is not reproduced with smaller timeouts.
> 
> I guess I don't get it. If is_available == invalidations_available_, all that
> means is invalidations are still available and we don't have to change the
> refresh timer interval (the old refresh timer is still valid). But the pending
> refresh timer should still fire after the device wakes up, right? Is the
refresh
> timer firing after the device wakes up, if it's timed to fire while the device
> is asleep?

The pending refresh timer doesn't fire after the device wakes up, even though it
was timed before felt asleep.

The user policy is updated because it's fired as a result of
SetInvalidationServiceAvailability scheduling a refresh. The same happens for
device policy, but the invalidations_available_ is not changed in that case.

[Andrew T Wilson (Slow), 2017-01-31 13:47:57]

On 2017/01/31 12:09:38, igorcov wrote:
> On 2017/01/31 10:19:06, Andrew T Wilson (Slow) wrote:
> > On 2017/01/31 09:40:07, igorcov wrote:
> > >
> >
>
https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
> > > File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
> > > components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:185:
> > > RefreshSoon();
> > > On 2017/01/31 09:29:26, Andrew T Wilson (Slow) wrote:
> > > > One thing I've never quite understood - is the problem a flaw in this
> logic
> > in
> > > > this class (i.e. there are cases where we fail to setup a refresh
timer)?
> Or
> > > is
> > > > the problem that after a sleep, pending timers get deleted and don't
fire?
> > > 
> > > We setup the times correctly. As far as I've reached with 
> > > investigation, the problem is the function
> > > SetInvalidationServiceAvailability is called but enters in
> > > if (is_available == invalidations_available_) {
> > > when is called for device policy. So, that is another place
> > > where I could put my temporary fix.
> > > 
> > > As for IPAddress changed, I think if that is not triggered,
> > > the policy is updated well and that is the reason the issue
> > > is not reproduced with smaller timeouts.
> > 
> > I guess I don't get it. If is_available == invalidations_available_, all
that
> > means is invalidations are still available and we don't have to change the
> > refresh timer interval (the old refresh timer is still valid). But the
pending
> > refresh timer should still fire after the device wakes up, right? Is the
> refresh
> > timer firing after the device wakes up, if it's timed to fire while the
device
> > is asleep?
> 
> The pending refresh timer doesn't fire after the device wakes up, even though
it
> was timed before felt asleep.

OK, this seems like a core bug then. If we can really repro this reliably, we
should loop in the cros platform team.

[igorcov, 2017-01-31 14:42:13]

On 2017/01/31 13:47:57, Andrew T Wilson (Slow) wrote:
> On 2017/01/31 12:09:38, igorcov wrote:
> > On 2017/01/31 10:19:06, Andrew T Wilson (Slow) wrote:
> > > On 2017/01/31 09:40:07, igorcov wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
> > > > File
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
> > > >
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:185:
> > > > RefreshSoon();
> > > > On 2017/01/31 09:29:26, Andrew T Wilson (Slow) wrote:
> > > > > One thing I've never quite understood - is the problem a flaw in this
> > logic
> > > in
> > > > > this class (i.e. there are cases where we fail to setup a refresh
> timer)?
> > Or
> > > > is
> > > > > the problem that after a sleep, pending timers get deleted and don't
> fire?
> > > > 
> > > > We setup the times correctly. As far as I've reached with 
> > > > investigation, the problem is the function
> > > > SetInvalidationServiceAvailability is called but enters in
> > > > if (is_available == invalidations_available_) {
> > > > when is called for device policy. So, that is another place
> > > > where I could put my temporary fix.
> > > > 
> > > > As for IPAddress changed, I think if that is not triggered,
> > > > the policy is updated well and that is the reason the issue
> > > > is not reproduced with smaller timeouts.
> > > 
> > > I guess I don't get it. If is_available == invalidations_available_, all
> that
> > > means is invalidations are still available and we don't have to change the
> > > refresh timer interval (the old refresh timer is still valid). But the
> pending
> > > refresh timer should still fire after the device wakes up, right? Is the
> > refresh
> > > timer firing after the device wakes up, if it's timed to fire while the
> device
> > > is asleep?
> > 
> > The pending refresh timer doesn't fire after the device wakes up, even
though
> it
> > was timed before felt asleep.
> 
> OK, this seems like a core bug then. If we can really repro this reliably, we
> should loop in the cros platform team.

OK, will try to check what happens in PostDelayedTask then. I didn't see in its
documentation anything related to the case when the device is asleep and assumed
it's not supposed to fire.

Pretty sure it happens all the time, but will check once more and loop in the
owners of that code if that's the case.

[Andrew T Wilson (Slow), 2017-01-31 15:20:28]

On 2017/01/31 14:42:13, igorcov wrote:
> On 2017/01/31 13:47:57, Andrew T Wilson (Slow) wrote:
> > On 2017/01/31 12:09:38, igorcov wrote:
> > > On 2017/01/31 10:19:06, Andrew T Wilson (Slow) wrote:
> > > > On 2017/01/31 09:40:07, igorcov wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
> > > > > File
> components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
> > > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
> > > > >
> components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:185:
> > > > > RefreshSoon();
> > > > > On 2017/01/31 09:29:26, Andrew T Wilson (Slow) wrote:
> > > > > > One thing I've never quite understood - is the problem a flaw in
this
> > > logic
> > > > in
> > > > > > this class (i.e. there are cases where we fail to setup a refresh
> > timer)?
> > > Or
> > > > > is
> > > > > > the problem that after a sleep, pending timers get deleted and don't
> > fire?
> > > > > 
> > > > > We setup the times correctly. As far as I've reached with 
> > > > > investigation, the problem is the function
> > > > > SetInvalidationServiceAvailability is called but enters in
> > > > > if (is_available == invalidations_available_) {
> > > > > when is called for device policy. So, that is another place
> > > > > where I could put my temporary fix.
> > > > > 
> > > > > As for IPAddress changed, I think if that is not triggered,
> > > > > the policy is updated well and that is the reason the issue
> > > > > is not reproduced with smaller timeouts.
> > > > 
> > > > I guess I don't get it. If is_available == invalidations_available_, all
> > that
> > > > means is invalidations are still available and we don't have to change
the
> > > > refresh timer interval (the old refresh timer is still valid). But the
> > pending
> > > > refresh timer should still fire after the device wakes up, right? Is the
> > > refresh
> > > > timer firing after the device wakes up, if it's timed to fire while the
> > device
> > > > is asleep?
> > > 
> > > The pending refresh timer doesn't fire after the device wakes up, even
> though
> > it
> > > was timed before felt asleep.
> > 
> > OK, this seems like a core bug then. If we can really repro this reliably,
we
> > should loop in the cros platform team.
> 
> OK, will try to check what happens in PostDelayedTask then. I didn't see in
its
> documentation anything related to the case when the device is asleep and
assumed
> it's not supposed to fire.
> 
> Pretty sure it happens all the time, but will check once more and loop in the
> owners of that code if that's the case.

If we just eat timers that fire when the device is asleep, then we need to
change our retry code (maybe all the retry code in multiple places - log
uploading, etc) to catch the "device woke up from sleep" event and trigger
refreshes/retries as appropriate. So definitely important to understand the
expected behavior.

[Thiemo Nagel, 2017-02-01 13:14:02]

https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
(right):

https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:181:
base::TimeDelta delta = last_refresh_
Please convert last_refresh_ (and creation_time_) to base::TimeTicks as
base::Time is not guaranteed to increase monotonically.

[igorcov, 2017-02-08 15:30:52]

On 2017/02/01 13:14:02, Thiemo Nagel (slow) wrote:
>
https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
> File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
> (right):
> 
>
https://codereview.chromium.org/2638923002/diff/1/components/policy/core/comm...
> components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:181:
> base::TimeDelta delta = last_refresh_
> Please convert last_refresh_ (and creation_time_) to base::TimeTicks as
> base::Time is not guaranteed to increase monotonically.

Since TimeTicks doesn't count the time spent while sleeping (as discovered
in other thread), the conversion wouldn't solve the problem.

Also this fix wouldn't trigger the policy fetch all the time with good
accuracy, since the device might be woken up before the refresh expired.
But it puts a maximum delay at twice the scheduled interval. I find this
solution good enough for now, until a better functionality in TimeTicks
is available.

[Thiemo Nagel, 2017-02-13 16:20:05]

> Since TimeTicks doesn't count the time spent while sleeping (as discovered
> in other thread), the conversion wouldn't solve the problem.

True.  However using Time leads to missing policy fetches when system time is
moving backwards.  You need to combine TimeTicks and Time to avoid that.
 
It seems to me that you're using the network change notification as a proxy for
wakeup-from-sleep.  Are you sure this is going to catch all wakeups?  Maybe
there's a dedicated wakeup-from-sleep notification that could be used?

[igorcov, 2017-02-15 12:42:43]

Answering in line:

> 
> True.  However using Time leads to missing policy fetches when system time is
> moving backwards.  You need to combine TimeTicks and Time to avoid that.
It is combined. TimeTicks doesn't go anywhere, the task is in the queue and will
run when time according to TimeTicks kicks in. This CL just adds to that another
check, based on system time.

>  
> It seems to me that you're using the network change notification as a proxy
for
> wakeup-from-sleep.  Are you sure this is going to catch all wakeups?  Maybe
> there's a dedicated wakeup-from-sleep notification that could be used?

I don't think we need the wake-up notification. There's no need to fetch the
device policy without network. So, we need the network established notification
and this is exactly that.

This is the notifier that ends up calling OnIpAddressChanged:
https://cs.chromium.org/chromium/src/chromeos/network/network_change_notifier...

It is called when the device is wakes up from suspend state according to
documentation:
https://cs.chromium.org/chromium/src/chromeos/dbus/power_manager_client.h?dr&...

We have the time delta there and I think that could be used in an implementation
of TimeTicks that takes into account the sleep time, but this CL is intended to
solve the problem until we have a implementation in TimeTicks for this.

[Thiemo Nagel, 2017-02-15 17:42:41]

> I don't think we need the wake-up notification. There's no need to fetch the
> device policy without network. So, we need the network established
notification
> and this is exactly that.

Interesting.  From what you've linked, wake-from-suspend always and immediately
triggers a network change notification.  I guess it would make sense to add a
comment about this to OnIPAddressChanged().

> > True.  However using Time leads to missing policy fetches when system time
is
> > moving backwards.  You need to combine TimeTicks and Time to avoid that.
> It is combined. TimeTicks doesn't go anywhere, the task is in the queue and
will
> run when time according to TimeTicks kicks in. This CL just adds to that
another
> check, based on system time.

Still RefreshSoon() might schedule the next refresh at a very long delay when
system time has been going backward.

[igorcov, 2017-02-16 17:11:20]

Good point Thiemo, I've included the last_refresh variable
in TimeTicks to combine them when deciding on delay of the
refresh.

[Thiemo Nagel, 2017-02-17 14:28:23]

Maybe (if you want) it would make sense to create a class (e.g. "SaneTime") that
wraps Time and TimeTicks while still retaining most of the TimeBase semantics
(eg. supporting the - operator).

https://codereview.chromium.org/2638923002/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
(right):

https://codereview.chromium.org/2638923002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:183:
base::TimeDelta delta =
Nit: const

https://codereview.chromium.org/2638923002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:187: if
(client_->status() == DM_STATUS_REQUEST_FAILED || delta.InSeconds() < 0)
Shouldn't you do a calculation based on TimeTicks as well?

https://codereview.chromium.org/2638923002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:227:
last_refresh_ =
I guess you need to set ticks here, too?

https://codereview.chromium.org/2638923002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:237:
last_refresh_ =
I guess you need to set ticks here, too?

https://codereview.chromium.org/2638923002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:326:
base::TimeDelta system_delay =
Nit: const

https://codereview.chromium.org/2638923002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:329:
base::TimeDelta time_ticks_delay =
Nit: const

https://codereview.chromium.org/2638923002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:332:
base::TimeDelta delay = std::min(system_delay, time_ticks_delay);
Nit:const

[igorcov, 2017-02-20 10:33:20]

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

[commit-bot: I haz the power, 2017-02-20 10:33:42]

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

[commit-bot: I haz the power, 2017-02-20 10:44:55]

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

[commit-bot: I haz the power, 2017-02-20 10:44:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[igorcov, 2017-02-20 11:37:31]

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

[commit-bot: I haz the power, 2017-02-20 11:37:56]

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

[igorcov, 2017-02-20 12:26:39]

The CQ bit was unchecked by igorcov@chromium.org

[igorcov, 2017-02-20 13:16:59]

I don't see much value in having a separate SaneTime class.
I think the right thing to do, is to have added functionality
in TimeTicks that is available everywhere. Also, I need to
land it soon, as it needs to be merged back to 57.

https://codereview.chromium.org/2638923002/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
(right):

https://codereview.chromium.org/2638923002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:187: if
(client_->status() == DM_STATUS_REQUEST_FAILED || delta.InSeconds() < 0)
On 2017/02/17 14:28:23, Thiemo Nagel wrote:
> Shouldn't you do a calculation based on TimeTicks as well?

There's a task based on TimeTicks in the queue already. If that time
is up, the message_loop will trigger the callback (PerformRefresh)
soon.

https://codereview.chromium.org/2638923002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:227:
last_refresh_ =
On 2017/02/17 14:28:23, Thiemo Nagel wrote:
> I guess you need to set ticks here, too?

Done.

https://codereview.chromium.org/2638923002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:237:
last_refresh_ =
On 2017/02/17 14:28:23, Thiemo Nagel wrote:
> I guess you need to set ticks here, too?

Done.

[igorcov, 2017-02-20 15:15:45]

Implemented better accuracy according to Drew's idea. Please
take a look.

[Andrew T Wilson (Slow), 2017-02-20 15:39:01]

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
(right):

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:153:
last_refresh_ = base::Time::NowFromSystemTime();
Let's just create an "UpdateLastRefresh()" helper function that updates both of
these.

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:188: if
(last_refresh_.is_null())
Why is it OK to immediately return if there's never been a refresh? Wouldn't we
want to force an update?

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:203:
RefreshAfter(system_delta.InMilliseconds());
I think RefreshAfter() cancels the previous callback, so don't need to call
CancelRefresh()

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:247:
base::TimeTicks::Now() -
Hrm. I think this can lead to a mismatch (for example, if policy->timestamp() is
in the future, then last_refresh_ will point at the future, while
last_refresh_ticks_ will point at Now().

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:261:
base::TimeTicks::Now() -
Ditto here.

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.h
(right):

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.h:131: //
functionality to count sleep time is available in TimeTicks class.
Add a crbug to track this work item.

[Andrew T Wilson (Slow), 2017-02-20 15:48:48]

Also, any way for us to add any test cases for this? Possibly difficult, because
none of the Timer* classes are mockable.

[igorcov, 2017-02-20 16:12:13]

Yes, I tried to look if any unit tests can be added, but it
seems hard provided we use static methods to update the
last_refresh variables.

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
(right):

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:153:
last_refresh_ = base::Time::NowFromSystemTime();
On 2017/02/20 15:39:01, Andrew T Wilson (Slow) wrote:
> Let's just create an "UpdateLastRefresh()" helper function that updates both
of
> these.

Done.

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:188: if
(last_refresh_.is_null())
On 2017/02/20 15:39:01, Andrew T Wilson (Slow) wrote:
> Why is it OK to immediately return if there's never been a refresh? Wouldn't
we
> want to force an update?

If this is not initialized, then the client is not registered.
Which means no refreshes anyway. Unless maybe some weird case happens
when this is called before other initialization.

Anyway, in case we have no information about the last refresh, there's
nothing we can do here. This logic aims to see if the refresh has to
be rescheduled to happen sooner based on system time, and last_refresh
is measured based on system time too.

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:203:
RefreshAfter(system_delta.InMilliseconds());
On 2017/02/20 15:39:01, Andrew T Wilson (Slow) wrote:
> I think RefreshAfter() cancels the previous callback, so don't need to call
> CancelRefresh()

Not directly. It's canceled in PerformRefresh but at that time the
callback is reset already to the new value. Which means if we don't 
reset here, the older that is queued will be executed too when the
timing comes.

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:247:
base::TimeTicks::Now() -
On 2017/02/20 15:39:01, Andrew T Wilson (Slow) wrote:
> Hrm. I think this can lead to a mismatch (for example, if policy->timestamp()
is
> in the future, then last_refresh_ will point at the future, while
> last_refresh_ticks_ will point at Now().

We want a more accurate version here, and if last refresh is in the
future (according to system time), then we want to at least refresh not
later than the refresh period starting from now.

[Andrew T Wilson (Slow), 2017-02-20 16:35:43]

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
(right):

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:188: if
(last_refresh_.is_null())
On 2017/02/20 16:12:12, igorcov wrote:
> On 2017/02/20 15:39:01, Andrew T Wilson (Slow) wrote:
> > Why is it OK to immediately return if there's never been a refresh? Wouldn't
> we
> > want to force an update?
> 
> If this is not initialized, then the client is not registered.
> Which means no refreshes anyway. Unless maybe some weird case happens
> when this is called before other initialization.
> 
> Anyway, in case we have no information about the last refresh, there's
> nothing we can do here. This logic aims to see if the refresh has to
> be rescheduled to happen sooner based on system time, and last_refresh
> is measured based on system time too.

OK, add a comment about last_refresh_ being null, since it's not clear that this
is the "client not registered" case.

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:203:
RefreshAfter(system_delta.InMilliseconds());
On 2017/02/20 16:12:12, igorcov wrote:
> On 2017/02/20 15:39:01, Andrew T Wilson (Slow) wrote:
> > I think RefreshAfter() cancels the previous callback, so don't need to call
> > CancelRefresh()
> 
> Not directly. It's canceled in PerformRefresh but at that time the
> callback is reset already to the new value. Which means if we don't 
> reset here, the older that is queued will be executed too when the
> timing comes.

It's fine to leave as-is, but to be clear, Reset() cancels the existing callback
from what I can tell in the code (see CancelableCallback::Cancel() and
CancelableCallback::Reset()). But I'm fine with leaving the explicit cancel call
in there.

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:247:
base::TimeTicks::Now() -
On 2017/02/20 16:12:12, igorcov wrote:
> On 2017/02/20 15:39:01, Andrew T Wilson (Slow) wrote:
> > Hrm. I think this can lead to a mismatch (for example, if
policy->timestamp()
> is
> > in the future, then last_refresh_ will point at the future, while
> > last_refresh_ticks_ will point at Now().
> 
> We want a more accurate version here, and if last refresh is in the
> future (according to system time), then we want to at least refresh not
> later than the refresh period starting from now.

Here's the problem, it *feels* like we should be trying to get last_refresh_ and
last_refresh_ticks_ to always point to the same value. Here's a case where we
are explicitly letting them point to different values which is weird to me.

If these aren't intended to point to the same instant in time, then you should
go document what exactly they *are* supposed to represent.

[igorcov, 2017-02-21 13:10:24]

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
(right):

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:188: if
(last_refresh_.is_null())
On 2017/02/20 16:35:43, Andrew T Wilson (Slow) wrote:
> On 2017/02/20 16:12:12, igorcov wrote:
> > On 2017/02/20 15:39:01, Andrew T Wilson (Slow) wrote:
> > > Why is it OK to immediately return if there's never been a refresh?
Wouldn't
> > we
> > > want to force an update?
> > 
> > If this is not initialized, then the client is not registered.
> > Which means no refreshes anyway. Unless maybe some weird case happens
> > when this is called before other initialization.
> > 
> > Anyway, in case we have no information about the last refresh, there's
> > nothing we can do here. This logic aims to see if the refresh has to
> > be rescheduled to happen sooner based on system time, and last_refresh
> > is measured based on system time too.
> 
> OK, add a comment about last_refresh_ being null, since it's not clear that
this
> is the "client not registered" case.

Done, also updated the comment here to cover this case.

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:203:
RefreshAfter(system_delta.InMilliseconds());
On 2017/02/20 16:35:43, Andrew T Wilson (Slow) wrote:
> On 2017/02/20 16:12:12, igorcov wrote:
> > On 2017/02/20 15:39:01, Andrew T Wilson (Slow) wrote:
> > > I think RefreshAfter() cancels the previous callback, so don't need to
call
> > > CancelRefresh()
> > 
> > Not directly. It's canceled in PerformRefresh but at that time the
> > callback is reset already to the new value. Which means if we don't 
> > reset here, the older that is queued will be executed too when the
> > timing comes.
> 
> It's fine to leave as-is, but to be clear, Reset() cancels the existing
callback
> from what I can tell in the code (see CancelableCallback::Cancel() and
> CancelableCallback::Reset()). But I'm fine with leaving the explicit cancel
call
> in there.

Right, it's not necessary. I think it's better to remove Reset in this case.

https://codereview.chromium.org/2638923002/diff/140001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:247:
base::TimeTicks::Now() -
On 2017/02/20 16:35:43, Andrew T Wilson (Slow) wrote:
> On 2017/02/20 16:12:12, igorcov wrote:
> > On 2017/02/20 15:39:01, Andrew T Wilson (Slow) wrote:
> > > Hrm. I think this can lead to a mismatch (for example, if
> policy->timestamp()
> > is
> > > in the future, then last_refresh_ will point at the future, while
> > > last_refresh_ticks_ will point at Now().
> > 
> > We want a more accurate version here, and if last refresh is in the
> > future (according to system time), then we want to at least refresh not
> > later than the refresh period starting from now.
> 
> Here's the problem, it *feels* like we should be trying to get last_refresh_
and
> last_refresh_ticks_ to always point to the same value. Here's a case where we
> are explicitly letting them point to different values which is weird to me.
> 
> If these aren't intended to point to the same instant in time, then you should
> go document what exactly they *are* supposed to represent.

I've decided to go with your suggestion, since it's a risk that it's some
intended case for the last refresh to be something in the 
future. Also is makes sense to have both variables act like pointing to the same
time.

[igorcov, 2017-02-22 13:34:57]

Description was changed from

==========
Device policy refresh fixed when the device is woken up.

When the device wakes up and the network is established, check if
policy refresh is required.

BUG=665343
TEST=Manually tested on lulu device.
==========

to

==========
Device policy refresh fixed when the device is woken up.

When the device wakes up, check and reschedule, if the policy refresh should
happen earlier based on system time.

BUG=665343
TEST=Manually tested on lulu device.
==========

[Andrew T Wilson (Slow), 2017-02-22 15:44:03]

lgtm

https://codereview.chromium.org/2638923002/diff/180001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc
(right):

https://codereview.chromium.org/2638923002/diff/180001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.cc:188: //
the next refresh should happend based on system time, and if this provides
happend->happen

[Thiemo Nagel, 2017-02-22 16:08:22]

Lgtm w/ nit

https://codereview.chromium.org/2638923002/diff/180001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_refresh_scheduler.h
(right):

https://codereview.chromium.org/2638923002/diff/180001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_refresh_scheduler.h:134: //
TODO(igorcov): crbug.com/665343. Reduce this to using only
Nit: I think it's unlikely that such functionality will be added to TimeTicks. 
Maybe just drop the TODO?

[igorcov, 2017-02-24 09:01:59]

The CQ bit was checked by igorcov@chromium.org

[igorcov, 2017-02-24 09:01:59]

The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org,
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/2638923002/#ps200001
(title: "Nits")

[commit-bot: I haz the power, 2017-02-24 09:02:05]

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

[commit-bot: I haz the power, 2017-02-24 09:38:35]

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

[commit-bot: I haz the power, 2017-02-24 09:38:36]

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

[igorcov, 2017-02-27 10:10:45]

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

[commit-bot: I haz the power, 2017-02-27 10:12:14]

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

[commit-bot: I haz the power, 2017-02-27 11:26:56]

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

[commit-bot: I haz the power, 2017-02-27 11:27:01]

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

[igorcov, 2017-02-27 13:20:31]

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

[commit-bot: I haz the power, 2017-02-27 13:20:39]

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

[commit-bot: I haz the power, 2017-02-27 14:19:47]

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

[commit-bot: I haz the power, 2017-02-27 14:19:48]

Dry run: This issue passed the CQ dry run.

[igorcov, 2017-02-27 14:20:39]

The CQ bit was checked by igorcov@chromium.org

[igorcov, 2017-02-27 14:20:39]

The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org,
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/2638923002/#ps240001
(title: "Fixed the tests")

[commit-bot: I haz the power, 2017-02-27 14:20:46]

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

[commit-bot: I haz the power, 2017-02-27 14:25:19]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1488205239113480,
"parent_rev": "1ee77687069628ebd0bba3ff791a5ee6c8d17b41", "commit_rev":
"5d10b590e88508313c9e68d004cabc11bf6f55ce"}

[commit-bot: I haz the power, 2017-02-27 14:25:54]

Description was changed from

==========
Device policy refresh fixed when the device is woken up.

When the device wakes up, check and reschedule, if the policy refresh should
happen earlier based on system time.

BUG=665343
TEST=Manually tested on lulu device.
==========

to

==========
Device policy refresh fixed when the device is woken up.

When the device wakes up, check and reschedule, if the policy refresh should
happen earlier based on system time.

BUG=665343
TEST=Manually tested on lulu device.

Review-Url: https://codereview.chromium.org/2638923002
Cr-Commit-Position: refs/heads/master@{#453207}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d10b590e88508313c9e68d004ca...
==========

[commit-bot: I haz the power, 2017-02-27 14:25:56]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/5d10b590e88508313c9e68d004ca...