Cap the number of endpoints ReportingEndpointManager tracks.

The class tracks which endpoints uploads have succeeded/failed to.
With the introduction of NetworkIsolationKeys, seems like we should
bound its size, just in case.

This CL uses an MRUCache to do that. Ideally, we'd just evict URLs with
no entries in the ReportingCache, but that's a fair bit more complicaed,
and it's not clear if it's worth doing.

Bug: 993805, 1012931
Change-Id: I169882870e5ec7728c66653cea2134313905508d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1853860
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704925}

net/reporting/reporting_endpoint_manager.cc

@@ -10,6 +10,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/containers/mru_cache.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/rand_util.h"
@@ -40,7 +41,8 @@ class ReportingEndpointManagerImpl : public ReportingEndpointManager {
         tick_clock_(tick_clock),
         delegate_(delegate),
         cache_(cache),
-        rand_callback_(rand_callback) {
+        rand_callback_(rand_callback),
+        endpoint_backoff_(kMaxEndpointBackoffCacheSize) {
     DCHECK(policy);
     DCHECK(tick_clock);
     DCHECK(delegate);
@@ -66,13 +68,6 @@ class ReportingEndpointManagerImpl : public ReportingEndpointManager {
     int total_weight = 0;
 
     for (const ReportingEndpoint endpoint : endpoints) {
-      auto endpoint_backoff_it = endpoint_backoff_.find(
-          EndpointBackoffKey(network_isolation_key, endpoint.info.url));
-      if (endpoint_backoff_it != endpoint_backoff_.end() &&
-          endpoint_backoff_it->second->ShouldRejectRequest()) {
-        continue;
-      }
-
       if (!delegate_->CanUseClient(endpoint.group_key.origin,
                                    endpoint.info.url)) {
         continue;
@@ -84,6 +79,15 @@ class ReportingEndpointManagerImpl : public ReportingEndpointManager {
         continue;
       }
 
+      // This brings each match to the front of the MRU cache, so if an entry
+      // frequently matches requests, it's more likely to stay in the cache.
+      auto endpoint_backoff_it = endpoint_backoff_.Get(
+          EndpointBackoffKey(network_isolation_key, endpoint.info.url));
+      if (endpoint_backoff_it != endpoint_backoff_.end() &&
+          endpoint_backoff_it->second->ShouldRejectRequest()) {
+        continue;
+      }
+
       // If this client is higher priority than the ones we've found (or we
       // haven't found any), forget about those ones and remember this one.
       if (available_endpoints.empty() ||
@@ -124,14 +128,13 @@ class ReportingEndpointManagerImpl : public ReportingEndpointManager {
                                const GURL& endpoint,
                                bool succeeded) override {
     EndpointBackoffKey endpoint_backoff_key(network_isolation_key, endpoint);
-    auto endpoint_backoff_it = endpoint_backoff_.find(endpoint_backoff_key);
+    // This will bring the entry to the front of the cache, if it exists.
+    auto endpoint_backoff_it = endpoint_backoff_.Get(endpoint_backoff_key);
     if (endpoint_backoff_it == endpoint_backoff_.end()) {
-      endpoint_backoff_it =
-          endpoint_backoff_
-              .emplace(std::move(endpoint_backoff_key),
-                       std::make_unique<BackoffEntry>(
-                           &policy_->endpoint_backoff_policy, tick_clock_))
-              .first;
+      endpoint_backoff_it = endpoint_backoff_.Put(
+          std::move(endpoint_backoff_key),
+          std::make_unique<BackoffEntry>(&policy_->endpoint_backoff_policy,
+                                         tick_clock_));
     }
     endpoint_backoff_it->second->InformOfRequest(succeeded);
   }
@@ -151,7 +154,7 @@ class ReportingEndpointManagerImpl : public ReportingEndpointManager {
   // to be cleared as well.
   // TODO(chlily): clear this data when endpoints are deleted to avoid unbounded
   // growth of this map.
-  std::map<EndpointBackoffKey, std::unique_ptr<net::BackoffEntry>>
+  base::MRUCache<EndpointBackoffKey, std::unique_ptr<net::BackoffEntry>>
       endpoint_backoff_;
 
   DISALLOW_COPY_AND_ASSIGN(ReportingEndpointManagerImpl);

net/reporting/reporting_endpoint_manager.h

@@ -35,6 +35,12 @@ struct ReportingPolicy;
 // endpoint from an endpoint group to receive reports for an origin.
 class NET_EXPORT ReportingEndpointManager {
  public:
+  // Maximum size of the backoff cache. This is deliberately much larger than is
+  // likely necessary - the only goal is to prevent it from growing without
+  // bound, while preventing repeatedly trying to upload data to down servrs.
+  // Public for tests.
+  static constexpr int kMaxEndpointBackoffCacheSize = 200;
+
   // The ReportingEndpointManager must not be used after any of the objects
   // passed to its constructor are destroyed.
   static std::unique_ptr<ReportingEndpointManager> Create(

net/reporting/reporting_endpoint_manager_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <string>
 
+#include "base/strings/stringprintf.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "base/time/time.h"
 #include "net/base/backoff_entry.h"
@@ -532,5 +533,63 @@ TEST_F(ReportingEndpointManagerTest, NetworkIsolationKeyWithMultipleEndpoints) {
   EXPECT_EQ(kEndpoint3, endpoint.info.url);
 }
 
+TEST_F(ReportingEndpointManagerTest, CacheEviction) {
+  // Add |kMaxEndpointBackoffCacheSize| endpoints.
+  for (int i = 0; i < ReportingEndpointManager::kMaxEndpointBackoffCacheSize;
+       ++i) {
+    SetEndpoint(GURL(base::StringPrintf("https://endpoint%i/", i)));
+  }
+
+  // Mark each endpoint as bad, one-at-a-time. Use FindEndpointForDelivery() to
+  // pick which one to mark as bad, both to exercise the code walking through
+  // all endpoints, and as a sanity check.
+  std::set<GURL> seen_endpoints;
+  for (int i = 0; i < ReportingEndpointManager::kMaxEndpointBackoffCacheSize;
+       ++i) {
+    ReportingEndpoint endpoint = endpoint_manager_->FindEndpointForDelivery(
+        NetworkIsolationKey(), kOrigin, kGroup);
+    EXPECT_TRUE(endpoint);
+    EXPECT_FALSE(seen_endpoints.count(endpoint.info.url));
+    seen_endpoints.insert(endpoint.info.url);
+    endpoint_manager_->InformOfEndpointRequest(NetworkIsolationKey(),
+                                               endpoint.info.url, false);
+  }
+  // All endpoints should now be marked as bad.
+  EXPECT_FALSE(endpoint_manager_->FindEndpointForDelivery(NetworkIsolationKey(),
+                                                          kOrigin, kGroup));
+
+  // Add another endpoint with a different NetworkIsolationKey;
+  const NetworkIsolationKey kNetworkIsolationKey(kOrigin, kOrigin);
+  SetEndpoint(kEndpoint, ReportingEndpoint::EndpointInfo::kDefaultPriority,
+              ReportingEndpoint::EndpointInfo::kDefaultWeight,
+              kNetworkIsolationKey);
+  // All endpoints associated with the empty NetworkIsolationKey should still be
+  // marked as bad.
+  EXPECT_FALSE(endpoint_manager_->FindEndpointForDelivery(NetworkIsolationKey(),
+                                                          kOrigin, kGroup));
+
+  // Make the endpoint added for the kNetworkIsolationKey as bad.
+  endpoint_manager_->InformOfEndpointRequest(kNetworkIsolationKey, kEndpoint,
+                                             false);
+  // The only endpoint for kNetworkIsolationKey should still be marked as bad.
+  EXPECT_FALSE(endpoint_manager_->FindEndpointForDelivery(kNetworkIsolationKey,
+                                                          kOrigin, kGroup));
+  // One of the endpoints for the empty NetworkIsolationKey should no longer be
+  // marked as bad, due to eviction.
+  ReportingEndpoint endpoint = endpoint_manager_->FindEndpointForDelivery(
+      NetworkIsolationKey(), kOrigin, kGroup);
+  EXPECT_TRUE(endpoint);
+
+  // Reporting a success for the (only) good endpoint for the empty
+  // NetworkIsolationKey should evict the entry for kNetworkIsolationKey, since
+  // the most recent FindEndpointForDelivery() call visited all of the empty
+  // NetworkIsolationKey's cached bad entries.
+  endpoint_manager_->InformOfEndpointRequest(NetworkIsolationKey(),
+                                             endpoint.info.url, true);
+
+  EXPECT_TRUE(endpoint_manager_->FindEndpointForDelivery(kNetworkIsolationKey,
+                                                         kOrigin, kGroup));
+}
+
 }  // namespace
 }  // namespace net