Move the sanity check on build time vs current time to the net test

Move the sanity check on build time vs current time to the net test
suite runner.

BUG=666821
Committed: https://crrev.com/ada22007f4df4381ed1092a96cec8dd836be5cb5
Cr-Commit-Position: refs/heads/master@{#434260}

[eroman, 2016-11-22 02:18:02]

eroman@chromium.org changed reviewers:
+ jbudorick@chromium.org, rsleevi@chromium.org

[eroman, 2016-11-22 02:18:11]

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

[commit-bot: I haz the power, 2016-11-22 02:19:04]

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

[jbudorick, 2016-11-22 02:29:53]

While the code looks fine, I'm a little concerned with how this will behave /
how the bots will react if we trigger it. Specifically, I'm not sure about:
 - the android test runner's --gtest_list_test logic
 - the desktop platform's JSON generation + recipe logic

I'm about to hop offline for the night, but I'd like to check out at least the
first of these locally before you land this if you don't mind.

[commit-bot: I haz the power, 2016-11-22 02:56:54]

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

[commit-bot: I haz the power, 2016-11-22 02:56:54]

Dry run: This issue passed the CQ dry run.

[jbudorick, 2016-11-22 15:59:53]

This is fine as far as --gtest_list_tests goes.

https://codereview.chromium.org/2524613002/diff/1/net/test/run_all_unittests.cc
File net/test/run_all_unittests.cc (right):

https://codereview.chromium.org/2524613002/diff/1/net/test/run_all_unittests....
net/test/run_all_unittests.cc:40: if ((now - build_time) <= kMaxAge)
This only works if now is >70 days ahead of build time; it does not work if now
is >70 days behind build time. While the latter is unlikely to happen in normal
operation, I wouldn't be surprised if that's what's happening on the phones.

[eroman, 2016-11-22 22:49:46]

https://codereview.chromium.org/2524613002/diff/1/net/test/run_all_unittests.cc
File net/test/run_all_unittests.cc (right):

https://codereview.chromium.org/2524613002/diff/1/net/test/run_all_unittests....
net/test/run_all_unittests.cc:40: if ((now - build_time) <= kMaxAge)
On 2016/11/22 15:59:53, jbudorick wrote:
> This only works if now is >70 days ahead of build time; it does not work if
now
> is >70 days behind build time. While the latter is unlikely to happen in
normal
> operation, I wouldn't be surprised if that's what's happening on the phones.

Good point! Fixed.

(Should probably apply something similar to the non-test implementation).

[eroman, 2016-11-22 22:50:33]

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

[commit-bot: I haz the power, 2016-11-22 22:51:18]

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

[Ryan Sleevi, 2016-11-22 22:52:30]

https://codereview.chromium.org/2524613002/diff/20001/net/test/run_all_unitte...
File net/test/run_all_unittests.cc (right):

https://codereview.chromium.org/2524613002/diff/20001/net/test/run_all_unitte...
net/test/run_all_unittests.cc:30: // This (currently) lines up with the policy
used by CTPolicyEnforcer, which
I would suggest rewording this, since it's used more than just CTPolicyEnforcer

// This lines up with various //net security features, like Certificate
Transparency or HPKP,
// in that they require the build time be less than 70 days old. Moreover,
operating on the
// assumption that tests are run against recently compiled builds, this also
serves as
// a sanity check for the system clock, which should be close to the build date.

https://codereview.chromium.org/2524613002/diff/20001/net/test/run_all_unitte...
net/test/run_all_unittests.cc:43: std::cerr
Why std::cerr and not LOG(ERROR)?

[eroman, 2016-11-22 22:56:03]

https://codereview.chromium.org/2524613002/diff/1/net/test/run_all_unittests.cc
File net/test/run_all_unittests.cc (right):

https://codereview.chromium.org/2524613002/diff/1/net/test/run_all_unittests....
net/test/run_all_unittests.cc:40: if ((now - build_time) <= kMaxAge)
On 2016/11/22 22:49:46, eroman (slow) wrote:
> On 2016/11/22 15:59:53, jbudorick wrote:
> > This only works if now is >70 days ahead of build time; it does not work if
> now
> > is >70 days behind build time. While the latter is unlikely to happen in
> normal
> > operation, I wouldn't be surprised if that's what's happening on the phones.
> 
> Good point! Fixed.
> 
> (Should probably apply something similar to the non-test implementation).

Also oddly, the opposite seems to be the case on the bots! Here is the output
from my earlier patch that is getting hit:


../../net/cert/ct_policy_enforcer_unittest.cc:144: Failure
Expected: ((now - build_time).InDays()) < (70), actual: 652 vs 70
IMPORTANT: There is a problem with the system clock and/or the build timestamp.
This will lead to many net_unittests failing (crbug.com/666821)
now: 2018-08-20 18:55:58.734 UTC, build_time: 2016-11-06 05:00:00.000 UTC


This shows that the current time is fine, but it is the build time that is
broken...

[jbudorick, 2016-11-22 22:58:59]

On 2016/11/22 22:56:03, eroman (slow) wrote:
>
https://codereview.chromium.org/2524613002/diff/1/net/test/run_all_unittests.cc
> File net/test/run_all_unittests.cc (right):
> 
>
https://codereview.chromium.org/2524613002/diff/1/net/test/run_all_unittests....
> net/test/run_all_unittests.cc:40: if ((now - build_time) <= kMaxAge)
> On 2016/11/22 22:49:46, eroman (slow) wrote:
> > On 2016/11/22 15:59:53, jbudorick wrote:
> > > This only works if now is >70 days ahead of build time; it does not work
if
> > now
> > > is >70 days behind build time. While the latter is unlikely to happen in
> > normal
> > > operation, I wouldn't be surprised if that's what's happening on the
phones.
> > 
> > Good point! Fixed.
> > 
> > (Should probably apply something similar to the non-test implementation).
> 
> Also oddly, the opposite seems to be the case on the bots! Here is the output
> from my earlier patch that is getting hit:
> 
> 
> ../../net/cert/ct_policy_enforcer_unittest.cc:144: Failure
> Expected: ((now - build_time).InDays()) < (70), actual: 652 vs 70
> IMPORTANT: There is a problem with the system clock and/or the build
timestamp.
> This will lead to many net_unittests failing (crbug.com/666821)
> now: 2018-08-20 18:55:58.734 UTC, build_time: 2016-11-06 05:00:00.000 UTC
> 
> 
> This shows that the current time is fine, but it is the build time that is
> broken...

The device thinking that it's nearly two years in the future is ... surprising.

[eroman, 2016-11-22 22:59:16]

https://codereview.chromium.org/2524613002/diff/1/net/test/run_all_unittests.cc
File net/test/run_all_unittests.cc (right):

https://codereview.chromium.org/2524613002/diff/1/net/test/run_all_unittests....
net/test/run_all_unittests.cc:40: if ((now - build_time) <= kMaxAge)
On 2016/11/22 22:56:03, eroman (slow) wrote:
> On 2016/11/22 22:49:46, eroman (slow) wrote:
> > On 2016/11/22 15:59:53, jbudorick wrote:
> > > This only works if now is >70 days ahead of build time; it does not work
if
> > now
> > > is >70 days behind build time. While the latter is unlikely to happen in
> > normal
> > > operation, I wouldn't be surprised if that's what's happening on the
phones.
> > 
> > Good point! Fixed.
> > 
> > (Should probably apply something similar to the non-test implementation).
> 
> Also oddly, the opposite seems to be the case on the bots! Here is the output
> from my earlier patch that is getting hit:
> 
> 
> ../../net/cert/ct_policy_enforcer_unittest.cc:144: Failure
> Expected: ((now - build_time).InDays()) < (70), actual: 652 vs 70
> IMPORTANT: There is a problem with the system clock and/or the build
timestamp.
> This will lead to many net_unittests failing (crbug.com/666821)
> now: 2018-08-20 18:55:58.734 UTC, build_time: 2016-11-06 05:00:00.000 UTC
> 
> 
> This shows that the current time is fine, but it is the build time that is
> broken...

Actually Ryan pointed out I am an idiot. Current time is not 2018 :)

[eroman, 2016-11-22 23:16:38]

https://codereview.chromium.org/2524613002/diff/20001/net/test/run_all_unitte...
File net/test/run_all_unittests.cc (right):

https://codereview.chromium.org/2524613002/diff/20001/net/test/run_all_unitte...
net/test/run_all_unittests.cc:30: // This (currently) lines up with the policy
used by CTPolicyEnforcer, which
On 2016/11/22 22:52:30, Ryan Sleevi (at IETF til 11-20 wrote:
> I would suggest rewording this, since it's used more than just
CTPolicyEnforcer
> 
> // This lines up with various //net security features, like Certificate
> Transparency or HPKP,
> // in that they require the build time be less than 70 days old. Moreover,
> operating on the
> // assumption that tests are run against recently compiled builds, this also
> serves as
> // a sanity check for the system clock, which should be close to the build
date.

Done.

https://codereview.chromium.org/2524613002/diff/20001/net/test/run_all_unitte...
net/test/run_all_unittests.cc:43: std::cerr
On 2016/11/22 22:52:30, Ryan Sleevi (at IETF til 11-20 wrote:
> Why std::cerr and not LOG(ERROR)?

This code runs before base::Logging has been initialized, so it should not be
using the base logging system.

I explored moving this check to NetTestSuite::Initialize (by which point the log
subsystem will be initialized), but that requires some other changes to
base/test that I didn't want to have to make.

Another choice would be to move the call to VerifyBuildIsTimely to after
NetTestSuite has been instantiated, and then we could safely use LOG(ERROR).

[eroman, 2016-11-22 23:21:37]

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

[commit-bot: I haz the power, 2016-11-22 23:22:24]

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-11-23 00:03:47]

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

[commit-bot: I haz the power, 2016-11-23 00:03:49]

Dry run: This issue passed the CQ dry run.

[eroman, 2016-11-23 20:40:48]

Ping.

Not urgent, but when possible would like to clear this from my queue.

[Ryan Sleevi, 2016-11-23 21:08:19]

Not sure if the ping was for me, but LGTM

[jbudorick, 2016-11-23 21:09:32]

On 2016/11/23 21:08:19, Ryan Sleevi wrote:
> Not sure if the ping was for me, but LGTM

same, also lgtm

[eroman, 2016-11-23 21:12:28]

thanks!

[eroman, 2016-11-23 21:13:12]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2016-11-23 21:14:01]

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-11-23 21:49:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479935592716810,
"parent_rev": "0d6e7eb0a1997e3604fd3c8841444fc85fdcd290", "commit_rev":
"d32cf8ecc4e258288418dc8773abc7dc63bf3290"}

[commit-bot: I haz the power, 2016-11-23 21:50:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-23 21:53:41]

Description was changed from

==========
Move the sanity check on build time vs current time to the net test
suite runner.

BUG=666821
==========

to

==========
Move the sanity check on build time vs current time to the net test
suite runner.

BUG=666821

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

[commit-bot: I haz the power, 2016-11-23 21:53:42]

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