DomainRelability + NetworkIsolationKeys CL 2

Wire NetworkIsolationKey through DomainReliabilityContext.

This is the interesting CL of the bunch. To keep things simple, it keeps
all pending beacons in a single list with a maximum size, even with
different NetworkIsolationKeys. To upload a report, it selects all
beacon with the same Key as the first (oldest) beacon and creates a
report using them. On successful upload, it will remove just those
beacons, and try to upload again (with backoff), if any beacons are
left.

This CL also adds some eviction tests, and removes some incorrect
eviction DCHECKs, which assumed that all beacons currently being
uploaded could not be evicted. That may be unlikely, but there's
nothing from preventing it from actually happening, and it may
be more likely once only beacons with a matching NIK are uploaded
each time.

Bug: 1138994
Change-Id: I37d97b679cc0309c5e128c239585555ab097369b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587669
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837146}

components/domain_reliability/beacon.h

@@ -11,6 +11,7 @@
 #include "base/time/time.h"
 #include "components/domain_reliability/domain_reliability_export.h"
 #include "net/base/net_error_details.h"
+#include "net/base/network_isolation_key.h"
 #include "url/gurl.h"
 
 namespace base {
@@ -44,6 +45,10 @@ struct DOMAIN_RELIABILITY_EXPORT DomainReliabilityBeacon {
   // The URL that the beacon is reporting on, if included.
   // The scheme can be non-secure.
   GURL url;
+  // The NetworkIsolationKey associated with the request being reported on. Must
+  // also be used to upload any report. This field does not appear in the
+  // uploaded report.
+  net::NetworkIsolationKey network_isolation_key;
   // The resource name that the beacon is reporting on, if included.
   std::string resource;
   // Status string (e.g. "ok", "dns.nxdomain", "http.403").

components/domain_reliability/context.cc

@@ -5,6 +5,7 @@
 #include "components/domain_reliability/context.h"
 
 #include <algorithm>
+#include <limits>
 #include <utility>
 
 #include "base/bind.h"
@@ -131,7 +132,19 @@ void DomainReliabilityContext::StartUpload() {
   if (beacons_.empty())
     return;
 
-  MarkUpload();
+  // Find the first beacon with an `upload_depth` of at most
+  // kMaxUploadDepthToSchedule, in preparation to create a report containing all
+  // beacons with matching NetworkIsolationKeys.
+  bool found_beacon_to_upload = false;
+  for (const auto& beacon : beacons_) {
+    if (beacon->upload_depth <= kMaxUploadDepthToSchedule) {
+      uploading_beacons_network_isolation_key_ = beacon->network_isolation_key;
+      found_beacon_to_upload = true;
+      break;
+    }
+  }
+  if (!found_beacon_to_upload)
+    return;
 
   size_t collector_index = scheduler_.OnUploadStart();
   const GURL& collector_url = *config().collectors[collector_index];
@@ -150,7 +163,7 @@ void DomainReliabilityContext::StartUpload() {
 
   uploader_->UploadReport(
       report_json, max_upload_depth, collector_url,
-      net::NetworkIsolationKey::Todo(),
+      uploading_beacons_network_isolation_key_,
       base::BindOnce(&DomainReliabilityContext::OnUploadComplete,
                      weak_factory_.GetWeakPtr()));
 }
@@ -165,24 +178,42 @@ void DomainReliabilityContext::OnUploadComplete(
   DCHECK(!upload_time_.is_null());
   last_upload_time_ = upload_time_;
   upload_time_ = base::TimeTicks();
+
+  // If there are pending beacons with a low enough depth, inform the scheduler
+  // - it's possible only some beacons were added because of NetworkIsolationKey
+  // mismatches, rather than due to new beacons being created.
+  if (GetMinBeaconUploadDepth() <= kMaxUploadDepthToSchedule)
+    scheduler_.OnBeaconAdded();
 }
 
 std::unique_ptr<const Value> DomainReliabilityContext::CreateReport(
     base::TimeTicks upload_time,
     const GURL& collector_url,
-    int* max_upload_depth_out) const {
+    int* max_upload_depth_out) {
+  DCHECK_GT(beacons_.size(), 0u);
+  DCHECK_EQ(0u, uploading_beacons_size_);
+
   int max_upload_depth = 0;
 
   std::unique_ptr<ListValue> beacons_value(new ListValue());
   for (const auto& beacon : beacons_) {
+    // Only include beacons with a matching NetworkIsolationKey in the report.
+    if (beacon->network_isolation_key !=
+        uploading_beacons_network_isolation_key_) {
+      continue;
+    }
+
     beacons_value->Append(beacon->ToValue(upload_time,
                                           *last_network_change_time_,
                                           collector_url,
                                           config().path_prefixes));
     if (beacon->upload_depth > max_upload_depth)
       max_upload_depth = beacon->upload_depth;
+    ++uploading_beacons_size_;
   }
 
+  DCHECK_GT(uploading_beacons_size_, 0u);
+
   std::unique_ptr<DictionaryValue> report_value(new DictionaryValue());
   report_value->SetString("reporter", upload_reporter_string_);
   report_value->Set("entries", std::move(beacons_value));
@@ -191,22 +222,22 @@ std::unique_ptr<const Value> DomainReliabilityContext::CreateReport(
   return std::move(report_value);
 }
 
-void DomainReliabilityContext::MarkUpload() {
-  DCHECK_EQ(0u, uploading_beacons_size_);
-  uploading_beacons_size_ = beacons_.size();
-  DCHECK_NE(0u, uploading_beacons_size_);
-}
-
 void DomainReliabilityContext::CommitUpload() {
-  auto begin = beacons_.begin();
-  auto end = begin + uploading_beacons_size_;
-  beacons_.erase(begin, end);
-  DCHECK_NE(0u, uploading_beacons_size_);
-  uploading_beacons_size_ = 0;
+  auto current = beacons_.begin();
+  while (uploading_beacons_size_ > 0) {
+    DCHECK(current != beacons_.end());
+
+    auto last = current;
+    ++current;
+    if ((*last)->network_isolation_key ==
+        uploading_beacons_network_isolation_key_) {
+      beacons_.erase(last);
+      --uploading_beacons_size_;
+    }
+  }
 }
 
 void DomainReliabilityContext::RollbackUpload() {
-  DCHECK_NE(0u, uploading_beacons_size_);
   uploading_beacons_size_ = 0;
 }
 
@@ -216,12 +247,15 @@ void DomainReliabilityContext::RemoveOldestBeacon() {
   DVLOG(1) << "Beacon queue for " << config().origin << " full; "
            << "removing oldest beacon";
 
-  beacons_.pop_front();
-
-  // If that just removed a beacon counted in uploading_beacons_size_, decrement
-  // that.
-  if (uploading_beacons_size_ > 0)
+  // If the beacon being removed has a NetworkIsolationKey that matches that of
+  // the current upload, decrement |uploading_beacons_size_|.
+  if (uploading_beacons_size_ > 0 &&
+      beacons_.front()->network_isolation_key ==
+          uploading_beacons_network_isolation_key_) {
     --uploading_beacons_size_;
+  }
+
+  beacons_.pop_front();
 }
 
 void DomainReliabilityContext::RemoveExpiredBeacons() {
@@ -231,4 +265,14 @@ void DomainReliabilityContext::RemoveExpiredBeacons() {
     beacons_.pop_front();
 }
 
+// Gets the minimum depth of all entries in |beacons_|.
+int DomainReliabilityContext::GetMinBeaconUploadDepth() const {
+  int min = std::numeric_limits<int>::max();
+  for (const auto& beacon : beacons_) {
+    if (beacon->upload_depth < min)
+      min = beacon->upload_depth;
+  }
+  return min;
+}
+
 }  // namespace domain_reliability

components/domain_reliability/context.h

@@ -7,10 +7,10 @@
 
 #include <stddef.h>
 
+#include <list>
 #include <memory>
 #include <vector>
 
-#include "base/containers/circular_deque.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
@@ -19,6 +19,7 @@
 #include "components/domain_reliability/domain_reliability_export.h"
 #include "components/domain_reliability/scheduler.h"
 #include "components/domain_reliability/uploader.h"
+#include "net/base/network_isolation_key.h"
 
 class GURL;
 
@@ -68,7 +69,7 @@ class DOMAIN_RELIABILITY_EXPORT DomainReliabilityContext {
   // debugging purposes.
   std::unique_ptr<base::Value> GetWebUIData() const;
 
-  // Gets the beacons queued for upload in this context. |*beacons_out| will be
+  // Gets the beacons queued for upload in this context. `*beacons_out` will be
   // cleared and filled with pointers to the beacons; the pointers remain valid
   // as long as no other requests are reported to the DomainReliabilityMonitor.
   void GetQueuedBeaconsForTesting(
@@ -87,17 +88,14 @@ class DOMAIN_RELIABILITY_EXPORT DomainReliabilityContext {
   void StartUpload();
   void OnUploadComplete(const DomainReliabilityUploader::UploadResult& result);
 
-  std::unique_ptr<const base::Value> CreateReport(
-      base::TimeTicks upload_time,
-      const GURL& collector_url,
-      int* max_beacon_depth_out) const;
+  // Creates a report from all beacons associated with
+  // `uploading_beacons_network_isolation_key_`. Updates
+  // `uploading_beacons_size_`.
+  std::unique_ptr<const base::Value> CreateReport(base::TimeTicks upload_time,
+                                                  const GURL& collector_url,
+                                                  int* max_beacon_depth_out);
 
-  // Remembers the current state of the context when an upload starts. Can be
-  // called multiple times in a row (without |CommitUpload|) if uploads fail
-  // and are retried.
-  void MarkUpload();
-
-  // Uses the state remembered by |MarkUpload| to remove successfully uploaded
+  // Uses the state remembered by `MarkUpload` to remove successfully uploaded
   // data but keep beacons and request counts added after the upload started.
   void CommitUpload();
 
@@ -109,6 +107,9 @@ class DOMAIN_RELIABILITY_EXPORT DomainReliabilityContext {
 
   void RemoveExpiredBeacons();
 
+  // Gets the minimum upload depth of all entries in |beacons_|.
+  int GetMinBeaconUploadDepth() const;
+
   std::unique_ptr<const DomainReliabilityConfig> config_;
   const MockableTime* time_;
   const std::string& upload_reporter_string_;
@@ -116,8 +117,16 @@ class DOMAIN_RELIABILITY_EXPORT DomainReliabilityContext {
   DomainReliabilityDispatcher* dispatcher_;
   DomainReliabilityUploader* uploader_;
 
-  base::circular_deque<std::unique_ptr<DomainReliabilityBeacon>> beacons_;
+  std::list<std::unique_ptr<DomainReliabilityBeacon>> beacons_;
+
   size_t uploading_beacons_size_;
+  // The NetworkIsolationKey associated with the beacons being uploaded. The
+  // first `uploading_beacons_size_` beacons that have NIK equal to
+  // `uploading_beacons_network_isolation_key_` are currently being uploaded.
+  // It's possible for this number to be 0 when there's still an active upload
+  // if all currently uploading beacons have been evicted.
+  net::NetworkIsolationKey uploading_beacons_network_isolation_key_;
+
   base::TimeTicks upload_time_;
   base::TimeTicks last_upload_time_;
   // The last network change time is not tracked per-context, so this is a

components/domain_reliability/test_util.cc

@@ -84,7 +84,7 @@ void TestCallback::OnCalled() {
 }
 
 MockUploader::MockUploader(UploadRequestCallback callback)
-    : callback_(std::move(callback)), discard_uploads_(true) {}
+    : callback_(callback), discard_uploads_(true) {}
 
 MockUploader::~MockUploader() = default;
 
@@ -96,8 +96,8 @@ void MockUploader::UploadReport(
     const GURL& upload_url,
     const net::NetworkIsolationKey& network_isolation_key,
     UploadCallback callback) {
-  std::move(callback_).Run(report_json, max_upload_depth, upload_url,
-                           network_isolation_key, std::move(callback));
+  callback_.Run(report_json, max_upload_depth, upload_url,
+                network_isolation_key, std::move(callback));
 }
 
 void MockUploader::Shutdown() {}

components/domain_reliability/test_util.h

@@ -41,7 +41,7 @@ class TestCallback {
 
 class MockUploader : public DomainReliabilityUploader {
  public:
-  typedef base::OnceCallback<void(
+  typedef base::RepeatingCallback<void(
       const std::string& report_json,
       int max_upload_depth,
       const GURL& upload_url,