Use network time for bad clock interstitial.

chrome/browser/ssl/ssl_error_handler.cc

Index: chrome/browser/ssl/ssl_error_handler.cc
diff --git a/chrome/browser/ssl/ssl_error_handler.cc b/chrome/browser/ssl/ssl_error_handler.cc
index 4e0a84bfa43deac3a4b5778737ba413f563e50ea..706b59913ddb0b8a46b37ac28ef506d9ce0eb6a2 100644
--- a/chrome/browser/ssl/ssl_error_handler.cc
+++ b/chrome/browser/ssl/ssl_error_handler.cc
@@ -14,6 +14,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/time/clock.h"
 #include "base/time/time.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ssl/bad_clock_blocking_page.h"
 #include "chrome/browser/ssl/ssl_blocking_page.h"
@@ -33,6 +34,10 @@
 #include "chrome/browser/ssl/captive_portal_blocking_page.h"
 #endif
 
+namespace network_time {
+class NetworkTimeTracker;
+}
+
 namespace {
 
 // The delay in milliseconds before displaying the SSL interstitial.
@@ -141,8 +146,16 @@ bool IsErrorDueToBadClock(const base::Time& now, int error) {
       ssl_errors::ErrorInfo::CERT_DATE_INVALID) {
     return false;
   }
-  return ssl_errors::IsUserClockInThePast(now) ||
-         ssl_errors::IsUserClockInTheFuture(now);
+  switch (ssl_errors::GetClockState(now,
+                                    g_browser_process->network_time_tracker(),
+                                    true /* record statistics */)) {

[estark, 2016/03/12 07:54:53]

Sorry, I promise this'll be the last annoying question I ask about this. I'm
wondering if you thought about returning the ClockState from this function
instead of a boolean, and then pass the ClockState around in place of the
NetworkTimeTracker. (So pass the ClockState to the BadClockBlockingPage
constructor, which passes it to the BadClockUI, which passes it to
RecordUMAStatistics and uses it to select the |heading_string|.) Seems like that
would remove the need for the boolean argument, and also save the extra work of
calculating the clock state multiple times, but I might be missing something. Or
is that the cleanup you wanted to do in a follow-up CL later?

[mab, 2016/03/14 21:25:25]

Not at all; I should have done it this way to begin with.

One thing that seems like a difficulty is SSLErrorUI, which calls
RecordUMAStatistics, which necessitates a ClockState argument.

In bad_clock_ui.cc there is this:

// TODO(felt): Separate the clock statistics from the main ssl statistics.

I take it that the idea is to have two versions of |RecordUMAStatistics|, one
called from ssl_error_ui.cc and the other called from bad_clock_ui.cc.

Does that sound right?  I think it would make this CL cleaner if I just went
ahead and did that.

[estark, 2016/03/15 05:50:00]

That sounds reasonable. I think we'd still want to record EXPIRED_RECENTLY on
non-bad-clock CERT_DATE_INVALID errors, though, so we should still pass the
system time into SSLErrorUI to use in RecordUMAStatistics, I suppose?

+    case ssl_errors::CLOCK_STATE_FUTURE:
+      return true;
+    case ssl_errors::CLOCK_STATE_PAST:
+      return true;
+    default:
+      return false;
+  }
 }
 
 }  // namespace