[Backport] Remove NOTREACHED assertions from NetworkContext's reporting methods

These functions are not supposed to be called if reporting is disabled
(enable_reporting=false). The usages was supposed to be guarded on the
caller side by BUILDFLAG(ENABLE_REPORTING). Nonetheless, some of the
usages are not guarded and they are still called and cause assert on
certain web pages.

As a fix, remove the guards where it is possible from the caller site and
guard the callee site to do nothing if reporting is disabled.

Also enable ReportingServiceProxy for disabled reporting to avoid
crash/assert when reporting API is accessed via mojo.

Bug: none
Test: load https://www.usnews.com/
Change-Id: If2cc6add7eee78e842a4663f857f07bf75cce149
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3412481

8 files changed, 27 insertions, 92 deletions

diff --git a/chromium/content/browser/BUILD.gn b/chromium/content/browser/BUILD.gn
index 2659b8a75e7..6cd684305c9 100644
--- a/chromium/content/browser/BUILD.gn
+++ b/chromium/content/browser/BUILD.gn
@@ -1255,6 +1255,8 @@ jumbo_source_set("browser") {
     "net/network_errors_listing_ui.h",
     "net/network_quality_observer_impl.cc",
     "net/network_quality_observer_impl.h",
+    "net/reporting_service_proxy.cc",
+    "net/reporting_service_proxy.h",
     "network_context_client_base_impl.cc",
     "network_context_client_base_impl.h",
     "network_service_client.cc",
@@ -3084,13 +3086,6 @@ jumbo_source_set("browser") {
     ]
   }
 
-  if (enable_reporting) {
-    sources += [
-      "net/reporting_service_proxy.cc",
-      "net/reporting_service_proxy.h",
-    ]
-  }
-
   if (enable_vr) {
     if (!is_android) {
       sources += [
diff --git a/chromium/content/browser/browser_interface_binders.cc b/chromium/content/browser/browser_interface_binders.cc
index e685a1d9038..1a77d8264df 100644
--- a/chromium/content/browser/browser_interface_binders.cc
+++ b/chromium/content/browser/browser_interface_binders.cc
@@ -31,6 +31,7 @@
 #include "content/browser/media/media_web_contents_observer.h"
 #include "content/browser/media/midi_host.h"
 #include "content/browser/media/session/media_session_service_impl.h"
+#include "content/browser/net/reporting_service_proxy.h"
 #include "content/browser/picture_in_picture/picture_in_picture_service_impl.h"
 #include "content/browser/prerender/prerender_internals.mojom.h"
 #include "content/browser/prerender/prerender_internals_ui.h"
@@ -173,10 +174,6 @@
 #include "media/mojo/mojom/remoting.mojom-forward.h"
 #endif
 
-#if BUILDFLAG(ENABLE_REPORTING)
-#include "content/browser/net/reporting_service_proxy.h"
-#endif
-
 #if BUILDFLAG(GOOGLE_CHROME_BRANDING) && \
     (BUILDFLAG(IS_CHROMEOS_ASH) || BUILDFLAG(IS_CHROMEOS_LACROS))
 #include "content/public/browser/service_process_host.h"
@@ -750,10 +747,8 @@ void PopulateFrameBinders(RenderFrameHostImpl* host, mojo::BinderMap* map) {
   map->Add<blink::mojom::QuotaManagerHost>(
       base::BindRepeating(&BindQuotaManagerHost, base::Unretained(host)));
 
-#if BUILDFLAG(ENABLE_REPORTING)
   map->Add<blink::mojom::ReportingServiceProxy>(base::BindRepeating(
       &CreateReportingServiceProxyForFrame, base::Unretained(host)));
-#endif
 
   map->Add<blink::mojom::SharedWorkerConnector>(
       base::BindRepeating(&BindSharedWorkerConnector, base::Unretained(host)));
@@ -1139,10 +1134,8 @@ void PopulateDedicatedWorkerBinders(DedicatedWorkerHost* host,
   map->Add<blink::mojom::BroadcastChannelProvider>(
       base::BindRepeating(&DedicatedWorkerHost::CreateBroadcastChannelProvider,
                           base::Unretained(host)));
-#if BUILDFLAG(ENABLE_REPORTING)
   map->Add<blink::mojom::ReportingServiceProxy>(base::BindRepeating(
       &CreateReportingServiceProxyForDedicatedWorker, base::Unretained(host)));
-#endif
 #if !defined(OS_ANDROID)
   map->Add<blink::mojom::SerialService>(base::BindRepeating(
       &DedicatedWorkerHost::BindSerialService, base::Unretained(host)));
@@ -1234,10 +1227,8 @@ void PopulateSharedWorkerBinders(SharedWorkerHost* host, mojo::BinderMap* map) {
   map->Add<blink::mojom::BroadcastChannelProvider>(
       base::BindRepeating(&SharedWorkerHost::CreateBroadcastChannelProvider,
                           base::Unretained(host)));
-#if BUILDFLAG(ENABLE_REPORTING)
   map->Add<blink::mojom::ReportingServiceProxy>(base::BindRepeating(
       &CreateReportingServiceProxyForSharedWorker, base::Unretained(host)));
-#endif
 
   // RenderProcessHost binders
   map->Add<media::mojom::VideoDecodePerfHistory>(BindWorkerReceiver(
@@ -1325,10 +1316,8 @@ void PopulateServiceWorkerBinders(ServiceWorkerHost* host,
   map->Add<blink::mojom::BroadcastChannelProvider>(
       base::BindRepeating(&ServiceWorkerHost::CreateBroadcastChannelProvider,
                           base::Unretained(host)));
-#if BUILDFLAG(ENABLE_REPORTING)
   map->Add<blink::mojom::ReportingServiceProxy>(base::BindRepeating(
       &CreateReportingServiceProxyForServiceWorker, base::Unretained(host)));
-#endif
 
   // RenderProcessHost binders
   map->Add<media::mojom::VideoDecodePerfHistory>(BindServiceWorkerReceiver(
diff --git a/chromium/content/browser/browsing_data/browsing_data_remover_impl.cc b/chromium/content/browser/browsing_data/browsing_data_remover_impl.cc
index f5ee91e7eb0..a698131c223 100644
--- a/chromium/content/browser/browsing_data/browsing_data_remover_impl.cc
+++ b/chromium/content/browser/browsing_data/browsing_data_remover_impl.cc
@@ -530,9 +530,9 @@ void BrowsingDataRemoverImpl::RemoveImpl(
         CreateTaskCompletionClosureForMojo(TracingDataType::kTrustTokens));
   }
 
-#if BUILDFLAG(ENABLE_REPORTING)
   //////////////////////////////////////////////////////////////////////////////
   // Reporting cache.
+  // TODO(https://crbug.com/1291489): Add unit test to cover this.
   if (remove_mask & DATA_TYPE_COOKIES) {
     network::mojom::NetworkContext* network_context =
         browser_context_->GetDefaultStoragePartition()->GetNetworkContext();
@@ -544,7 +544,6 @@ void BrowsingDataRemoverImpl::RemoveImpl(
         CreateTaskCompletionClosureForMojo(
             TracingDataType::kNetworkErrorLogging));
   }
-#endif  // BUILDFLAG(ENABLE_REPORTING)
 
   //////////////////////////////////////////////////////////////////////////////
   // Auth cache.
diff --git a/chromium/content/browser/browsing_data/browsing_data_remover_impl_unittest.cc b/chromium/content/browser/browsing_data/browsing_data_remover_impl_unittest.cc
index 5a70d89f741..9c61fb42063 100644
--- a/chromium/content/browser/browsing_data/browsing_data_remover_impl_unittest.cc
+++ b/chromium/content/browser/browsing_data/browsing_data_remover_impl_unittest.cc
@@ -62,17 +62,6 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/origin.h"
 
-#if BUILDFLAG(ENABLE_REPORTING)
-#include "net/network_error_logging/mock_persistent_nel_store.h"
-#include "net/network_error_logging/network_error_logging_service.h"
-#include "net/reporting/mock_persistent_reporting_store.h"
-#include "net/reporting/reporting_cache.h"
-#include "net/reporting/reporting_endpoint.h"
-#include "net/reporting/reporting_report.h"
-#include "net/reporting/reporting_service.h"
-#include "net/reporting/reporting_test_util.h"
-#endif  // BUILDFLAG(ENABLE_REPORTING)
-
 using base::test::RunOnceClosure;
 using testing::_;
 using testing::ByRef;
diff --git a/chromium/content/browser/net/reporting_service_proxy.cc b/chromium/content/browser/net/reporting_service_proxy.cc
index 695704d10d0..5868b2d83bb 100644
--- a/chromium/content/browser/net/reporting_service_proxy.cc
+++ b/chromium/content/browser/net/reporting_service_proxy.cc
@@ -14,16 +14,12 @@
 #include "content/browser/service_worker/service_worker_host.h"
 #include "content/browser/worker_host/dedicated_worker_host.h"
 #include "content/browser/worker_host/shared_worker_host.h"
-#include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
-#include "content/public/browser/site_instance.h"
 #include "content/public/browser/storage_partition.h"
 #include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "net/base/network_isolation_key.h"
-#include "net/reporting/reporting_report.h"
-#include "net/reporting/reporting_service.h"
 #include "services/network/public/mojom/network_context.mojom.h"
 #include "third_party/blink/public/mojom/reporting/reporting.mojom.h"
 #include "url/gurl.h"
diff --git a/chromium/content/web_test/browser/web_test_content_browser_client.cc b/chromium/content/web_test/browser/web_test_content_browser_client.cc
index 99d4577526d..b334e66f426 100644
--- a/chromium/content/web_test/browser/web_test_content_browser_client.cc
+++ b/chromium/content/web_test/browser/web_test_content_browser_client.cc
@@ -544,7 +544,6 @@ void WebTestContentBrowserClient::ConfigureNetworkContextParamsForShell(
   ShellContentBrowserClient::ConfigureNetworkContextParamsForShell(
       context, context_params, cert_verifier_creation_params);
 
-#if BUILDFLAG(ENABLE_REPORTING)
   // Configure the Reporting service in a manner expected by certain Web
   // Platform Tests (network-error-logging and reporting-api).
   //
@@ -553,7 +552,6 @@ void WebTestContentBrowserClient::ConfigureNetworkContextParamsForShell(
   context_params->reporting_delivery_interval =
       kReportingDeliveryIntervalTimeForWebTests;
   context_params->skip_reporting_send_permission_check = true;
-#endif
 }
 
 void WebTestContentBrowserClient::CreateFakeBluetoothChooserFactory(
diff --git a/chromium/services/network/network_context.cc b/chromium/services/network/network_context.cc
index db0142de45e..3f482aa2351 100644
--- a/chromium/services/network/network_context.cc
+++ b/chromium/services/network/network_context.cc
@@ -826,7 +826,11 @@ size_t NetworkContext::GetNumOutstandingResolveHostRequestsForTesting() const {
 }
 
 bool NetworkContext::SkipReportingPermissionCheck() const {
+#if BUILDFLAG(ENABLE_REPORTING)
   return params_ && params_->skip_reporting_send_permission_check;
+#else
+  return false;
+#endif
 }
 
 void NetworkContext::ClearTrustTokenData(mojom::ClearDataFilterPtr filter,
@@ -912,10 +916,10 @@ void NetworkContext::ClearHttpAuthCache(base::Time start_time,
   std::move(callback).Run();
 }
 
-#if BUILDFLAG(ENABLE_REPORTING)
 void NetworkContext::ClearReportingCacheReports(
     mojom::ClearDataFilterPtr filter,
     ClearReportingCacheReportsCallback callback) {
+#if BUILDFLAG(ENABLE_REPORTING)
   net::ReportingService* reporting_service =
       url_request_context_->reporting_service();
   if (reporting_service) {
@@ -928,6 +932,7 @@ void NetworkContext::ClearReportingCacheReports(
           net::ReportingBrowsingDataRemover::DATA_TYPE_REPORTS);
     }
   }
+#endif  // BUILDFLAG(ENABLE_REPORTING)
 
   std::move(callback).Run();
 }
@@ -935,6 +940,7 @@ void NetworkContext::ClearReportingCacheReports(
 void NetworkContext::ClearReportingCacheClients(
     mojom::ClearDataFilterPtr filter,
     ClearReportingCacheClientsCallback callback) {
+#if BUILDFLAG(ENABLE_REPORTING)
   net::ReportingService* reporting_service =
       url_request_context_->reporting_service();
   if (reporting_service) {
@@ -947,6 +953,7 @@ void NetworkContext::ClearReportingCacheClients(
           net::ReportingBrowsingDataRemover::DATA_TYPE_CLIENTS);
     }
   }
+#endif  // BUILDFLAG(ENABLE_REPORTING)
 
   std::move(callback).Run();
 }
@@ -954,6 +961,7 @@ void NetworkContext::ClearReportingCacheClients(
 void NetworkContext::ClearNetworkErrorLogging(
     mojom::ClearDataFilterPtr filter,
     ClearNetworkErrorLoggingCallback callback) {
+#if BUILDFLAG(ENABLE_REPORTING)
   net::NetworkErrorLoggingService* logging_service =
       url_request_context_->network_error_logging_service();
   if (logging_service) {
@@ -963,6 +971,7 @@ void NetworkContext::ClearNetworkErrorLogging(
       logging_service->RemoveAllBrowsingData();
     }
   }
+#endif  // BUILDFLAG(ENABLE_REPORTING)
 
   std::move(callback).Run();
 }
@@ -972,6 +981,7 @@ void NetworkContext::SetDocumentReportingEndpoints(
     const url::Origin& origin,
     const net::IsolationInfo& isolation_info,
     const base::flat_map<std::string, std::string>& endpoints) {
+#if BUILDFLAG(ENABLE_REPORTING)
   DCHECK(!reporting_source.is_empty());
   DCHECK_EQ(net::IsolationInfo::RequestType::kOther,
             isolation_info.request_type());
@@ -981,15 +991,18 @@ void NetworkContext::SetDocumentReportingEndpoints(
     reporting_service->SetDocumentReportingEndpoints(reporting_source, origin,
                                                      isolation_info, endpoints);
   }
+#endif  // BUILDFLAG(ENABLE_REPORTING)
 }
 
 void NetworkContext::SendReportsAndRemoveSource(
     const base::UnguessableToken& reporting_source) {
+#if BUILDFLAG(ENABLE_REPORTING)
   DCHECK(!reporting_source.is_empty());
   net::ReportingService* reporting_service =
       url_request_context()->reporting_service();
   if (reporting_service)
     reporting_service->SendReportsAndRemoveSource(reporting_source);
+#endif  // BUILDFLAG(ENABLE_REPORTING)
 }
 
 void NetworkContext::QueueReport(
@@ -1000,6 +1013,7 @@ void NetworkContext::QueueReport(
     const net::NetworkIsolationKey& network_isolation_key,
     const absl::optional<std::string>& user_agent,
     base::Value body) {
+#if BUILDFLAG(ENABLE_REPORTING)
   // If |reporting_source| is provided, it must not be empty.
   DCHECK(!(reporting_source.has_value() && reporting_source->is_empty()));
   if (require_network_isolation_key_)
@@ -1028,11 +1042,13 @@ void NetworkContext::QueueReport(
   reporting_service->QueueReport(
       url, reporting_source, network_isolation_key, reported_user_agent, group,
       type, base::Value::ToUniquePtrValue(std::move(body)), 0 /* depth */);
+#endif  // BUILDFLAG(ENABLE_REPORTING)
 }
 
 void NetworkContext::QueueSignedExchangeReport(
     mojom::SignedExchangeReportPtr report,
     const net::NetworkIsolationKey& network_isolation_key) {
+#if BUILDFLAG(ENABLE_REPORTING)
   if (require_network_isolation_key_)
     DCHECK(!network_isolation_key.IsEmpty());
 
@@ -1060,8 +1076,10 @@ void NetworkContext::QueueSignedExchangeReport(
   details.elapsed_time = report->elapsed_time;
   details.user_agent = std::move(user_agent);
   logging_service->QueueSignedExchangeReport(std::move(details));
+#endif  // BUILDFLAG(ENABLE_REPORTING)
 }
 
+#if BUILDFLAG(ENABLE_REPORTING)
 void NetworkContext::AddReportingApiObserver(
     mojo::PendingRemote<network::mojom::ReportingApiObserver> observer) {
   if (url_request_context() && url_request_context()->reporting_service()) {
@@ -1121,55 +1139,6 @@ void NetworkContext::OnReportingObserverDisconnect(
     is_observing_reporting_service_ = false;
   }
 }
-
-#else   // BUILDFLAG(ENABLE_REPORTING)
-void NetworkContext::ClearReportingCacheReports(
-    mojom::ClearDataFilterPtr filter,
-    ClearReportingCacheReportsCallback callback) {
-  NOTREACHED();
-}
-
-void NetworkContext::ClearReportingCacheClients(
-    mojom::ClearDataFilterPtr filter,
-    ClearReportingCacheClientsCallback callback) {
-  NOTREACHED();
-}
-
-void NetworkContext::ClearNetworkErrorLogging(
-    mojom::ClearDataFilterPtr filter,
-    ClearNetworkErrorLoggingCallback callback) {
-  NOTREACHED();
-}
-
-void NetworkContext::SetDocumentReportingEndpoints(
-    const base::UnguessableToken& reporting_source,
-    const url::Origin& origin,
-    const net::IsolationInfo& isolation_info,
-    const base::flat_map<std::string, std::string>& endpoints) {
-  NOTREACHED();
-}
-
-void NetworkContext::SendReportsAndRemoveSource(
-    const base::UnguessableToken& reporting_source) {
-  NOTREACHED();
-}
-
-void NetworkContext::QueueReport(
-    const std::string& type,
-    const std::string& group,
-    const GURL& url,
-    const absl::optional<base::UnguessableToken>& reporting_source,
-    const net::NetworkIsolationKey& network_isolation_key,
-    const absl::optional<std::string>& user_agent,
-    base::Value body) {
-  NOTREACHED();
-}
-
-void NetworkContext::QueueSignedExchangeReport(
-    mojom::SignedExchangeReportPtr report,
-    const net::NetworkIsolationKey& network_isolation_key) {
-  NOTREACHED();
-}
 #endif  // BUILDFLAG(ENABLE_REPORTING)
 
 void NetworkContext::ClearDomainReliability(
diff --git a/chromium/services/network/public/mojom/network_context.mojom b/chromium/services/network/public/mojom/network_context.mojom
index 0240d83bbbb..cf48d2ea97a 100644
--- a/chromium/services/network/public/mojom/network_context.mojom
+++ b/chromium/services/network/public/mojom/network_context.mojom
@@ -919,22 +919,22 @@ interface NetworkContext {
   ClearHttpAuthCache(mojo_base.mojom.Time start_time,
                      mojo_base.mojom.Time end_time) => ();
 
-  // Clears all report entries from the reporting cache. Should not be called if
+  // Clears all report entries from the reporting cache. This has no effect if
   // the ENABLE_REPORTING build flag is false.
   //
   // If a non-null |filter| is specified, will clear only entries matching the
   // filter.
   ClearReportingCacheReports(ClearDataFilter? filter) => ();
 
-  // Clears all client entries from the reporting cache. Should not be called if
+  // Clears all client entries from the reporting cache. This has no effect if
   // the ENABLE_REPORTING build flag is false.
   //
   // If a non-null |filter| is specified, will clear only entries matching the
   // filter.
   ClearReportingCacheClients(ClearDataFilter? filter) => ();
 
-  // Clears policy entries from the NetworkErrorLoggingService. Should not be
-  // called if the ENABLE_REPORTING build flag is false.
+  // Clears policy entries from the NetworkErrorLoggingService. This has no
+  // effect if the ENABLE_REPORTING build flag is false.
   //
   // If a non-null |filter| is specified, will clear only entries matching the
   // filter.