Network Error Logging: Ignore Reporting API upload requests

Right now, the ReportingUploader cancels requests once the headers are
received, which causes the requests to fail with ERR_ABORTED, which
triggers a NEL report.

This CL breaks that loop by attaching user data to upload requests and
ignoring ERR_ABORTED errors stemming from them.

TBR=mkwst@chromium.org

Bug: 793392
Change-Id: Id26516533ec0e7531797717526b015c56a742c1b
Reviewed-on: https://chromium-review.googlesource.com/817553
Commit-Queue: Julia Tuttle <juliatuttle@chromium.org>
Reviewed-by: Miriam Gershenson <mgersh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524390}

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc

@@ -942,6 +942,11 @@ class MockReportingService : public net::ReportingService {
     last_origin_filter_ = origin_filter;
   }
 
+  bool RequestIsUpload(const net::URLRequest& request) override {
+    NOTREACHED();
+    return true;
+  }
+
   int remove_calls() const { return remove_calls_; }
   int last_data_type_mask() const { return last_data_type_mask_; }
   base::Callback<bool(const GURL&)> last_origin_filter() const {

net/network_error_logging/network_error_logging_service.cc

@@ -171,6 +171,12 @@ void NetworkErrorLoggingService::OnNetworkError(const ErrorDetails& details) {
   if (!reporting_service_)
     return;
 
+  // It is expected for Reporting uploads to terminate with ERR_ABORTED, since
+  // the ReportingUploader cancels them after receiving the response code and
+  // headers.
+  if (details.is_reporting_upload && details.type == ERR_ABORTED)
+    return;
+
   url::Origin origin = url::Origin::Create(details.uri);
 
   // NEL is only available to secure origins, so ignore network errors from

net/network_error_logging/network_error_logging_service_unittest.cc

@@ -80,6 +80,11 @@ class TestReportingService : public ReportingService {
     NOTREACHED();
   }
 
+  bool RequestIsUpload(const URLRequest& request) override {
+    NOTREACHED();
+    return true;
+  }
+
  private:
   std::vector<Report> reports_;
 

net/reporting/reporting_service.cc

@@ -16,6 +16,7 @@
 #include "net/reporting/reporting_context.h"
 #include "net/reporting/reporting_delegate.h"
 #include "net/reporting/reporting_header_parser.h"
+#include "net/reporting/reporting_uploader.h"
 #include "url/gurl.h"
 
 namespace net {
@@ -27,6 +28,8 @@ class ReportingServiceImpl : public ReportingService {
   ReportingServiceImpl(std::unique_ptr<ReportingContext> context)
       : context_(std::move(context)) {}
 
+  // ReportingService implementation:
+
   ~ReportingServiceImpl() override = default;
 
   void QueueReport(const GURL& url,
@@ -52,6 +55,10 @@ class ReportingServiceImpl : public ReportingService {
         context_->cache(), data_type_mask, origin_filter);
   }
 
+  bool RequestIsUpload(const URLRequest& request) override {
+    return context_->uploader()->RequestIsUpload(request);
+  }
+
  private:
   std::unique_ptr<ReportingContext> context_;
 

net/reporting/reporting_service.h

@@ -22,6 +22,7 @@ namespace net {
 
 class ReportingContext;
 struct ReportingPolicy;
+class URLRequest;
 class URLRequestContext;
 
 // The external interface to the Reporting system, used by the embedder of //net
@@ -65,6 +66,10 @@ class NET_EXPORT ReportingService {
       int data_type_mask,
       base::RepeatingCallback<bool(const GURL&)> origin_filter) = 0;
 
+  // Checks whether |request| is a Reporting upload, to avoid loops of reporting
+  // about report uploads.
+  virtual bool RequestIsUpload(const URLRequest& request) = 0;
+
  protected:
   ReportingService() {}
 

net/reporting/reporting_test_util.cc

@@ -105,6 +105,11 @@ void TestReportingUploader::StartUpload(const GURL& url,
       base::BindOnce(&ErasePendingUpload, &pending_uploads_)));
 }
 
+bool TestReportingUploader::RequestIsUpload(const URLRequest& request) {
+  NOTIMPLEMENTED();
+  return true;
+}
+
 TestReportingDelegate::TestReportingDelegate() = default;
 
 TestReportingDelegate::~TestReportingDelegate() = default;

net/reporting/reporting_test_util.h

@@ -68,10 +68,13 @@ class TestReportingUploader : public ReportingUploader {
   }
 
   // ReportingUploader implementation:
+
   void StartUpload(const GURL& url,
                    const std::string& json,
                    UploadCallback callback) override;
 
+  bool RequestIsUpload(const URLRequest& request) override;
+
  private:
   std::vector<std::unique_ptr<PendingUpload>> pending_uploads_;
 

net/reporting/reporting_uploader.cc

@@ -23,6 +23,15 @@ namespace net {
 
 namespace {
 
+class UploadUserData : public base::SupportsUserData::Data {
+ public:
+  static const void* const kUserDataKey;
+};
+
+// SetUserData needs a unique const void* to serve as the key, so create a const
+// void* and use its own address as the unique pointer.
+const void* const UploadUserData::kUserDataKey = &UploadUserData::kUserDataKey;
+
 ReportingUploader::Outcome ResponseCodeToOutcome(int response_code) {
   if (response_code >= 200 && response_code <= 299)
     return ReportingUploader::Outcome::SUCCESS;
@@ -85,6 +94,9 @@ class ReportingUploaderImpl : public ReportingUploader, URLRequest::Delegate {
     request->set_upload(
         ElementsUploadDataStream::CreateWithReader(std::move(reader), 0));
 
+    request->SetUserData(UploadUserData::kUserDataKey,
+                         std::make_unique<UploadUserData>());
+
     // This inherently sets mode = "no-cors", but that doesn't matter, because
     // the origins that are included in the upload don't actually get to see
     // the response.
@@ -98,6 +110,11 @@ class ReportingUploaderImpl : public ReportingUploader, URLRequest::Delegate {
     *upload = std::make_unique<Upload>(std::move(request), std::move(callback));
   }
 
+  // static
+  bool RequestIsUpload(const net::URLRequest& request) override {
+    return request.GetUserData(UploadUserData::kUserDataKey);
+  }
+
   // URLRequest::Delegate implementation:
 
   void OnReceivedRedirect(URLRequest* request,

net/reporting/reporting_uploader.h

@@ -15,6 +15,7 @@ class GURL;
 
 namespace net {
 
+class URLRequest;
 class URLRequestContext;
 
 // Uploads already-serialized reports and converts responses to one of the
@@ -35,6 +36,9 @@ class NET_EXPORT ReportingUploader {
                            const std::string& json,
                            UploadCallback callback) = 0;
 
+  // Returns whether |request| is an upload request sent by this uploader.
+  virtual bool RequestIsUpload(const URLRequest& request) = 0;
+
   // Creates a real implementation of |ReportingUploader| that uploads reports
   // using |context|.
   static std::unique_ptr<ReportingUploader> Create(

net/url_request/network_error_logging_delegate.h

@@ -38,6 +38,8 @@ class NET_EXPORT NetworkErrorLoggingDelegate {
     int status_code;
     base::TimeDelta elapsed_time;
     Error type;
+
+    bool is_reporting_upload;
   };
 
   static const char kHeaderName[];

net/url_request/url_request.cc

@@ -31,6 +31,7 @@
 #include "net/log/net_log.h"
 #include "net/log/net_log_event_type.h"
 #include "net/log/net_log_source_type.h"
+#include "net/reporting/reporting_service.h"
 #include "net/ssl/ssl_cert_request_info.h"
 #include "net/url_request/redirect_info.h"
 #include "net/url_request/redirect_util.h"
@@ -1194,6 +1195,10 @@ void URLRequest::MaybeGenerateNetworkErrorLoggingReport() {
       base::TimeTicks::Now() - load_timing_info_.request_start;
   details.type = status().ToNetError();
 
+  details.is_reporting_upload =
+      context()->reporting_service() &&
+      context()->reporting_service()->RequestIsUpload(*this);
+
   delegate->OnNetworkError(details);
 }
 #endif  // BUILDFLAG(ENABLE_REPORTING)

net/url_request/url_request_unittest.cc

@@ -7043,6 +7043,11 @@ class TestReportingService : public ReportingService {
     NOTIMPLEMENTED();
   }
 
+  bool RequestIsUpload(const URLRequest& request) override {
+    NOTIMPLEMENTED();
+    return true;
+  }
+
  private:
   std::vector<Header> headers_;
 };