Reduce unneeded policy fetches by detecting expired invalidations.

Reduce unneeded policy fetches by detecting expired invalidations.

User policy fetches currently occur whenever a user with policy signs into Chrome. If the policy was changed while the user was offline, the new policy is received at that time. However, the invalidation service will still send a notification that the policy was updated, possibly causing Chrome to fetch the policy again needlessly (depending on the initial policy fetch time vs the invalidation delivery time).

For versioned invalidations, we can solve this problem by comparing the version, which is a timestamp, to the timestamp of the current policy. If the invalidation was created sufficiently long before the policy was fetched, we can ignore it assuming that we already have the update.

For unknown version invalidations, we did some investigation on the server side indicating that most of these are due to users that have never had a policy change, so the invalidation server determines that the policy version is unknown. We will ignore unknown version invalidations if the policy has been fetched within the past 30 seconds.

BUG=355884
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262782

[Steve Condie, 2014-04-04 20:57:27]

CL 3 of 3 for crbug.com/355884

Added Ilya for histogram update

[Ilya Sherman, 2014-04-04 21:48:56]

LGTM with a question/suggestion:

https://codereview.chromium.org/226093005/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/226093005/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:31714: <int value="1" label="Payload"/>
Do the existing labels cover both the expired and the unexpired case, or just
the unexpired case?  It might help to clarify in the label text.

[Steve Condie, 2014-04-04 22:11:57]

https://codereview.chromium.org/226093005/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/226093005/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:31714: <int value="1" label="Payload"/>
On 2014/04/04 21:48:56, Ilya Sherman wrote:
> Do the existing labels cover both the expired and the unexpired case, or just
> the unexpired case?  It might help to clarify in the label text.

Just unexpired; all the labels are mutually exclusive. I'll add some text to
clarify, thanks.

[Joao da Silva, 2014-04-08 12:31:59]

lgtm

@Ilya: this metric used to be sampled as UMA_HISTOGRAM_BOOLEAN and now is a
UMA_HISTOGRAM_ENUMERATION. IIUC, booleans are sampled as 0 or 1 int values so
this should be fine, but I'd ask for an ACK from your side to be sure :-)

https://codereview.chromium.org/226093005/diff/40001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/226093005/diff/40001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:57: static const int
kMaxInvalidationTimeDelta;
I'd suggest having both times in seconds, up to you.

https://codereview.chromium.org/226093005/diff/40001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc (right):

https://codereview.chromium.org/226093005/diff/40001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc:91: base::Time
time);
Pass by const&

[Steve Condie, 2014-04-08 18:09:38]

https://codereview.chromium.org/226093005/diff/40001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/226093005/diff/40001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:57: static const int
kMaxInvalidationTimeDelta;
On 2014/04/08 12:31:59, Joao da Silva wrote:
> I'd suggest having both times in seconds, up to you.

Done.

https://codereview.chromium.org/226093005/diff/40001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc (right):

https://codereview.chromium.org/226093005/diff/40001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc:91: base::Time
time);
On 2014/04/08 12:31:59, Joao da Silva wrote:
> Pass by const&

Done.

[Steve Condie, 2014-04-08 18:14:56]

The CQ bit was checked by stepco@chromium.org

[commit-bot: I haz the power, 2014-04-08 18:15:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/226093005/50001

[commit-bot: I haz the power, 2014-04-08 18:49:48]

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

[commit-bot: I haz the power, 2014-04-08 18:49:48]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Ilya Sherman, 2014-04-08 19:36:10]

On 2014/04/08 12:31:59, Joao da Silva wrote:
> lgtm
> 
> @Ilya: this metric used to be sampled as UMA_HISTOGRAM_BOOLEAN and now is a
> UMA_HISTOGRAM_ENUMERATION. IIUC, booleans are sampled as 0 or 1 int values so
> this should be fine, but I'd ask for an ACK from your side to be sure :-)

Yep, this should be fine.

[Steve Condie, 2014-04-08 20:16:56]

The CQ bit was checked by stepco@chromium.org

[commit-bot: I haz the power, 2014-04-08 20:17:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/226093005/70001

[commit-bot: I haz the power, 2014-04-08 20:48:07]

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

[commit-bot: I haz the power, 2014-04-08 20:48:07]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Steve Condie, 2014-04-09 18:39:06]

The CQ bit was checked by stepco@chromium.org

[commit-bot: I haz the power, 2014-04-09 18:39:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/226093005/70001

[commit-bot: I haz the power, 2014-04-09 20:00:43]

Change committed as 262782