Use network time for bad clock interstitial.

components/ssl_errors/error_classification.cc

Index: components/ssl_errors/error_classification.cc
diff --git a/components/ssl_errors/error_classification.cc b/components/ssl_errors/error_classification.cc
index 7ab390bc62adc8d5e8918e574f77f7654a99c4bf..3221ddd491422da92ca7a32f482e1efdc22ca6a6 100644
--- a/components/ssl_errors/error_classification.cc
+++ b/components/ssl_errors/error_classification.cc
@@ -16,6 +16,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
+#include "components/network_time/network_time_tracker.h"
 #include "components/ssl_errors/error_info.h"
 #include "components/url_formatter/url_formatter.h"
 #include "net/base/network_change_notifier.h"
@@ -119,6 +120,7 @@ base::LazyInstance<base::Time> g_testing_build_time = LAZY_INSTANCE_INITIALIZER;
 
 void RecordUMAStatistics(bool overridable,
                          const base::Time& current_time,
+                         const network_time::NetworkTimeTracker* network_time,
                          const GURL& request_url,
                          int cert_error,
                          const net::X509Certificate& cert) {
@@ -128,9 +130,10 @@ void RecordUMAStatistics(bool overridable,
                             ssl_errors::ErrorInfo::END_OF_ENUM);
   switch (type) {
     case ssl_errors::ErrorInfo::CERT_DATE_INVALID: {
-      if (IsUserClockInThePast(base::Time::NowFromSystemTime())) {
+      if (IsUserClockAhead(base::Time::NowFromSystemTime(), network_time)) {
         RecordSSLInterstitialCause(overridable, CLOCK_PAST);

[estark, 2016/03/08 23:17:36]

I think we should preserve the meaning of the existing UMA metrics; otherwise
we'll have a period of time where we'll have to be very careful about how we
query the data to make sure we get meaningful results and not mixed old-and-new
data.

To that end, I suggest:
1. Rename CLOCK_PAST to CLOCK_PAST_BY_BUILD_HEURISTIC (or something), same for
CLOCK_FUTURE. I think renaming UMA histogram values is fine as long as the
ordering doesn't change. So you should be able to rename the enum value here in
this file and the corresponding values in SSLErrorCauses in
tools/metrics/histograms.xml.
2. Add new enum values for CLOCK_PAST_BY_NETWORK_TIME and
CLOCK_FUTURE_BY_NETWORK_TIME and record those too. (I guess you might have to
return an enum value or something from IsUserClockAhead() to give you enough
information to know which value to record in UMA.)
3. Maybe in a separate CL, we can add additional UMA metrics to answer questions
like "How often did we fall back to the build time heuristic?" and "How often
did the network time tracker trigger a bad clock interstitial that the build
time heuristic wouldn't have caught?"

[mab, 2016/03/09 23:35:28]

I like the idea of having an enum to describe what we know about the clock, so,
that part is done.

But I'm not sure about the rest of it.  Consider: today, the heuristic always
fires if the system clock is outside the bound (build-2d, build+365d).  Whereas
now, if network time is available, which will be true for many users, the
heuristic *never* fires.

To preserve this meaning exactly, we must either (a) sometimes record two
metrics to UMA when network time is available, or (b) reverse the precedence
such that network time is used only within the bound (build-2d, build+365d),
about which today's heuristic has nothing to say.

(b) is temporary at best; (a) is wasteful but otherwise inoffensive.  But I
first want to ask what is so bad about changing the existing metrics? 
CLOCK_PAST still means the same thing as before; we're simply improving its
accuracy.

[estark, 2016/03/10 20:10:08]

I had (a) in mind, which is wasteful as you said but not horribly so. (And it's
not really wasteful if we get value out of it.) My opinion about not wanting to
change the existing metrics is based on two things:

1.) We get a useful measure of impact by leaving the existing metric as it is
and adding a new metric that includes network time. We'll be able to say "the
build time heuristic caught X bad clocks, and we showed Y bad clock
interstitials using a combination of build time + network time, so network time
caught an additional Y-X bad clocks that we wouldn't have caught otherwise."

2.) It just sounds annoying and error-prone to use the same histogram value to
measure something in a very different way. Someone without much context will
look at the histogram and be very confused as to why there was a giant increase
in bad clocks. At the very least, if we do change the meaning of the existing
value, I think we should update the description in histograms.xml to note that
the heuristic for detecting bad clocks changed in Jan 16.

[mab, 2016/03/11 04:18:41]

For background, the calling sequence is like this: 

IsErrorDueToBadClock → ShowBadClockInterstitial → RecordUMAStatistics.  

|RecordUMAStatistics| is passed enough bits of state to repeat the time
analysis, i.e. rather than simply taking it as a given that the clock is off, it
calls |IsClockInThePast| again.

I'd argue that's poor separation of concerns: the decision about whether to show
the  interstitial has already been made.  |RecordUMAStatistics| should simply
record it.

So why am I bringing this up now?  Because, if we want to record the verdicts of
the build-time heuristic and the network clock separately, we need to account
for the possibility that they might disagree!

For example, network time might say that the system clock is just fine, in which
case we should not call ShowBadClockInterstitial.  But if the build-time
heuristic disagrees, the CLOCK_PAST metric cannot be recorded, because we only
record metrics if we show the interstitial.  

In short, if we give precedence to network time, we can't help but change the
meaning of an existing metric.

To your points, I think (1) seems true but not weighty.  If network time has a
significant impact, I would think we could just eyeball it.  

To (2) I'd say, yes, let's simply document that our ability to detect bad clocks
has improved over time.  I would even argue that in the long term, we would
rather have one metric than two.

Let me know if I'm convincing you.  I did try to implement separation between
the two methods of assessing the accuracy of the clock, but I found having two
bits of clock state was painful to reason about.

[mab, 2016/03/11 21:07:15]

Oh, I should add: if you buy my argument, I think there is no point to
distinguishing between CLOCK_STATE_NETWORK_PAST and CLOCK_STATE_BUILD_PAST, so I
would collapse the enum a bit.

[estark, 2016/03/11 22:00:05]

Oh, drat! I totally forgot that this is called only after the interstitial is
shown. So, yeah, you're right, it would take some pretty weird gymnastics to
preserve the existing metric, so I suppose we should just update the description
in histograms.xml. (And collapse the enum... or maybe even go back to just a
bool? Though I suppose we might want to use the enum to record additional
metrics in a follow-up CL.)

[mab, 2016/03/11 23:12:49]

Updated histograms.xml.  I've kept the enum, because network time adds the OK
value, and I think it's better to have just one function than a function for the
past and a function for the future.

-      } else if (IsUserClockInTheFuture(base::Time::NowFromSystemTime())) {

[mab, 2016/03/09 23:35:28]

Why the use of NowFromSystemTime here, BTW, instead of the current_time
argument?

[estark, 2016/03/10 20:10:08]

Hmm... good question. Probably an oversight? I'd say fix it in a separate CL,
though (or I can do it if you prefer), and have maybe felt@ and palmer@ look at
it to make sure it's not deliberate for some reason.

[mab, 2016/03/11 04:18:41]

Done.

+      } else if (IsUserClockBehind(base::Time::NowFromSystemTime(),
+                                   network_time)) {
         RecordSSLInterstitialCause(overridable, CLOCK_FUTURE);
       } else if (cert.HasExpired() &&
                  (current_time - cert.valid_expiry()).InDays() < 28) {
@@ -181,7 +184,15 @@ void RecordUMAStatistics(bool overridable,
                             net::NetworkChangeNotifier::CONNECTION_LAST);
 }
 
-bool IsUserClockInThePast(const base::Time& time_now) {
+bool IsUserClockBehind(const base::Time& now_system,

[estark, 2016/03/08 23:17:36]

Definition order should match declaration order (it's reversed).

[estark, 2016/03/08 23:17:36]

Could you add a couple unit tests for these functions?

[mab, 2016/03/09 23:35:28]

Done.  I couldn't figure out how to run error_classification_unittest, so I
added it to components_unittest.  Was that the right thing?

[mab, 2016/03/09 23:35:28]

Sort of moot if you mean just these two functions, but if you want I can move
GetClockState's definition down.  It's a clumsier diff, tho.

[estark, 2016/03/10 20:10:08]

Urgh, I guess that means error_classification_unittest was never getting built
or run before now...? But yes I think that's right.

[estark, 2016/03/10 20:10:08]

I just meant these two functions, so this is fine.

+                       const network_time::NetworkTimeTracker* network_time) {
+  base::Time now_network;
+  base::TimeDelta uncertainty;
+  if (network_time->GetNetworkTime(&now_network, &uncertainty)) {
+    return now_system <
+        now_network - uncertainty - base::TimeDelta::FromMinutes(5);
+  }
+
   base::Time build_time;
   if (!g_testing_build_time.Get().is_null()) {
     build_time = g_testing_build_time.Get();
@@ -189,12 +200,20 @@ bool IsUserClockInThePast(const base::Time& time_now) {
     build_time = base::GetBuildTime();
   }
 
-  if (time_now < build_time - base::TimeDelta::FromDays(2))
+  if (now_system < build_time - base::TimeDelta::FromDays(2))
     return true;
   return false;
 }
 
-bool IsUserClockInTheFuture(const base::Time& time_now) {
+bool IsUserClockAhead(const base::Time& now_system,
+                      const network_time::NetworkTimeTracker* network_time) {
+  base::Time now_network;
+  base::TimeDelta uncertainty;
+  if (network_time->GetNetworkTime(&now_network, &uncertainty)) {
+    return now_system >
+        now_network + uncertainty + base::TimeDelta::FromMinutes(5);
+  }
+
   base::Time build_time;
   if (!g_testing_build_time.Get().is_null()) {
     build_time = g_testing_build_time.Get();
@@ -202,7 +221,7 @@ bool IsUserClockInTheFuture(const base::Time& time_now) {
     build_time = base::GetBuildTime();
   }
 
-  if (time_now > build_time + base::TimeDelta::FromDays(365))
+  if (now_system > build_time + base::TimeDelta::FromDays(365))
     return true;
   return false;
 }