Make the site engagement service more robust to clock changes and time inconsistencies.

Make the site engagement service more robust to clock changes and time inconsistencies.

This CL adjusts the site engagement service's logic for rebasing last
engagement times when no engagement has been earned for some time.

This code was prone to errors when users changed their clocks, where
last engagement times could be moved into the future or into the past
based on a clock change. This has only recently become an issue because
the rebasing codepath now triggers if no engagement is earned for ~9
hours, as opposed to three weeks.

The code was also brittle if the overall last engagement time (a lossy
pref) became out of sync with any single engagement score's last
engagement time (a lossy content setting). It is theoretically possible
that the content setting would be written to prefs, but the overall last
engagement time would not due to the prefs system buffering the lossy
write and then failing to complete it before browser shutdown.

This CL changes the rebase logic to also sanely handle unexpected times
caused by changing clocks or pref writing race conditions. Time rebasing
now occurs when the last engagement time is detected to be in the future
as well as in the past. Regression tests are added to ensure the correct
behaviour.

BUG=654560
Committed: https://crrev.com/ed64b2a6aaeb060283ca5583abc1583a879b1ce1
Cr-Commit-Position: refs/heads/master@{#426696}

[dominickn, 2016-10-19 03:33:05]

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

[commit-bot: I haz the power, 2016-10-19 03:34:50]

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

[dominickn, 2016-10-19 03:37:28]

Description was changed from

==========
Make the site engagement service more robust to clock changes and time
inconsistencies.

This CL adjusts the site engagement service's logic for rebasing last
engagement times when no engagement has been earned for some time.

This code was prone to errors when users changed their clocks, where
last engagement times could be moved into the future or into the past
based on a clock change. This has only recently become an issue because
the rebasing codepath now triggers if no engagement is earned for ~9
hours, as opposed to three weeks.

The code was also brittle if the overall last engagement time (a lossy
pref) became out of sync with any single engagement score's last
engagement time (a lossy content setting). It is theoretically possible
that the content setting would be written to prefs, but the overall last
engagement time would not due to the prefs system buffering the lossy
write and then failing to complete it before browser shutdown.

This CL changes the rebase logic to also sanely handle unexpected times
caused by changing clocks or pref writing race conditions. Regression
tests are added to ensure the correct behaviour.

BUG=654560
==========

to

==========
Make the site engagement service more robust to clock changes and time
inconsistencies.

This CL adjusts the site engagement service's logic for rebasing last
engagement times when no engagement has been earned for some time.

This code was prone to errors when users changed their clocks, where
last engagement times could be moved into the future or into the past
based on a clock change. This has only recently become an issue because
the rebasing codepath now triggers if no engagement is earned for ~9
hours, as opposed to three weeks.

The code was also brittle if the overall last engagement time (a lossy
pref) became out of sync with any single engagement score's last
engagement time (a lossy content setting). It is theoretically possible
that the content setting would be written to prefs, but the overall last
engagement time would not due to the prefs system buffering the lossy
write and then failing to complete it before browser shutdown.

This CL changes the rebase logic to also sanely handle unexpected times
caused by changing clocks or pref writing race conditions. Regression
tests are added to ensure the correct behaviour.

BUG=654560
==========

[dominickn, 2016-10-19 03:37:29]

dominickn@chromium.org changed reviewers:
+ benwells@chromium.org

[dominickn, 2016-10-19 04:05:12]

PTAL, thanks!

[commit-bot: I haz the power, 2016-10-19 05:39:24]

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

[commit-bot: I haz the power, 2016-10-19 05:39:25]

Dry run: Exceeded global retry quota

[benwells, 2016-10-19 23:54:06]

https://codereview.chromium.org/2427343002/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service_unittest.cc (right):

https://codereview.chromium.org/2427343002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service_unittest.cc:1524:
clock->SetNow(before_stale_period);
What would happen if CleanupEngagementScores(true) was called now? From my
understanding of the code there is still a DCHECK that would fire, is that
right?

[dominickn, 2016-10-20 05:19:53]

Description was changed from

==========
Make the site engagement service more robust to clock changes and time
inconsistencies.

This CL adjusts the site engagement service's logic for rebasing last
engagement times when no engagement has been earned for some time.

This code was prone to errors when users changed their clocks, where
last engagement times could be moved into the future or into the past
based on a clock change. This has only recently become an issue because
the rebasing codepath now triggers if no engagement is earned for ~9
hours, as opposed to three weeks.

The code was also brittle if the overall last engagement time (a lossy
pref) became out of sync with any single engagement score's last
engagement time (a lossy content setting). It is theoretically possible
that the content setting would be written to prefs, but the overall last
engagement time would not due to the prefs system buffering the lossy
write and then failing to complete it before browser shutdown.

This CL changes the rebase logic to also sanely handle unexpected times
caused by changing clocks or pref writing race conditions. Regression
tests are added to ensure the correct behaviour.

BUG=654560
==========

to

==========
Make the site engagement service more robust to clock changes and time
inconsistencies.

This CL adjusts the site engagement service's logic for rebasing last
engagement times when no engagement has been earned for some time.

This code was prone to errors when users changed their clocks, where
last engagement times could be moved into the future or into the past
based on a clock change. This has only recently become an issue because
the rebasing codepath now triggers if no engagement is earned for ~9
hours, as opposed to three weeks.

The code was also brittle if the overall last engagement time (a lossy
pref) became out of sync with any single engagement score's last
engagement time (a lossy content setting). It is theoretically possible
that the content setting would be written to prefs, but the overall last
engagement time would not due to the prefs system buffering the lossy
write and then failing to complete it before browser shutdown.

This CL changes the rebase logic to also sanely handle unexpected times
caused by changing clocks or pref writing race conditions. Time rebasing
now occurs when the last engagement time is detected to be in the future
as well as in the past. Regression tests are added to ensure the correct
behaviour.

BUG=654560
==========

[dominickn, 2016-10-20 05:20:25]

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

[commit-bot: I haz the power, 2016-10-20 05:20:41]

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, 2016-10-20 06:06:00]

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

[commit-bot: I haz the power, 2016-10-20 06:06:01]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-10-20 06:11:00]

PTAL, thanks! The code now allows CleanupEngagementScores to run if
last_engagement_time is in the future, and it should sanely handle that
circumstance. I've modified the test to ensure that a situation with a score in
the future and one in the past works as expected.

[dominickn, 2016-10-20 06:11:05]

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

[commit-bot: I haz the power, 2016-10-20 06:11: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, 2016-10-20 07:06:09]

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

[commit-bot: I haz the power, 2016-10-20 07:06:09]

Dry run: This issue passed the CQ dry run.

[benwells, 2016-10-20 07:14:43]

lgtm

https://codereview.chromium.org/2427343002/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/2427343002/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:307:
DCHECK(!update_last_engagement_time || ((last_engagement_time >= now) ||
Nit: So complicated. Can you remove some brackets, i.e. just make this 

A || B || (C && D), 
instead of
A || ((B) || (C && D))

[dominickn, 2016-10-21 00:02:44]

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

[dominickn, 2016-10-21 00:03:25]

Thanks!

https://codereview.chromium.org/2427343002/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/2427343002/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:307:
DCHECK(!update_last_engagement_time || ((last_engagement_time >= now) ||
On 2016/10/20 07:14:43, benwells (slow) wrote:
> Nit: So complicated. Can you remove some brackets, i.e. just make this 
> 
> A || B || (C && D), 
> instead of
> A || ((B) || (C && D))

Done.

[commit-bot: I haz the power, 2016-10-21 00:03:45]

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, 2016-10-21 02:04:17]

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

[commit-bot: I haz the power, 2016-10-21 02:04:18]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-10-21 02:13:00]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-10-21 02:13:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2427343002/#ps60001
(title: "Address nit")

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

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, 2016-10-21 02:26:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-21 13:26:20]

Description was changed from

==========
Make the site engagement service more robust to clock changes and time
inconsistencies.

This CL adjusts the site engagement service's logic for rebasing last
engagement times when no engagement has been earned for some time.

This code was prone to errors when users changed their clocks, where
last engagement times could be moved into the future or into the past
based on a clock change. This has only recently become an issue because
the rebasing codepath now triggers if no engagement is earned for ~9
hours, as opposed to three weeks.

The code was also brittle if the overall last engagement time (a lossy
pref) became out of sync with any single engagement score's last
engagement time (a lossy content setting). It is theoretically possible
that the content setting would be written to prefs, but the overall last
engagement time would not due to the prefs system buffering the lossy
write and then failing to complete it before browser shutdown.

This CL changes the rebase logic to also sanely handle unexpected times
caused by changing clocks or pref writing race conditions. Time rebasing
now occurs when the last engagement time is detected to be in the future
as well as in the past. Regression tests are added to ensure the correct
behaviour.

BUG=654560
==========

to

==========
Make the site engagement service more robust to clock changes and time
inconsistencies.

This CL adjusts the site engagement service's logic for rebasing last
engagement times when no engagement has been earned for some time.

This code was prone to errors when users changed their clocks, where
last engagement times could be moved into the future or into the past
based on a clock change. This has only recently become an issue because
the rebasing codepath now triggers if no engagement is earned for ~9
hours, as opposed to three weeks.

The code was also brittle if the overall last engagement time (a lossy
pref) became out of sync with any single engagement score's last
engagement time (a lossy content setting). It is theoretically possible
that the content setting would be written to prefs, but the overall last
engagement time would not due to the prefs system buffering the lossy
write and then failing to complete it before browser shutdown.

This CL changes the rebase logic to also sanely handle unexpected times
caused by changing clocks or pref writing race conditions. Time rebasing
now occurs when the last engagement time is detected to be in the future
as well as in the past. Regression tests are added to ensure the correct
behaviour.

BUG=654560

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

[commit-bot: I haz the power, 2016-10-21 13:26:20]

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