Refactor ReportingCache's storage of reports

This converts storage of ReportingReports in the ReportingCache into a
flat_set. Also slightly refactors the interface exposed to the
ReportingDeliveryAgent (in preparation for double-keying it).

Bug: 1035660
Change-Id: I4f85283079ecf2e40e4840aac038078a646ec0bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1978218Reviewed-by: David Benjamin <davidben@chromium.org>
Commit-Queue: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728307}

net/reporting/reporting_cache.h

@@ -79,18 +79,11 @@ class NET_EXPORT ReportingCache {
   // base::Value.
   virtual base::Value GetReportsAsValue() const = 0;
 
-  // Gets all reports in the cache that aren't pending. The returned pointers
-  // are valid as long as either no calls to |RemoveReports| have happened or
-  // the reports' |pending| flag has been set to true using |SetReportsPending|.
-  //
-  // (Clears any existing data in |*reports_out|.)
-  virtual void GetNonpendingReports(
-      std::vector<const ReportingReport*>* reports_out) const = 0;
-
-  // Marks a set of reports as pending. |reports| must not already be marked as
-  // pending.
-  virtual void SetReportsPending(
-      const std::vector<const ReportingReport*>& reports) = 0;
+  // Gets all reports in the cache that aren't pending or doomed (i.e. that are
+  // eligible for delivery), and marks returned reports as pending in
+  // preparation for a delivery attempt. The returned pointers are valid as long
+  // as the reports are still pending.
+  virtual std::vector<const ReportingReport*> GetReportsToDeliver() = 0;
 
   // Unmarks a set of reports as pending. |reports| must be previously marked as
   // pending.

net/reporting/reporting_cache_impl.h

@@ -14,6 +14,8 @@
 #include <utility>
 #include <vector>
 
+#include "base/containers/flat_set.h"
+#include "base/containers/unique_ptr_adapters.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/sequence_checker.h"
@@ -49,10 +51,7 @@ class ReportingCacheImpl : public ReportingCache {
   void GetReports(
       std::vector<const ReportingReport*>* reports_out) const override;
   base::Value GetReportsAsValue() const override;
-  void GetNonpendingReports(
-      std::vector<const ReportingReport*>* reports_out) const override;
-  void SetReportsPending(
-      const std::vector<const ReportingReport*>& reports) override;
+  std::vector<const ReportingReport*> GetReportsToDeliver() override;
   void ClearReportsPending(
       const std::vector<const ReportingReport*>& reports) override;
   void IncrementReportsAttempts(
@@ -130,15 +129,15 @@ class ReportingCacheImpl : public ReportingCache {
     std::set<std::string> endpoint_group_names;
   };
 
+  using ReportSet = base::flat_set<std::unique_ptr<ReportingReport>,
+                                   base::UniquePtrComparator>;
   using OriginClientMap = std::unordered_multimap<std::string, OriginClient>;
   using EndpointGroupMap =
       std::map<ReportingEndpointGroupKey, CachedReportingEndpointGroup>;
   using EndpointMap =
       std::multimap<ReportingEndpointGroupKey, ReportingEndpoint>;
 
-  void RemoveReportInternal(const ReportingReport* report);
-
-  const ReportingReport* FindReportToEvict() const;
+  ReportSet::const_iterator FindReportToEvict() const;
 
   // Sanity-checks the entire data structure of clients, groups, and endpoints,
   // if DCHECK is on. The cached clients should pass this sanity check after
@@ -295,17 +294,8 @@ class ReportingCacheImpl : public ReportingCache {
 
   ReportingContext* context_;
 
-  // Owns all reports, keyed by const raw pointer for easier lookup.
-  std::unordered_map<const ReportingReport*, std::unique_ptr<ReportingReport>>
-      reports_;
-
-  // Reports that have been marked pending (in use elsewhere and should not be
-  // deleted until no longer pending).
-  std::unordered_set<const ReportingReport*> pending_reports_;
-
-  // Reports that have been marked doomed (would have been deleted, but were
-  // pending when the deletion was requested).
-  std::unordered_set<const ReportingReport*> doomed_reports_;
+  // Reports that have not yet been successfully uploaded.
+  ReportSet reports_;
 
   // Map of clients for all configured origins, keyed on domain name (there may
   // be multiple origins per domain name).

net/reporting/reporting_cache_unittest.cc

@@ -246,10 +246,14 @@ TEST_P(ReportingCacheTest, RemovePendingReports) {
   EXPECT_FALSE(cache()->IsReportPendingForTesting(reports[0]));
   EXPECT_FALSE(cache()->IsReportDoomedForTesting(reports[0]));
 
-  cache()->SetReportsPending(reports);
+  EXPECT_EQ(reports, cache()->GetReportsToDeliver());
   EXPECT_TRUE(cache()->IsReportPendingForTesting(reports[0]));
   EXPECT_FALSE(cache()->IsReportDoomedForTesting(reports[0]));
 
+  // After getting reports to deliver, everything in the cache should be
+  // pending, so another call to GetReportsToDeliver should return nothing.
+  EXPECT_EQ(0u, cache()->GetReportsToDeliver().size());
+
   cache()->RemoveReports(reports, ReportingReport::Outcome::UNKNOWN);
   EXPECT_TRUE(cache()->IsReportPendingForTesting(reports[0]));
   EXPECT_TRUE(cache()->IsReportDoomedForTesting(reports[0]));
@@ -280,10 +284,14 @@ TEST_P(ReportingCacheTest, RemoveAllPendingReports) {
   EXPECT_FALSE(cache()->IsReportPendingForTesting(reports[0]));
   EXPECT_FALSE(cache()->IsReportDoomedForTesting(reports[0]));
 
-  cache()->SetReportsPending(reports);
+  EXPECT_EQ(reports, cache()->GetReportsToDeliver());
   EXPECT_TRUE(cache()->IsReportPendingForTesting(reports[0]));
   EXPECT_FALSE(cache()->IsReportDoomedForTesting(reports[0]));
 
+  // After getting reports to deliver, everything in the cache should be
+  // pending, so another call to GetReportsToDeliver should return nothing.
+  EXPECT_EQ(0u, cache()->GetReportsToDeliver().size());
+
   cache()->RemoveAllReports(ReportingReport::Outcome::UNKNOWN);
   EXPECT_TRUE(cache()->IsReportPendingForTesting(reports[0]));
   EXPECT_TRUE(cache()->IsReportDoomedForTesting(reports[0]));
@@ -313,19 +321,50 @@ TEST_P(ReportingCacheTest, GetReportsAsValue) {
       AddAndReturnReport(kUrl1_, kUserAgent_, kGroup2_, kType_,
                          std::make_unique<base::DictionaryValue>(), 0,
                          now + base::TimeDelta::FromSeconds(100), 1);
-  cache()->AddReport(kUrl2_, kUserAgent_, kGroup1_, kType_,
-                     std::make_unique<base::DictionaryValue>(), 2,
-                     now + base::TimeDelta::FromSeconds(200), 0);
-  cache()->AddReport(kUrl1_, kUserAgent_, kGroup1_, kType_,
-                     std::make_unique<base::DictionaryValue>(), 0,
-                     now + base::TimeDelta::FromSeconds(300), 0);
-  // Mark report1 as pending as report2 as doomed
-  cache()->SetReportsPending({report1, report2});
+  // Mark report1 and report2 as pending.
+  EXPECT_THAT(cache()->GetReportsToDeliver(),
+              ::testing::UnorderedElementsAre(report1, report2));
+  // Mark report2 as doomed.
   cache()->RemoveReports({report2}, ReportingReport::Outcome::UNKNOWN);
 
   base::Value actual = cache()->GetReportsAsValue();
-  std::unique_ptr<base::Value> expected =
-      base::test::ParseJsonDeprecated(R"json(
+  base::Value expected = base::test::ParseJson(R"json(
+      [
+        {
+          "url": "https://origin1/path",
+          "group": "group2",
+          "type": "default",
+          "status": "doomed",
+          "body": {},
+          "attempts": 1,
+          "depth": 0,
+          "queued": "100000",
+        },
+        {
+          "url": "https://origin1/path",
+          "group": "group1",
+          "type": "default",
+          "status": "pending",
+          "body": {},
+          "attempts": 0,
+          "depth": 0,
+          "queued": "200000",
+        },
+      ]
+      )json");
+  EXPECT_EQ(expected, actual);
+
+  // Add two new reports that will show up as "queued".
+  const ReportingReport* report3 =
+      AddAndReturnReport(kUrl2_, kUserAgent_, kGroup1_, kType_,
+                         std::make_unique<base::DictionaryValue>(), 2,
+                         now + base::TimeDelta::FromSeconds(200), 0);
+  const ReportingReport* report4 =
+      AddAndReturnReport(kUrl1_, kUserAgent_, kGroup1_, kType_,
+                         std::make_unique<base::DictionaryValue>(), 0,
+                         now + base::TimeDelta::FromSeconds(300), 0);
+  actual = cache()->GetReportsAsValue();
+  expected = base::test::ParseJson(R"json(
       [
         {
           "url": "https://origin1/path",
@@ -369,7 +408,11 @@ TEST_P(ReportingCacheTest, GetReportsAsValue) {
         },
       ]
       )json");
-  EXPECT_EQ(*expected, actual);
+  EXPECT_EQ(expected, actual);
+
+  // GetReportsToDeliver only returns the non-pending reports.
+  EXPECT_THAT(cache()->GetReportsToDeliver(),
+              ::testing::UnorderedElementsAre(report3, report4));
 }
 
 TEST_P(ReportingCacheTest, Endpoints) {
@@ -961,18 +1004,19 @@ TEST_P(ReportingCacheTest, DontEvictPendingReports) {
   ASSERT_GT(std::numeric_limits<size_t>::max(), max_report_count);
 
   // Enqueue the maximum number of reports, spaced apart in time.
+  std::vector<const ReportingReport*> reports;
   for (size_t i = 0; i < max_report_count; ++i) {
-    cache()->AddReport(kUrl1_, kUserAgent_, kGroup1_, kType_,
-                       std::make_unique<base::DictionaryValue>(), 0,
-                       tick_clock()->NowTicks(), 0);
+    reports.push_back(
+        AddAndReturnReport(kUrl1_, kUserAgent_, kGroup1_, kType_,
+                           std::make_unique<base::DictionaryValue>(), 0,
+                           tick_clock()->NowTicks(), 0));
     tick_clock()->Advance(base::TimeDelta::FromMinutes(1));
   }
   EXPECT_EQ(max_report_count, report_count());
 
   // Mark all of the queued reports pending.
-  std::vector<const ReportingReport*> queued_reports;
-  cache()->GetReports(&queued_reports);
-  cache()->SetReportsPending(queued_reports);
+  EXPECT_THAT(cache()->GetReportsToDeliver(),
+              ::testing::UnorderedElementsAreArray(reports));
 
   // Add one more report to force the cache to evict one. Since the cache has
   // only pending reports, it will be forced to evict the *new* report!
@@ -982,11 +1026,15 @@ TEST_P(ReportingCacheTest, DontEvictPendingReports) {
 
   // Make sure the cache evicted a report, and make sure the report evicted was
   // the new, non-pending one.
-  std::vector<const ReportingReport*> reports;
-  cache()->GetReports(&reports);
-  EXPECT_EQ(max_report_count, reports.size());
-  for (const ReportingReport* report : reports)
+  std::vector<const ReportingReport*> reports_after_eviction;
+  cache()->GetReports(&reports_after_eviction);
+  EXPECT_EQ(max_report_count, reports_after_eviction.size());
+  for (const ReportingReport* report : reports_after_eviction) {
     EXPECT_TRUE(cache()->IsReportPendingForTesting(report));
+  }
+
+  EXPECT_THAT(reports_after_eviction,
+              ::testing::UnorderedElementsAreArray(reports));
 }
 
 TEST_P(ReportingCacheTest, EvictEndpointsOverPerOriginLimit) {

net/reporting/reporting_delivery_agent.cc

@@ -136,13 +136,8 @@ class ReportingDeliveryAgentImpl : public ReportingDeliveryAgent,
   }
 
   void SendReports() {
-    std::vector<const ReportingReport*> reports;
-    cache()->GetNonpendingReports(&reports);
-
-    // Mark all of these reports as pending, so that they're not deleted out
-    // from under us while we're checking permissions (possibly on another
-    // thread).
-    cache()->SetReportsPending(reports);
+    std::vector<const ReportingReport*> reports =
+        cache()->GetReportsToDeliver();
 
     // First determine which origins we're allowed to upload reports about.
     std::set<url::Origin> report_origins;

net/reporting/reporting_delivery_agent_unittest.cc

@@ -378,9 +378,6 @@ TEST_F(ReportingDeliveryAgentTest, ConcurrentRemove) {
   pending_uploads()[0]->Complete(ReportingUploader::Outcome::SUCCESS);
   cache()->GetReports(&reports);
   EXPECT_TRUE(reports.empty());
-  // This is slightly sketchy since |report| has been freed, but it nonetheless
-  // should not be in the set of doomed reports.
-  EXPECT_FALSE(cache()->IsReportDoomedForTesting(report));
 }
 
 TEST_F(ReportingDeliveryAgentTest, ConcurrentRemoveDuringPermissionsCheck) {
@@ -417,9 +414,6 @@ TEST_F(ReportingDeliveryAgentTest, ConcurrentRemoveDuringPermissionsCheck) {
   pending_uploads()[0]->Complete(ReportingUploader::Outcome::SUCCESS);
   cache()->GetReports(&reports);
   EXPECT_TRUE(reports.empty());
-  // This is slightly sketchy since |report| has been freed, but it nonetheless
-  // should not be in the set of doomed reports.
-  EXPECT_FALSE(cache()->IsReportDoomedForTesting(report));
 }
 
 // Test that the agent will combine reports destined for the same endpoint, even

net/reporting/reporting_endpoint_manager_unittest.cc

@@ -67,13 +67,9 @@ class TestReportingCache : public ReportingCache {
     NOTREACHED();
     return base::Value();
   }
-  void GetNonpendingReports(
-      std::vector<const ReportingReport*>* reports_out) const override {
-    NOTREACHED();
-  }
-  void SetReportsPending(
-      const std::vector<const ReportingReport*>& reports) override {
+  std::vector<const ReportingReport*> GetReportsToDeliver() override {
     NOTREACHED();
+    return {};
   }
   void ClearReportsPending(
       const std::vector<const ReportingReport*>& reports) override {

net/reporting/reporting_report.cc

@@ -39,12 +39,17 @@ ReportingReport::ReportingReport(const GURL& url,
       body(std::move(body)),
       depth(depth),
       queued(queued),
-      attempts(attempts),
-      outcome(Outcome::UNKNOWN),
-      recorded_outcome(false) {}
+      attempts(attempts) {}
 
 ReportingReport::~ReportingReport() {
-  DCHECK(recorded_outcome);
+  if (outcome == Outcome::DELIVERED) {
+    // |delivered| should always have a value here, since the ReportingCache
+    // records the delivery time for any successful delivery.
+    UMA_HISTOGRAM_LONG_TIMES_100("Net.Reporting.ReportDeliveredLatency",
+                                 delivered.value() - queued);
+    UMA_HISTOGRAM_COUNTS_100("Net.Reporting.ReportDeliveredAttempts", attempts);
+  }
+  RecordReportOutcome(outcome);
 }
 
 // static
@@ -57,18 +62,8 @@ void ReportingReport::RecordReportDiscardedForNoReportingService() {
   RecordReportOutcome(Outcome::DISCARDED_NO_REPORTING_SERVICE);
 }
 
-void ReportingReport::RecordOutcome(base::TimeTicks now) {
-  DCHECK(!recorded_outcome);
-
-  RecordReportOutcome(outcome);
-
-  if (outcome == Outcome::DELIVERED) {
-    UMA_HISTOGRAM_LONG_TIMES_100("Net.Reporting.ReportDeliveredLatency",
-                                 now - queued);
-    UMA_HISTOGRAM_COUNTS_100("Net.Reporting.ReportDeliveredAttempts", attempts);
-  }
-
-  recorded_outcome = true;
+bool ReportingReport::IsUploadPending() const {
+  return status == Status::PENDING || status == Status::DOOMED;
 }
 
 }  // namespace net

net/reporting/reporting_report.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <string>
 
+#include "base/optional.h"
 #include "base/time/time.h"
 #include "net/base/net_export.h"
 #include "url/gurl.h"
@@ -37,6 +38,19 @@ struct NET_EXPORT ReportingReport {
     MAX
   };
 
+  enum class Status {
+    // Report has been queued but no attempt has been made to deliver it yet.
+    QUEUED,
+
+    // There is an ongoing attempt to upload this report.
+    PENDING,
+
+    // Deletion of this report was requested while it was pending, so it should
+    // be removed after the attempted upload completes.
+    DOOMED,
+  };
+
+  // TODO(chlily): Remove |attempts| argument as it is (almost?) always 0.
   ReportingReport(const GURL& url,
                   const std::string& user_agent,
                   const std::string& group,
@@ -45,12 +59,15 @@ struct NET_EXPORT ReportingReport {
                   int depth,
                   base::TimeTicks queued,
                   int attempts);
+
+  // Records metrics about report outcome.
   ~ReportingReport();
 
   static void RecordReportDiscardedForNoURLRequestContext();
   static void RecordReportDiscardedForNoReportingService();
 
-  void RecordOutcome(base::TimeTicks now);
+  // Whether the report is part of an ongoing delivery attempt.
+  bool IsUploadPending() const;
 
   // The URL of the document that triggered the report. (Included in the
   // delivered report.)
@@ -78,14 +95,17 @@ struct NET_EXPORT ReportingReport {
   // relative to the time of the delivery attempt.)
   base::TimeTicks queued;
 
+  // Time when report was delivered, if it was delivered successfully.
+  // The destructor assumes that this has a value if the outcome is DELIVERED.
+  base::Optional<base::TimeTicks> delivered = base::nullopt;
+
   // The number of delivery attempts made so far, not including an active
   // attempt. (Not included in the delivered report.)
   int attempts = 0;
 
-  Outcome outcome;
+  Outcome outcome = Outcome::UNKNOWN;
 
- private:
-  bool recorded_outcome;
+  Status status = Status::QUEUED;
 
   DISALLOW_COPY_AND_ASSIGN(ReportingReport);
 };