Network Error Logging: Evict expired and/or least recently used policies

Previously we accumulated NEL policies forever until they were wiped out
by a browser restart. This CL introduces a maximum policy limit, beyond
which eviction of expired and/or least recently used policies is
triggered. A last_used field is added to the policies in order to find
the stalest policy to evict.

Bug: 930301
Change-Id: Ifebfc64c19a4228e8830f62c52e83bc06790cba5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1474530
Commit-Queue: Lily Chen <chlily@chromium.org>
Reviewed-by: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638683}

net/network_error_logging/network_error_logging_service.cc

@@ -207,17 +207,20 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService {
     OriginPolicy policy;
     policy.origin = origin;
     policy.received_ip_address = received_ip_address;
+    policy.last_used = clock_->Now();
     HeaderOutcome outcome = ParseHeader(value, clock_->Now(), &policy);
     RecordHeaderOutcome(outcome);
     if (outcome != HeaderOutcome::SET && outcome != HeaderOutcome::REMOVED)
       return;
 
+    // If a policy for |origin| already existed, remove the old poliicy.
     auto it = policies_.find(origin);
-    if (it != policies_.end()) {
-      MaybeRemoveWildcardPolicy(origin, &it->second);
-      policies_.erase(it);
-    }
+    if (it != policies_.end())
+      RemovePolicy(it);
 
+    // A policy's |expires| field is set to a null time if the max_age was 0.
+    // Having a max_age of 0 means that the policy should be removed, so return
+    // here instead of continuing on to inserting the policy.
     if (policy.expires.is_null())
       return;
 
@@ -225,6 +228,14 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService {
     auto inserted = policies_.insert(std::make_pair(origin, policy));
     DCHECK(inserted.second);
     MaybeAddWildcardPolicy(origin, &inserted.first->second);
+
+    // Evict policies if the policy limit is exceeded.
+    if (policies_.size() > kMaxPolicies) {
+      RemoveAllExpiredPolicies();
+      while (policies_.size() > kMaxPolicies) {
+        EvictStalestPolicy();
+      }
+    }
   }
 
   void OnRequest(RequestDetails details) override {
@@ -246,6 +257,9 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService {
       return;
     }
 
+    // Mark the policy used.
+    policy->last_used = clock_->Now();
+
     Error type = details.type;
     // It is expected for Reporting uploads to terminate with ERR_ABORTED, since
     // the ReportingUploader cancels them after receiving the response code and
@@ -338,6 +352,10 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService {
           RequestOutcome::kDiscardedNoOriginPolicy);
       return;
     }
+
+    // Mark the policy used.
+    policy->last_used = clock_->Now();
+
     if (IsMismatchingSubdomainReport(*policy, report_origin)) {
       RecordSignedExchangeRequestOutcome(
           RequestOutcome::kDiscardedNonDNSSubdomainReport);
@@ -371,17 +389,14 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService {
 
   void RemoveBrowsingData(const base::RepeatingCallback<bool(const GURL&)>&
                               origin_filter) override {
-    std::vector<url::Origin> origins_to_remove;
-
-    for (auto it = policies_.begin(); it != policies_.end(); ++it) {
-      if (origin_filter.Run(it->first.GetURL()))
-        origins_to_remove.push_back(it->first);
-    }
-
-    for (auto it = origins_to_remove.begin(); it != origins_to_remove.end();
-         ++it) {
-      MaybeRemoveWildcardPolicy(*it, &policies_[*it]);
-      policies_.erase(*it);
+    for (auto it = policies_.begin(); it != policies_.end();) {
+      const url::Origin& origin = it->first;
+      // Remove policies matching the filter.
+      if (origin_filter.Run(origin.GetURL())) {
+        it = RemovePolicy(it);
+      } else {
+        ++it;
+      }
     }
   }
 
@@ -437,6 +452,10 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService {
     double success_fraction;
     double failure_fraction;
     bool include_subdomains;
+
+    // Last time the policy was accessed to create a report, even if no report
+    // ends up being queued. Also updated when the policy is first set.
+    mutable base::Time last_used;
   };
 
   // Map from origin to origin's (owned) policy.
@@ -524,7 +543,6 @@ 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() && clock_->Now() < it->second.expires)
       return &it->second;
@@ -576,14 +594,24 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService {
     DCHECK(inserted.second);
   }
 
-  void MaybeRemoveWildcardPolicy(const url::Origin& origin,
-                                 const OriginPolicy* policy) {
+  // Removes the policy pointed to by |policy_it|. Invalidates |policy_it|.
+  // Returns the iterator to the next element.
+  PolicyMap::iterator RemovePolicy(PolicyMap::iterator policy_it) {
+    DCHECK(policy_it != policies_.end());
+    OriginPolicy* policy = &policy_it->second;
+    MaybeRemoveWildcardPolicy(policy);
+    return policies_.erase(policy_it);
+  }
+
+  void MaybeRemoveWildcardPolicy(const OriginPolicy* policy) {
     DCHECK(policy);
-    DCHECK_EQ(policy, &policies_[origin]);
 
     if (!policy->include_subdomains)
       return;
 
+    const url::Origin& origin = policy->origin;
+    DCHECK_EQ(policy, &policies_[origin]);
+
     auto wildcard_it = wildcard_policies_.find(origin.host());
     DCHECK(wildcard_it != wildcard_policies_.end());
 
@@ -593,6 +621,30 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService {
       wildcard_policies_.erase(wildcard_it);
   }
 
+  void RemoveAllExpiredPolicies() {
+    for (auto it = policies_.begin(); it != policies_.end();) {
+      if (it->second.expires < clock_->Now()) {
+        it = RemovePolicy(it);
+      } else {
+        ++it;
+      }
+    }
+  }
+
+  void EvictStalestPolicy() {
+    PolicyMap::iterator stalest_it = policies_.begin();
+    for (auto it = policies_.begin(); it != policies_.end(); ++it) {
+      if (it->second.last_used < stalest_it->second.last_used)
+        stalest_it = it;
+    }
+
+    // This should only be called if we have hit the max policy limit, so there
+    // should be at least one policy.
+    DCHECK(stalest_it != policies_.end());
+
+    RemovePolicy(stalest_it);
+  }
+
   std::unique_ptr<const base::Value> CreateReportBody(
       const std::string& phase,
       const std::string& type,
@@ -715,6 +767,9 @@ const char NetworkErrorLoggingService::kOuterUrlKey[] = "outer_url";
 const char NetworkErrorLoggingService::kInnerUrlKey[] = "inner_url";
 const char NetworkErrorLoggingService::kCertUrlKey[] = "cert_url";
 
+// See also: max number of Reporting endpoints specified in ReportingPolicy.
+const size_t NetworkErrorLoggingService::kMaxPolicies = 1000u;
+
 // static
 void NetworkErrorLoggingService::
     RecordHeaderDiscardedForNoNetworkErrorLoggingService() {

net/network_error_logging/network_error_logging_service.h

@@ -115,6 +115,9 @@ class NET_EXPORT NetworkErrorLoggingService {
   static const char kInnerUrlKey[];
   static const char kCertUrlKey[];
 
+  // Maximum number of NEL policies to store before evicting.
+  static const size_t kMaxPolicies;
+
   // Histograms.  These are mainly used in test cases to verify that interesting
   // events occurred.
 

net/network_error_logging/network_error_logging_service_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/macros.h"
+#include "base/strings/stringprintf.h"
 #include "base/test/simple_test_clock.h"
 #include "base/test/values_test_util.h"
 #include "base/time/time.h"
@@ -185,6 +186,21 @@ class NetworkErrorLoggingServiceTest : public ::testing::Test {
     return reporting_service_->reports();
   }
 
+  const url::Origin MakeOrigin(size_t index) {
+    GURL url(base::StringPrintf("https://example%zd.com/", index));
+    return url::Origin::Create(url);
+  }
+
+  // Returns whether the NetworkErrorLoggingService has a policy corresponding
+  // to |origin|. Returns true if so, even if the policy is expired.
+  bool HasPolicyForOrigin(const url::Origin& origin) {
+    std::set<url::Origin> all_policy_origins =
+        service_->GetPolicyOriginsForTesting();
+    return all_policy_origins.find(origin) != all_policy_origins.end();
+  }
+
+  size_t PolicyCount() { return service_->GetPolicyOriginsForTesting().size(); }
+
   const GURL kUrl_ = GURL("https://example.com/path");
   const GURL kUrlDifferentPort_ = GURL("https://example.com:4433/path");
   const GURL kUrlSubdomain_ = GURL("https://subdomain.example.com/path");
@@ -585,8 +601,11 @@ TEST_F(NetworkErrorLoggingServiceTest, SuccessPOSTReportQueued) {
 
 TEST_F(NetworkErrorLoggingServiceTest, MaxAge0) {
   service()->OnHeader(kOrigin_, kServerIP_, kHeader_);
+  EXPECT_EQ(1u, PolicyCount());
 
+  // Max_age of 0 removes the policy.
   service()->OnHeader(kOrigin_, kServerIP_, kHeaderMaxAge0_);
+  EXPECT_EQ(0u, PolicyCount());
 
   service()->OnRequest(MakeRequestDetails(kUrl_, ERR_CONNECTION_REFUSED));
 
@@ -775,22 +794,31 @@ TEST_F(NetworkErrorLoggingServiceTest,
 
 TEST_F(NetworkErrorLoggingServiceTest, RemoveAllBrowsingData) {
   service()->OnHeader(kOrigin_, kServerIP_, kHeader_);
+  EXPECT_EQ(1u, PolicyCount());
+  EXPECT_TRUE(HasPolicyForOrigin(kOrigin_));
 
   service()->RemoveAllBrowsingData();
 
   service()->OnRequest(MakeRequestDetails(kUrl_, ERR_CONNECTION_REFUSED));
 
+  EXPECT_EQ(0u, PolicyCount());
+  EXPECT_FALSE(HasPolicyForOrigin(kOrigin_));
   EXPECT_TRUE(reports().empty());
 }
 
 TEST_F(NetworkErrorLoggingServiceTest, RemoveSomeBrowsingData) {
   service()->OnHeader(kOrigin_, kServerIP_, kHeader_);
   service()->OnHeader(kOriginDifferentHost_, kServerIP_, kHeader_);
+  EXPECT_EQ(2u, PolicyCount());
 
+  // Remove policy for kOrigin_ but not kOriginDifferentHost_
   service()->RemoveBrowsingData(
       base::BindRepeating([](const GURL& origin) -> bool {
         return origin.host() == "example.com";
       }));
+  EXPECT_EQ(1u, PolicyCount());
+  EXPECT_TRUE(HasPolicyForOrigin(kOriginDifferentHost_));
+  EXPECT_FALSE(HasPolicyForOrigin(kOrigin_));
 
   service()->OnRequest(MakeRequestDetails(kUrl_, ERR_CONNECTION_REFUSED));
 
@@ -1034,5 +1062,86 @@ TEST_F(NetworkErrorLoggingServiceTest, MismatchingIPAddress_SignedExchange) {
   EXPECT_TRUE(reports().empty());
 }
 
+// When the max number of policies is exceeded, first try to remove expired
+// policies before evicting the least recently used unexpired policy.
+TEST_F(NetworkErrorLoggingServiceTest, EvictAllExpiredPoliciesFirst) {
+  base::SimpleTestClock clock;
+  service()->SetClockForTesting(&clock);
+
+  // Add 100 policies then make them expired.
+  for (size_t i = 0; i < 100; ++i) {
+    service()->OnHeader(MakeOrigin(i), kServerIP_, kHeader_);
+  }
+  EXPECT_EQ(100u, PolicyCount());
+  clock.Advance(base::TimeDelta::FromSeconds(86401));  // max_age is 86400 sec
+  // Expired policies are allowed to linger before hitting the policy limit.
+  EXPECT_EQ(100u, PolicyCount());
+
+  // Reach the max policy limit.
+  for (size_t i = 100; i < NetworkErrorLoggingService::kMaxPolicies; ++i) {
+    service()->OnHeader(MakeOrigin(i), kServerIP_, kHeader_);
+  }
+  EXPECT_EQ(NetworkErrorLoggingService::kMaxPolicies, PolicyCount());
+
+  // Add one more policy to trigger eviction of only the expired policies.
+  service()->OnHeader(kOrigin_, kServerIP_, kHeader_);
+  EXPECT_EQ(NetworkErrorLoggingService::kMaxPolicies - 100 + 1, PolicyCount());
+}
+
+TEST_F(NetworkErrorLoggingServiceTest, EvictLeastRecentlyUsedPolicy) {
+  base::SimpleTestClock clock;
+  service()->SetClockForTesting(&clock);
+
+  // A policy's |last_used| is updated when it is added
+  for (size_t i = 0; i < NetworkErrorLoggingService::kMaxPolicies; ++i) {
+    service()->OnHeader(MakeOrigin(i), kServerIP_, kHeader_);
+    clock.Advance(base::TimeDelta::FromSeconds(1));
+  }
+
+  EXPECT_EQ(PolicyCount(), NetworkErrorLoggingService::kMaxPolicies);
+
+  // Set another policy which triggers eviction. None of the policies have
+  // expired, so the least recently used (i.e. least recently added) policy
+  // should be evicted.
+  service()->OnHeader(kOrigin_, kServerIP_, kHeader_);
+  clock.Advance(base::TimeDelta::FromSeconds(1));
+  EXPECT_EQ(PolicyCount(), NetworkErrorLoggingService::kMaxPolicies);
+
+  EXPECT_FALSE(HasPolicyForOrigin(MakeOrigin(0)));  // evicted
+  std::set<url::Origin> all_policy_origins =
+      service()->GetPolicyOriginsForTesting();
+  for (size_t i = 1; i < NetworkErrorLoggingService::kMaxPolicies; ++i) {
+    // Avoid n calls to HasPolicyForOrigin(), which would be O(n^2).
+    EXPECT_EQ(1u, all_policy_origins.count(MakeOrigin(i)));
+  }
+  EXPECT_TRUE(HasPolicyForOrigin(kOrigin_));
+
+  // Now use the policies in reverse order starting with kOrigin_, then add
+  // another policy to trigger eviction, to check that the stalest policy is
+  // identified correctly.
+  service()->OnRequest(
+      MakeRequestDetails(kOrigin_.GetURL(), ERR_CONNECTION_REFUSED));
+  clock.Advance(base::TimeDelta::FromSeconds(1));
+  for (size_t i = NetworkErrorLoggingService::kMaxPolicies - 1; i >= 1; --i) {
+    service()->OnRequest(
+        MakeRequestDetails(MakeOrigin(i).GetURL(), ERR_CONNECTION_REFUSED));
+    clock.Advance(base::TimeDelta::FromSeconds(1));
+  }
+  service()->OnHeader(kOriginSubdomain_, kServerIP_, kHeader_);
+  EXPECT_EQ(PolicyCount(), NetworkErrorLoggingService::kMaxPolicies);
+
+  EXPECT_FALSE(HasPolicyForOrigin(kOrigin_));  // evicted
+  all_policy_origins = service()->GetPolicyOriginsForTesting();
+  for (size_t i = NetworkErrorLoggingService::kMaxPolicies - 1; i >= 1; --i) {
+    // Avoid n calls to HasPolicyForOrigin(), which would be O(n^2).
+    EXPECT_EQ(1u, all_policy_origins.count(MakeOrigin(i)));
+  }
+  EXPECT_TRUE(HasPolicyForOrigin(kOriginSubdomain_));  // most recently added
+
+  // Note: This test advances the clock by ~2000 seconds, which is below the
+  // specified max_age of 86400 seconds, so none of the policies expire during
+  // this test.
+}
+
 }  // namespace
 }  // namespace net