make a singleton ChromeDataUseMeasurement for network service

When network service is enabled, it is possible for ServiceManager
to launch network service without the browser process. This will cause
a crash in ChromeContentBrowserClient::OnNetworkServiceDataUseUpdate().
This CL fixes the issue by making ChromeDataUsemeasurement a singleton
for network service. Also it fixes a bunch of BrowserThread checks
as BrowserThread will not be available when browser process isn't created.

BUG=898970

Change-Id: I8638239c62a852374f52d5832d86558f4c4bef19
Reviewed-on: https://chromium-review.googlesource.com/c/1303180Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: rajendrant <rajendrant@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603960}

chrome/browser/browser_process.h

@@ -61,10 +61,6 @@ class ComponentUpdateService;
 class SupervisedUserWhitelistInstaller;
 }
 
-namespace data_use_measurement {
-class ChromeDataUseMeasurement;
-}
-
 namespace extensions {
 class EventRouterForwarder;
 }
@@ -293,9 +289,6 @@ class BrowserProcess {
 
   virtual prefs::InProcessPrefServiceFactory* pref_service_factory() const = 0;
 
-  virtual data_use_measurement::ChromeDataUseMeasurement*
-  data_use_measurement() = 0;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(BrowserProcess);
 };

chrome/browser/browser_process_impl.cc

@@ -41,7 +41,6 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/component_updater/chrome_component_updater_configurator.h"
 #include "chrome/browser/component_updater/supervised_user_whitelist_installer.h"
-#include "chrome/browser/data_use_measurement/chrome_data_use_measurement.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/devtools/devtools_auto_opener.h"
 #include "chrome/browser/devtools/remote_debugging_server.h"
@@ -923,16 +922,6 @@ prefs::InProcessPrefServiceFactory* BrowserProcessImpl::pref_service_factory()
   return pref_service_factory_.get();
 }
 
-data_use_measurement::ChromeDataUseMeasurement*
-BrowserProcessImpl::data_use_measurement() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!data_use_measurement_) {
-    data_use_measurement_ = data_use_measurement::ChromeDataUseMeasurement::
-        CreateForNetworkService();
-  }
-  return data_use_measurement_.get();
-}
-
 // static
 void BrowserProcessImpl::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterBooleanPref(prefs::kDefaultBrowserSettingEnabled,

chrome/browser/browser_process_impl.h

@@ -199,8 +199,6 @@ class BrowserProcessImpl : public BrowserProcess,
   shell_integration::DefaultWebClientState CachedDefaultWebClientState()
       override;
   prefs::InProcessPrefServiceFactory* pref_service_factory() const override;
-  data_use_measurement::ChromeDataUseMeasurement* data_use_measurement()
-      override;
 
   static void RegisterPrefs(PrefRegistrySimple* registry);
 
@@ -431,9 +429,6 @@ class BrowserProcessImpl : public BrowserProcess,
   base::OnceClosure quit_closure_;
 #endif
 
-  std::unique_ptr<data_use_measurement::ChromeDataUseMeasurement>
-      data_use_measurement_;
-
   SEQUENCE_CHECKER(sequence_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(BrowserProcessImpl);

chrome/browser/chrome_content_browser_client.cc

@@ -4951,9 +4951,10 @@ void ChromeContentBrowserClient::OnNetworkServiceDataUseUpdate(
     int32_t network_traffic_annotation_id_hash,
     int64_t recv_bytes,
     int64_t sent_bytes) {
-  if (g_browser_process->data_use_measurement()) {
-    g_browser_process->data_use_measurement()->ReportNetworkServiceDataUse(
-        network_traffic_annotation_id_hash, recv_bytes, sent_bytes);
+  if (data_use_measurement::ChromeDataUseMeasurement::GetInstance()) {
+    data_use_measurement::ChromeDataUseMeasurement::GetInstance()
+        ->ReportNetworkServiceDataUse(network_traffic_annotation_id_hash,
+                                      recv_bytes, sent_bytes);
   }
 }
 

chrome/browser/data_use_measurement/chrome_data_use_measurement.cc

@@ -21,9 +21,15 @@
 #include "content/public/browser/network_service_instance.h"
 #include "services/network/public/cpp/features.h"
 
+using content::BrowserThread;
+
 namespace data_use_measurement {
 
 namespace {
+// Global instance to be used when network service is enabled, this will never
+// be deleted. When network service is disabled, this should always be null.
+ChromeDataUseMeasurement* g_chrome_data_use_measurement = nullptr;
+
 void UpdateMetricsUsagePrefs(int64_t total_bytes,
                              bool is_cellular,
                              bool is_metrics_service_usage) {
@@ -54,17 +60,20 @@ void UpdateMetricsUsagePrefsOnUIThread(int64_t total_bytes,
 }  // namespace
 
 // static
-std::unique_ptr<ChromeDataUseMeasurement>
-ChromeDataUseMeasurement::CreateForNetworkService() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+ChromeDataUseMeasurement* ChromeDataUseMeasurement::GetInstance() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
+         !BrowserThread::IsThreadInitialized(BrowserThread::UI));
 
   // Do not create when NetworkService is disabled, since data use of URLLoader
   // is reported via the network delegate callbacks.
   if (!base::FeatureList::IsEnabled(network::features::kNetworkService))
     return nullptr;
 
-  return std::make_unique<ChromeDataUseMeasurement>(
-      nullptr, nullptr, content::GetNetworkConnectionTracker());
+  if (!g_chrome_data_use_measurement) {
+    g_chrome_data_use_measurement = new ChromeDataUseMeasurement(
+        nullptr, nullptr, content::GetNetworkConnectionTracker());
+  }
+  return g_chrome_data_use_measurement;
 }
 
 ChromeDataUseMeasurement::ChromeDataUseMeasurement(
@@ -93,7 +102,6 @@ void ChromeDataUseMeasurement::ReportNetworkServiceDataUse(
     int64_t recv_bytes,
     int64_t sent_bytes) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(base::FeatureList::IsEnabled(network::features::kNetworkService));
   // Negative byte numbres is not a critical problem (i.e. should have no security implications) but
   // is not expected. TODO(rajendrant): remove these DCHECKs or consider using uint in Mojo instead.

chrome/browser/data_use_measurement/chrome_data_use_measurement.h

@@ -17,7 +17,7 @@ class DataUseAscriber;
 
 class ChromeDataUseMeasurement : public DataUseMeasurement {
  public:
-  static std::unique_ptr<ChromeDataUseMeasurement> CreateForNetworkService();
+  static ChromeDataUseMeasurement* GetInstance();
 
   ChromeDataUseMeasurement(
       std::unique_ptr<URLRequestClassifier> url_request_classifier,

chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc

@@ -226,8 +226,9 @@ void DataReductionProxyChromeSettings::InitDataReductionProxySettings(
               url_loader_factory, ui_task_runner),
           g_browser_process->network_quality_tracker(),
           content::GetNetworkConnectionTracker(),
-          g_browser_process->data_use_measurement(), ui_task_runner,
-          io_data->io_task_runner(), db_task_runner, commit_delay);
+          data_use_measurement::ChromeDataUseMeasurement::GetInstance(),
+          ui_task_runner, io_data->io_task_runner(), db_task_runner,
+          commit_delay);
   data_reduction_proxy::DataReductionProxySettings::
       InitDataReductionProxySettings(data_reduction_proxy_enabled_pref_name_,
                                      profile_prefs, io_data,

chrome/test/base/testing_browser_process.cc

@@ -452,11 +452,6 @@ TestingBrowserProcess::pref_service_factory() const {
   return nullptr;
 }
 
-data_use_measurement::ChromeDataUseMeasurement*
-TestingBrowserProcess::data_use_measurement() {
-  return nullptr;
-}
-
 void TestingBrowserProcess::SetSystemRequestContext(
     net::URLRequestContextGetter* context_getter) {
   system_request_context_ = context_getter;

chrome/test/base/testing_browser_process.h

@@ -137,8 +137,6 @@ class TestingBrowserProcess : public BrowserProcess {
   shell_integration::DefaultWebClientState CachedDefaultWebClientState()
       override;
   prefs::InProcessPrefServiceFactory* pref_service_factory() const override;
-  data_use_measurement::ChromeDataUseMeasurement* data_use_measurement()
-      override;
 
   // Set the local state for tests. Consumer is responsible for cleaning it up
   // afterwards (using ScopedTestingLocalState, for example).