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 exis
mab
2016/03/09 23:35:28
I like the idea of having an enum to describe what
estark
2016/03/10 20:10:08
I had (a) in mind, which is wasteful as you said b
mab
2016/03/11 04:18:41
For background, the calling sequence is like this:
mab
2016/03/11 21:07:15
Oh, I should add: if you buy my argument, I think
estark
2016/03/11 22:00:05
Oh, drat! I totally forgot that this is called onl
mab
2016/03/11 23:12:49
Updated histograms.xml. I've kept the enum, becau
|
- } else if (IsUserClockInTheFuture(base::Time::NowFromSystemTime())) { |
mab
2016/03/09 23:35:28
Why the use of NowFromSystemTime here, BTW, instea
estark
2016/03/10 20:10:08
Hmm... good question. Probably an oversight? I'd s
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 (i
estark
2016/03/08 23:17:36
Could you add a couple unit tests for these functi
mab
2016/03/09 23:35:28
Done. I couldn't figure out how to run error_clas
mab
2016/03/09 23:35:28
Sort of moot if you mean just these two functions,
estark
2016/03/10 20:10:08
Urgh, I guess that means error_classification_unit
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; |
} |