Reporting and Network Error Logging: Add OnShutdown() methods
This CL adds methods to shut down the ReportingService and the
NetworkErrorLoggingService. Previously, we were destroying the
NEL service and the ReportingService when the NetworkContext was
torn down, leading to a use-after-free when destroying the
ReportingService caused pending uploads to be cancelled,
generating NEL reports for the cancelled requests, that would be sent
to the already-destroyed NEL service. This fixes the issue by shutting
down rather than destroying the NEL service and ReportingService in the
destructor of ContainerURLRequestContext.
Bug: 935209
Change-Id: Id53d8430a974336b8016d0cd73fa837f2a1e1d1b
Reviewed-on: https://chromium-review.googlesource.com/c/1489098
Reviewed-by: Martin Šrámek <[email protected]>
Reviewed-by: Matt Menke <[email protected]>
Commit-Queue: Lily Chen <[email protected]>
Cr-Commit-Position: refs/heads/master@{#636837}diff --git a/net/reporting/reporting_context.cc b/net/reporting/reporting_context.cc
index 8dd28a4..5228305 100644
--- a/net/reporting/reporting_context.cc
+++ b/net/reporting/reporting_context.cc
@@ -75,6 +75,10 @@
observer.OnClientsUpdated();
}
+void ReportingContext::OnShutdown() {
+ uploader_->OnShutdown();
+}
+
ReportingContext::ReportingContext(const ReportingPolicy& policy,
base::Clock* clock,
const base::TickClock* tick_clock,
diff --git a/net/reporting/reporting_context.h b/net/reporting/reporting_context.h
index 2c27c75..73d1476 100644
--- a/net/reporting/reporting_context.h
+++ b/net/reporting/reporting_context.h
@@ -63,6 +63,8 @@
void NotifyCachedReportsUpdated();
void NotifyCachedClientsUpdated();
+ void OnShutdown();
+
protected:
ReportingContext(const ReportingPolicy& policy,
base::Clock* clock,
diff --git a/net/reporting/reporting_service.cc b/net/reporting/reporting_service.cc
index ae9f290..16901a0c94 100644
--- a/net/reporting/reporting_service.cc
+++ b/net/reporting/reporting_service.cc
@@ -31,7 +31,7 @@
class ReportingServiceImpl : public ReportingService {
public:
ReportingServiceImpl(std::unique_ptr<ReportingContext> context)
- : context_(std::move(context)) {}
+ : context_(std::move(context)), shut_down_(false) {}
// ReportingService implementation:
@@ -43,6 +43,9 @@
const std::string& type,
std::unique_ptr<const base::Value> body,
int depth) override {
+ if (shut_down_)
+ return;
+
DCHECK(context_);
DCHECK(context_->delegate());
@@ -61,6 +64,9 @@
void ProcessHeader(const GURL& url,
const std::string& header_string) override {
+ if (shut_down_)
+ return;
+
if (header_string.size() > kMaxJsonSize) {
ReportingHeaderParser::RecordHeaderDiscardedForJsonTooBig();
return;
@@ -91,6 +97,11 @@
data_type_mask);
}
+ void OnShutdown() override {
+ shut_down_ = true;
+ context_->OnShutdown();
+ }
+
const ReportingPolicy& GetPolicy() const override {
return context_->policy();
}
@@ -103,8 +114,13 @@
return dict;
}
+ ReportingContext* GetContextForTesting() const override {
+ return context_.get();
+ }
+
private:
std::unique_ptr<ReportingContext> context_;
+ bool shut_down_;
DISALLOW_COPY_AND_ASSIGN(ReportingServiceImpl);
};
diff --git a/net/reporting/reporting_service.h b/net/reporting/reporting_service.h
index b13c8f6d..c7671d1 100644
--- a/net/reporting/reporting_service.h
+++ b/net/reporting/reporting_service.h
@@ -72,10 +72,16 @@
// filter.
virtual void RemoveAllBrowsingData(int data_type_mask) = 0;
+ // Shuts down the Reporting service so that no new headers or reports are
+ // processed, and pending uploads are cancelled.
+ virtual void OnShutdown() = 0;
+
virtual const ReportingPolicy& GetPolicy() const = 0;
virtual base::Value StatusAsValue() const;
+ virtual ReportingContext* GetContextForTesting() const = 0;
+
protected:
ReportingService() {}
diff --git a/net/reporting/reporting_test_util.cc b/net/reporting/reporting_test_util.cc
index 0eb369a..125b6fc 100644
--- a/net/reporting/reporting_test_util.cc
+++ b/net/reporting/reporting_test_util.cc
@@ -112,6 +112,14 @@
base::BindOnce(&ErasePendingUpload, &pending_uploads_)));
}
+void TestReportingUploader::OnShutdown() {
+ pending_uploads_.clear();
+}
+
+int TestReportingUploader::GetPendingUploadCountForTesting() const {
+ return pending_uploads_.size();
+}
+
TestReportingDelegate::TestReportingDelegate()
: test_request_context_(std::make_unique<TestURLRequestContext>()) {}
diff --git a/net/reporting/reporting_test_util.h b/net/reporting/reporting_test_util.h
index d0c3f5ce..5950e5a 100644
--- a/net/reporting/reporting_test_util.h
+++ b/net/reporting/reporting_test_util.h
@@ -79,6 +79,10 @@
int max_depth,
UploadCallback callback) override;
+ void OnShutdown() override;
+
+ int GetPendingUploadCountForTesting() const override;
+
private:
std::vector<std::unique_ptr<PendingUpload>> pending_uploads_;
diff --git a/net/reporting/reporting_uploader.cc b/net/reporting/reporting_uploader.cc
index 87e1318..5e5848b7 100644
--- a/net/reporting/reporting_uploader.cc
+++ b/net/reporting/reporting_uploader.cc
@@ -155,6 +155,11 @@
}
}
+ void OnShutdown() override {
+ // Cancels all pending uploads.
+ uploads_.clear();
+ }
+
void StartPreflightRequest(std::unique_ptr<PendingUpload> upload) {
DCHECK(upload->state == PendingUpload::CREATED);
@@ -321,6 +326,10 @@
NOTREACHED();
}
+ int GetPendingUploadCountForTesting() const override {
+ return uploads_.size();
+ }
+
private:
const URLRequestContext* context_;
std::map<const URLRequest*, std::unique_ptr<PendingUpload>> uploads_;
diff --git a/net/reporting/reporting_uploader.h b/net/reporting/reporting_uploader.h
index 96c91cc..0f3948a 100644
--- a/net/reporting/reporting_uploader.h
+++ b/net/reporting/reporting_uploader.h
@@ -41,10 +41,15 @@
int max_depth,
UploadCallback callback) = 0;
+ // Cancels pending uploads.
+ virtual void OnShutdown() = 0;
+
// Creates a real implementation of |ReportingUploader| that uploads reports
// using |context|.
static std::unique_ptr<ReportingUploader> Create(
const URLRequestContext* context);
+
+ virtual int GetPendingUploadCountForTesting() const = 0;
};
} // namespace net
