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 <chlily@chromium.org>
Reviewed-by: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#632357}

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) {

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);
 

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,19 +448,18 @@ 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;
     }
   }
 
   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<url::Origin> NetworkErrorLoggingService::GetPolicyOriginsForTesting() {
 }
 
 NetworkErrorLoggingService::NetworkErrorLoggingService()
-    : tick_clock_(base::DefaultTickClock::GetInstance()),
-      reporting_service_(nullptr) {}
+    : clock_(base::DefaultClock::GetInstance()), reporting_service_(nullptr) {}
 
 }  // namespace net

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<url::Origin> GetPolicyOriginsForTesting();
 
  protected:
   NetworkErrorLoggingService();
 
   // Unowned:
-  const base::TickClock* tick_clock_;
+  const base::Clock* clock_;
   ReportingService* reporting_service_;
 
  private:

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}";