From 2ad52fb2a486cc9c99df8cdf9fc8adf09808e7a2 Mon Sep 17 00:00:00 2001 From: Lily Chen Date: Thu, 14 Feb 2019 20:57:20 +0000 Subject: [PATCH] NetworkErrorLoggingService: change policy expiration field to base::Time This CL changes the expires field of a NEL policy from base::TimeTicks to base::Time. This will make it easier to serialize and deserialize policies for persistent storage and, per the comment in //base/time/time.h, a base::Time is more appropriate for tracking expiration times. Bug: None Change-Id: Ib4530a97c178441228f42c46ac5195e04e9ba7df Reviewed-on: https://chromium-review.googlesource.com/c/1461327 Commit-Queue: Lily Chen Reviewed-by: Misha Efimov Cr-Commit-Position: refs/heads/master@{#632357} --- net/log/net_log.cc | 10 +++++- net/log/net_log.h | 8 +++++ .../network_error_logging_service.cc | 32 ++++++++----------- .../network_error_logging_service.h | 13 +++++--- .../network_error_logging_service_unittest.cc | 16 ++++++++-- 5 files changed, 52 insertions(+), 27 deletions(-) diff --git a/net/log/net_log.cc b/net/log/net_log.cc index 95b42e7f8be4a9..44810124f616e4 100644 --- a/net/log/net_log.cc +++ b/net/log/net_log.cc @@ -209,11 +209,19 @@ bool NetLog::HasObserver(ThreadSafeObserver* observer) { // static std::string NetLog::TickCountToString(const base::TimeTicks& time) { - int64_t delta_time = (time - base::TimeTicks()).InMilliseconds(); + int64_t delta_time = time.since_origin().InMilliseconds(); // TODO(https://crbug.com/915391): Use NetLogNumberValue(). return base::NumberToString(delta_time); } +// static +std::string NetLog::TimeToString(const base::Time& time) { + // Convert the base::Time to its (approximate) equivalent in base::TimeTicks. + base::TimeTicks time_ticks = + base::TimeTicks::UnixEpoch() + (time - base::Time::UnixEpoch()); + return TickCountToString(time_ticks); +} + // static const char* NetLog::EventTypeToString(NetLogEventType event) { switch (event) { diff --git a/net/log/net_log.h b/net/log/net_log.h index 64415296a320e6..0dfba993e24eed 100644 --- a/net/log/net_log.h +++ b/net/log/net_log.h @@ -150,8 +150,16 @@ class NET_EXPORT NetLog { // Converts a time to the string format that the NetLog uses to represent // times. Strings are used since integers may overflow. + // The resulting string contains the number of milliseconds since the origin + // or "zero" point of the TimeTicks class, which can vary each time the + // application is restarted. This number is related to an actual time via the + // timeTickOffset recorded in GetNetConstants(). static std::string TickCountToString(const base::TimeTicks& time); + // Same as above but takes a base::Time. Should not be used if precise + // timestamps are desired, but is suitable for e.g. expiration times. + static std::string TimeToString(const base::Time& time); + // Returns a C-String symbolic name for |event_type|. static const char* EventTypeToString(NetLogEventType event_type); diff --git a/net/network_error_logging/network_error_logging_service.cc b/net/network_error_logging/network_error_logging_service.cc index 383426aeef4172..20e14baf4eba0d 100644 --- a/net/network_error_logging/network_error_logging_service.cc +++ b/net/network_error_logging/network_error_logging_service.cc @@ -14,8 +14,8 @@ #include "base/metrics/histogram_macros.h" #include "base/rand_util.h" #include "base/stl_util.h" -#include "base/time/default_tick_clock.h" -#include "base/time/tick_clock.h" +#include "base/time/clock.h" +#include "base/time/default_clock.h" #include "base/time/time.h" #include "base/values.h" #include "net/base/ip_address.h" @@ -197,8 +197,7 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService { OriginPolicy policy; policy.origin = origin; policy.received_ip_address = received_ip_address; - HeaderOutcome outcome = - ParseHeader(value, tick_clock_->NowTicks(), &policy); + HeaderOutcome outcome = ParseHeader(value, clock_->Now(), &policy); RecordHeaderOutcome(outcome); if (outcome != HeaderOutcome::SET && outcome != HeaderOutcome::REMOVED) return; @@ -337,8 +336,8 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService { policy_dict.SetKey("includeSubdomains", base::Value(policy.include_subdomains)); policy_dict.SetKey("reportTo", base::Value(policy.report_to)); - policy_dict.SetKey( - "expires", base::Value(NetLog::TickCountToString(policy.expires))); + policy_dict.SetKey("expires", + base::Value(NetLog::TimeToString(policy.expires))); policy_dict.SetKey("successFraction", base::Value(policy.success_fraction)); policy_dict.SetKey("failureFraction", @@ -366,7 +365,7 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService { // Reporting API endpoint group to which reports should be sent. std::string report_to; - base::TimeTicks expires; + base::Time expires; double success_fraction; double failure_fraction; @@ -397,7 +396,7 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService { WildcardPolicyMap wildcard_policies_; HeaderOutcome ParseHeader(const std::string& json_value, - base::TimeTicks now_ticks, + base::Time now, OriginPolicy* policy_out) const { DCHECK(policy_out); @@ -449,11 +448,10 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService { policy_out->success_fraction = success_fraction; policy_out->failure_fraction = failure_fraction; if (max_age_sec > 0) { - policy_out->expires = - now_ticks + base::TimeDelta::FromSeconds(max_age_sec); + policy_out->expires = now + base::TimeDelta::FromSeconds(max_age_sec); return HeaderOutcome::SET; } else { - policy_out->expires = base::TimeTicks(); + policy_out->expires = base::Time(); return HeaderOutcome::REMOVED; } } @@ -461,7 +459,7 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService { const OriginPolicy* FindPolicyForOrigin(const url::Origin& origin) const { // TODO(juliatuttle): Clean out expired policies sometime/somewhere. auto it = policies_.find(origin); - if (it != policies_.end() && tick_clock_->NowTicks() < it->second.expires) + if (it != policies_.end() && clock_->Now() < it->second.expires) return &it->second; std::string domain = origin.host(); @@ -492,7 +490,7 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService { } for (auto jt = it->second.begin(); jt != it->second.end(); ++jt) { - if (tick_clock_->NowTicks() < (*jt)->expires) + if (clock_->Now() < (*jt)->expires) return *jt; } @@ -635,9 +633,8 @@ void NetworkErrorLoggingService::SetReportingService( reporting_service_ = reporting_service; } -void NetworkErrorLoggingService::SetTickClockForTesting( - const base::TickClock* tick_clock) { - tick_clock_ = tick_clock; +void NetworkErrorLoggingService::SetClockForTesting(const base::Clock* clock) { + clock_ = clock; } base::Value NetworkErrorLoggingService::StatusAsValue() const { @@ -651,7 +648,6 @@ std::set NetworkErrorLoggingService::GetPolicyOriginsForTesting() { } NetworkErrorLoggingService::NetworkErrorLoggingService() - : tick_clock_(base::DefaultTickClock::GetInstance()), - reporting_service_(nullptr) {} + : clock_(base::DefaultClock::GetInstance()), reporting_service_(nullptr) {} } // namespace net diff --git a/net/network_error_logging/network_error_logging_service.h b/net/network_error_logging/network_error_logging_service.h index 165ca37b6faf97..226c5f69232f68 100644 --- a/net/network_error_logging/network_error_logging_service.h +++ b/net/network_error_logging/network_error_logging_service.h @@ -12,7 +12,7 @@ #include "base/feature_list.h" #include "base/macros.h" -#include "base/time/tick_clock.h" +#include "base/time/clock.h" #include "base/time/time.h" #include "net/base/ip_address.h" #include "net/base/net_errors.h" @@ -182,20 +182,23 @@ class NET_EXPORT NetworkErrorLoggingService { // |reporting_service| must outlive the NetworkErrorLoggingService. void SetReportingService(ReportingService* reporting_service); - // Sets a base::TickClock (used to track policy expiration) for tests. - // |tick_clock| must outlive the NetworkErrorLoggingService, and cannot be + // Sets a base::Clock (used to track policy expiration) for tests. + // |clock| must outlive the NetworkErrorLoggingService, and cannot be // nullptr. - void SetTickClockForTesting(const base::TickClock* tick_clock); + void SetClockForTesting(const base::Clock* clock); + // Dumps info about all the currently stored policies, including expired ones. + // Used to display information about NEL policies on the NetLog Reporting tab. virtual base::Value StatusAsValue() const; + // Gets the origins of all currently stored policies, including expired ones. virtual std::set GetPolicyOriginsForTesting(); protected: NetworkErrorLoggingService(); // Unowned: - const base::TickClock* tick_clock_; + const base::Clock* clock_; ReportingService* reporting_service_; private: diff --git a/net/network_error_logging/network_error_logging_service_unittest.cc b/net/network_error_logging/network_error_logging_service_unittest.cc index 0d740a72c028a8..0d57ed82ce04c8 100644 --- a/net/network_error_logging/network_error_logging_service_unittest.cc +++ b/net/network_error_logging/network_error_logging_service_unittest.cc @@ -9,7 +9,7 @@ #include "base/bind.h" #include "base/callback.h" #include "base/macros.h" -#include "base/test/simple_test_tick_clock.h" +#include "base/test/simple_test_clock.h" #include "base/test/values_test_util.h" #include "base/time/time.h" #include "base/values.h" @@ -807,8 +807,18 @@ TEST_F(NetworkErrorLoggingServiceTest, NestedTooDeep) { } TEST_F(NetworkErrorLoggingServiceTest, StatusAsValue) { - base::SimpleTestTickClock clock; - service()->SetTickClockForTesting(&clock); + // The expiration times will be bogus, but we need a reproducible value for + // this test. + base::SimpleTestClock clock; + service()->SetClockForTesting(&clock); + // The clock is initialized to the "zero" or origin point of the Time class. + // This sets the clock's Time to the equivalent of the "zero" or origin point + // of the TimeTicks class, so that the serialized value produced by + // NetLog::TimeToString is consistent across restarts. + base::TimeDelta delta_from_origin = + base::Time::UnixEpoch().since_origin() - + base::TimeTicks::UnixEpoch().since_origin(); + clock.Advance(delta_from_origin); static const std::string kHeaderSuccessFraction1 = "{\"report_to\":\"group\",\"max_age\":86400,\"success_fraction\":1.0}";