Revert "Network Error Logging: Ignore Reporting API upload requests"

This reverts commit a8a1b79. Reason for revert: Caused MSan failures. (Thanks Findit!) https://luci-milo.appspot.com/buildbot/chromium.memory/Linux%20MSan%20Tests/6611 Original change's description: > 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} TBR=mgersh@chromium.org,juliatuttle@chromium.org Change-Id: I8aae7138cb616fb86e038463c80560f9e916f0c9 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 793392 Reviewed-on: https://chromium-review.googlesource.com/830313 Reviewed-by: Miriam Gershenson <mgersh@chromium.org> Commit-Queue: Miriam Gershenson <mgersh@chromium.org> Cr-Commit-Position: refs/heads/master@{#524419}
marcosholgado · Dec 15, 2017 · 7dc15ab · 7dc15ab
1 parent ca38723
commit 7dc15ab
Show file tree

Hide file tree

Showing 12 changed files with 0 additions and 69 deletions.
diff --git a/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc b/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
@@ -942,11 +942,6 @@ 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 {

diff --git a/net/network_error_logging/network_error_logging_service.cc b/net/network_error_logging/network_error_logging_service.cc
@@ -171,12 +171,6 @@ 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

diff --git a/net/network_error_logging/network_error_logging_service_unittest.cc b/net/network_error_logging/network_error_logging_service_unittest.cc
@@ -80,11 +80,6 @@ class TestReportingService : public ReportingService {
     NOTREACHED();
   }
 
-  bool RequestIsUpload(const URLRequest& request) override {
-    NOTREACHED();
-    return true;
-  }
-
  private:
   std::vector<Report> reports_;
 

diff --git a/net/reporting/reporting_service.cc b/net/reporting/reporting_service.cc
@@ -16,7 +16,6 @@
 #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 {
@@ -28,8 +27,6 @@ class ReportingServiceImpl : public ReportingService {
   ReportingServiceImpl(std::unique_ptr<ReportingContext> context)
       : context_(std::move(context)) {}
 
-  // ReportingService implementation:
-
   ~ReportingServiceImpl() override = default;
 
   void QueueReport(const GURL& url,
@@ -55,10 +52,6 @@ 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_;
 

diff --git a/net/reporting/reporting_service.h b/net/reporting/reporting_service.h
@@ -22,7 +22,6 @@ namespace net {
 
 class ReportingContext;
 struct ReportingPolicy;
-class URLRequest;
 class URLRequestContext;
 
 // The external interface to the Reporting system, used by the embedder of //net
@@ -66,10 +65,6 @@ 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() {}
 

diff --git a/net/reporting/reporting_test_util.cc b/net/reporting/reporting_test_util.cc
@@ -105,11 +105,6 @@ 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;

diff --git a/net/reporting/reporting_test_util.h b/net/reporting/reporting_test_util.h
@@ -68,13 +68,10 @@ 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_;
 

diff --git a/net/reporting/reporting_uploader.cc b/net/reporting/reporting_uploader.cc
@@ -23,15 +23,6 @@ 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;
@@ -94,9 +85,6 @@ 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.
@@ -110,11 +98,6 @@ 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,

diff --git a/net/reporting/reporting_uploader.h b/net/reporting/reporting_uploader.h
@@ -15,7 +15,6 @@ class GURL;
 
 namespace net {
 
-class URLRequest;
 class URLRequestContext;
 
 // Uploads already-serialized reports and converts responses to one of the
@@ -36,9 +35,6 @@ 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(

diff --git a/net/url_request/network_error_logging_delegate.h b/net/url_request/network_error_logging_delegate.h
@@ -38,8 +38,6 @@ class NET_EXPORT NetworkErrorLoggingDelegate {
     int status_code;
     base::TimeDelta elapsed_time;
     Error type;
-
-    bool is_reporting_upload;
   };
 
   static const char kHeaderName[];

diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
@@ -31,7 +31,6 @@
 #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"
@@ -1195,10 +1194,6 @@ 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)

diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
@@ -7043,11 +7043,6 @@ class TestReportingService : public ReportingService {
     NOTIMPLEMENTED();
   }
 
-  bool RequestIsUpload(const URLRequest& request) override {
-    NOTIMPLEMENTED();
-    return true;
-  }
-
  private:
   std::vector<Header> headers_;
 };