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
Reviewed-by: Rodney Ding <rodneyding@google.com>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#965655}

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc

@@ -1146,7 +1146,6 @@ void ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData(
       (constants::DATA_TYPE_ISOLATED_ORIGINS | constants::DATA_TYPE_HISTORY))
     browsing_data::RemoveSiteIsolationData(prefs);
 
-#if BUILDFLAG(ENABLE_REPORTING)
   if (remove_mask & constants::DATA_TYPE_HISTORY) {
     network::mojom::NetworkContext* network_context =
         profile_->GetDefaultStoragePartition()->GetNetworkContext();
@@ -1158,7 +1157,6 @@ void ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData(
         CreateTaskCompletionClosureForMojo(
             TracingDataType::kNetworkErrorLogging));
   }
-#endif  // BUILDFLAG(ENABLE_REPORTING)
 
 //////////////////////////////////////////////////////////////////////////////
 // DATA_TYPE_WEB_APP_DATA

content/browser/BUILD.gn

@@ -1234,6 +1234,8 @@ 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",
@@ -3035,13 +3037,6 @@ source_set("browser") {
     ]
   }
 
-  if (enable_reporting) {
-    sources += [
-      "net/reporting_service_proxy.cc",
-      "net/reporting_service_proxy.h",
-    ]
-  }
-
   if (enable_vr) {
     if (!is_android) {
       sources += [

content/browser/browser_interface_binders.cc

@@ -32,6 +32,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"
@@ -818,10 +815,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)));
@@ -1212,10 +1207,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 !BUILDFLAG(IS_ANDROID)
   map->Add<blink::mojom::SerialService>(base::BindRepeating(
       &DedicatedWorkerHost::BindSerialService, base::Unretained(host)));
@@ -1307,10 +1300,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(
@@ -1398,10 +1389,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(

content/browser/browsing_data/browsing_data_remover_impl.cc

@@ -534,9 +534,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();
@@ -548,7 +548,6 @@ void BrowsingDataRemoverImpl::RemoveImpl(
         CreateTaskCompletionClosureForMojo(
             TracingDataType::kNetworkErrorLogging));
   }
-#endif  // BUILDFLAG(ENABLE_REPORTING)
 
   //////////////////////////////////////////////////////////////////////////////
   // Auth cache.

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;

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"

content/web_test/browser/web_test_content_browser_client.cc

@@ -545,7 +545,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).
   //
@@ -554,7 +553,6 @@ void WebTestContentBrowserClient::ConfigureNetworkContextParamsForShell(
   context_params->reporting_delivery_interval =
       kReportingDeliveryIntervalTimeForWebTests;
   context_params->skip_reporting_send_permission_check = true;
-#endif
 }
 
 void WebTestContentBrowserClient::CreateFakeBluetoothChooserFactory(

services/network/network_context.cc

@@ -860,7 +860,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,
@@ -946,10 +950,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) {
@@ -962,6 +966,7 @@ void NetworkContext::ClearReportingCacheReports(
           net::ReportingBrowsingDataRemover::DATA_TYPE_REPORTS);
     }
   }
+#endif  // BUILDFLAG(ENABLE_REPORTING)
 
   std::move(callback).Run();
 }
@@ -969,6 +974,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) {
@@ -981,6 +987,7 @@ void NetworkContext::ClearReportingCacheClients(
           net::ReportingBrowsingDataRemover::DATA_TYPE_CLIENTS);
     }
   }
+#endif  // BUILDFLAG(ENABLE_REPORTING)
 
   std::move(callback).Run();
 }
@@ -988,6 +995,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) {
@@ -997,6 +1005,7 @@ void NetworkContext::ClearNetworkErrorLogging(
       logging_service->RemoveAllBrowsingData();
     }
   }
+#endif  // BUILDFLAG(ENABLE_REPORTING)
 
   std::move(callback).Run();
 }
@@ -1006,6 +1015,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());
@@ -1015,15 +1025,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(
@@ -1034,6 +1047,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_)
@@ -1062,11 +1076,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());
 
@@ -1094,8 +1110,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()) {
@@ -1155,55 +1173,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(

services/network/public/mojom/network_context.mojom

@@ -925,22 +925,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.