Make NetLog officially a global leaked object.

This allows NetLog to safely be used from worker threads that are not
joined on shutdown.

It already was a leaked object in Chromium and in Cronet on Android (but
not on Cronet iOS).

Moves ownership of the global object into net_log.cc and adds
NetLog::Get() to access it, and makes ~NetLog destructor private so that
code can't accidentally create local NetLog objects.

Tests can create a local NetLog with the TestNetLog class. Tests that
only needed an NetLog incidentally but don't actually observe/test it
are changed to use the global NetLog::Get().

Bug: 177538
Change-Id: I12a832801c108483db5b1a577c1094e7d2ceb3f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1912696
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Commit-Queue: Matt Mueller <mattm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719717}

components/cronet/cronet_url_request_context.cc

@@ -68,9 +68,9 @@ namespace {
 // This class wraps a NetLog that also contains network change events.
 class NetLogWithNetworkChangeEvents {
  public:
-  NetLogWithNetworkChangeEvents() {}
+  NetLogWithNetworkChangeEvents() : net_log_(net::NetLog::Get()) {}
 
-  net::NetLog* net_log() { return &net_log_; }
+  net::NetLog* net_log() { return net_log_; }
   // This function registers with the NetworkChangeNotifier and so must be
   // called *after* the NetworkChangeNotifier is created. Should only be
   // called on the init thread as it is not thread-safe and the init thread is
@@ -85,11 +85,11 @@ class NetLogWithNetworkChangeEvents {
     DCHECK(cronet::OnInitThread());
     if (net_change_logger_)
       return;
-    net_change_logger_.reset(new net::LoggingNetworkChangeObserver(&net_log_));
+    net_change_logger_.reset(new net::LoggingNetworkChangeObserver(net_log_));
   }
 
  private:
-  net::NetLog net_log_;
+  net::NetLog* net_log_;
   // LoggingNetworkChangeObserver logs network change events to a NetLog.
   // This class bundles one LoggingNetworkChangeObserver with one NetLog,
   // so network change event are logged just once in the NetLog.
@@ -288,8 +288,7 @@ void CronetURLRequestContext::NetworkTasks::Initialize(
       cronet::CreateProxyResolutionService(std::move(proxy_config_service),
                                            g_net_log.Get().net_log()));
 
-  config->ConfigureURLRequestContextBuilder(&context_builder,
-                                            g_net_log.Get().net_log());
+  config->ConfigureURLRequestContextBuilder(&context_builder);
   effective_experimental_options_ =
       std::move(config->effective_experimental_options);
 

components/cronet/ios/cronet_environment.h

@@ -200,7 +200,7 @@ class CronetEnvironment {
   scoped_refptr<net::URLRequestContextGetter> main_context_getter_;
   std::string user_agent_;
   bool user_agent_partial_;
-  std::unique_ptr<net::NetLog> net_log_;
+  net::NetLog* net_log_;
   std::unique_ptr<net::FileNetLogObserver> file_net_log_observer_;
   bool enable_pkp_bypass_for_local_trust_anchors_;
   double network_thread_priority_;

components/cronet/ios/cronet_environment.mm

@@ -243,7 +243,7 @@ bool IsNetLogPathValid(const base::FilePath& path) {
       http_cache_(URLRequestContextConfig::HttpCacheType::DISK),
       user_agent_(user_agent),
       user_agent_partial_(user_agent_partial),
-      net_log_(new net::NetLog),
+      net_log_(net::NetLog::Get()),
       enable_pkp_bypass_for_local_trust_anchors_(true),
       network_thread_priority_(kKeepDefaultThreadPriority) {}
 
@@ -354,7 +354,7 @@ bool IsNetLogPathValid(const base::FilePath& path) {
   // future.
   context_builder.set_transport_security_persister_path(base::FilePath());
 
-  config->ConfigureURLRequestContextBuilder(&context_builder, net_log_.get());
+  config->ConfigureURLRequestContextBuilder(&context_builder);
 
   effective_experimental_options_ =
       std::move(config->effective_experimental_options);
@@ -369,7 +369,7 @@ bool IsNetLogPathValid(const base::FilePath& path) {
     cronet_prefs_manager_ = std::make_unique<CronetPrefsManager>(
         config->storage_path, GetNetworkThreadTaskRunner(),
         file_thread_->task_runner(), false /* nqe */, false /* host_cache */,
-        net_log_.get(), &context_builder);
+        net_log_, &context_builder);
   }
 
   context_builder.set_host_resolver(std::move(mapped_host_resolver));

components/cronet/stale_host_resolver_unittest.cc

@@ -40,7 +40,6 @@
 #include "net/dns/public/dns_protocol.h"
 #include "net/dns/public/dns_query_type.h"
 #include "net/http/http_network_session.h"
-#include "net/log/net_log.h"
 #include "net/log/net_log_with_source.h"
 #include "net/proxy_resolution/proxy_config.h"
 #include "net/proxy_resolution/proxy_config_service_fixed.h"
@@ -698,8 +697,7 @@ TEST_F(StaleHostResolverTest, CreatedByContext) {
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(

components/cronet/url_request_context_config.cc

@@ -29,6 +29,7 @@
 #include "net/dns/mapped_host_resolver.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_server_properties.h"
+#include "net/log/net_log.h"
 #include "net/nqe/network_quality_estimator_params.h"
 #include "net/quic/quic_utils_chromium.h"
 #include "net/reporting/reporting_policy.h"
@@ -292,13 +293,10 @@ URLRequestContextConfig::~URLRequestContextConfig() {}
 
 void URLRequestContextConfig::ParseAndSetExperimentalOptions(
     net::URLRequestContextBuilder* context_builder,
-    net::HttpNetworkSession::Params* session_params,
-    net::NetLog* net_log) {
+    net::HttpNetworkSession::Params* session_params) {
   if (experimental_options.empty())
     return;
 
-  DCHECK(net_log);
-
   DVLOG(1) << "Experimental Options:" << experimental_options;
   std::unique_ptr<base::Value> options =
       base::JSONReader::ReadDeprecated(experimental_options);
@@ -699,7 +697,6 @@ void URLRequestContextConfig::ParseAndSetExperimentalOptions(
 
   if (async_dns_enable || stale_dns_enable || host_resolver_rules_enable ||
       disable_ipv6_on_wifi) {
-    CHECK(net_log) << "All DNS-related experiments require NetLog.";
     std::unique_ptr<net::HostResolver> host_resolver;
     net::HostResolver::ManagerOptions host_resolver_manager_options;
     host_resolver_manager_options.insecure_dns_client_enabled =
@@ -711,11 +708,11 @@ void URLRequestContextConfig::ParseAndSetExperimentalOptions(
       DCHECK(!disable_ipv6_on_wifi);
       host_resolver.reset(new StaleHostResolver(
           net::HostResolver::CreateStandaloneContextResolver(
-              net_log, std::move(host_resolver_manager_options)),
+              net::NetLog::Get(), std::move(host_resolver_manager_options)),
           stale_dns_options));
     } else {
       host_resolver = net::HostResolver::CreateStandaloneResolver(
-          net_log, std::move(host_resolver_manager_options));
+          net::NetLog::Get(), std::move(host_resolver_manager_options));
     }
     if (host_resolver_rules_enable) {
       std::unique_ptr<net::MappedHostResolver> remapped_resolver(
@@ -745,10 +742,7 @@ void URLRequestContextConfig::ParseAndSetExperimentalOptions(
 }
 
 void URLRequestContextConfig::ConfigureURLRequestContextBuilder(
-    net::URLRequestContextBuilder* context_builder,
-    net::NetLog* net_log) {
-  DCHECK(net_log);
-
+    net::URLRequestContextBuilder* context_builder) {
   std::string config_cache;
   if (http_cache != DISABLED) {
     net::URLRequestContextBuilder::HttpCacheParams cache_params;
@@ -778,7 +772,7 @@ void URLRequestContextConfig::ConfigureURLRequestContextBuilder(
         kDefaultQuicGoAwaySessionsOnIpChange;
   }
 
-  ParseAndSetExperimentalOptions(context_builder, &session_params, net_log);
+  ParseAndSetExperimentalOptions(context_builder, &session_params);
   context_builder->set_http_network_session_params(session_params);
 
   if (mock_cert_verifier)

components/cronet/url_request_context_config.h

@@ -21,7 +21,6 @@
 
 namespace net {
 class CertVerifier;
-class NetLog;
 class URLRequestContextBuilder;
 }  // namespace net
 
@@ -133,8 +132,7 @@ struct URLRequestContextConfig {
 
   // Configures |context_builder| based on |this|.
   void ConfigureURLRequestContextBuilder(
-      net::URLRequestContextBuilder* context_builder,
-      net::NetLog* net_log);
+      net::URLRequestContextBuilder* context_builder);
 
   // Enable QUIC.
   const bool enable_quic;
@@ -205,8 +203,7 @@ struct URLRequestContextConfig {
   // the URLRequestContextConfig and URLRequestContextBuilder.
   void ParseAndSetExperimentalOptions(
       net::URLRequestContextBuilder* context_builder,
-      net::HttpNetworkSession::Params* session_params,
-      net::NetLog* net_log);
+      net::HttpNetworkSession::Params* session_params);
 
   // Experimental options encoded as a string in a JSON format containing
   // experiments and their corresponding configuration options. The format

components/cronet/url_request_context_config_unittest.cc

@@ -21,7 +21,6 @@
 #include "net/dns/host_resolver.h"
 #include "net/dns/host_resolver_manager.h"
 #include "net/http/http_network_session.h"
-#include "net/log/net_log.h"
 #include "net/log/net_log_with_source.h"
 #include "net/proxy_resolution/proxy_config.h"
 #include "net/proxy_resolution/proxy_config_service_fixed.h"
@@ -182,8 +181,7 @@ TEST(URLRequestContextConfigTest, TestExperimentalOptionParsing) {
       base::Optional<double>(42.0));
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   EXPECT_FALSE(config.effective_experimental_options->HasKey("UnknownOption"));
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
@@ -339,8 +337,7 @@ TEST(URLRequestContextConfigTest, SetSupportedQuicVersion) {
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(
@@ -392,8 +389,7 @@ TEST(URLRequestContextConfigTest, SetUnsupportedQuicVersion) {
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(
@@ -445,8 +441,7 @@ TEST(URLRequestContextConfigTest, SetQuicServerMigrationOptions) {
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(
@@ -506,8 +501,7 @@ TEST(URLRequestContextConfigTest,
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(
@@ -568,8 +562,7 @@ TEST(URLRequestContextConfigTest,
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(
@@ -630,8 +623,7 @@ TEST(URLRequestContextConfigTest,
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(
@@ -693,8 +685,7 @@ TEST(URLRequestContextConfigTest, SetQuicConnectionMigrationV2Options) {
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(
@@ -759,8 +750,7 @@ TEST(URLRequestContextConfigTest, SetQuicStaleDNSracing) {
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(
@@ -810,8 +800,7 @@ TEST(URLRequestContextConfigTest, SetQuicGoawayOnPathDegrading) {
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(
@@ -861,8 +850,7 @@ TEST(URLRequestContextConfigTest, SetQuicHostWhitelist) {
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(
@@ -914,8 +902,7 @@ TEST(URLRequestContextConfigTest, SetQuicMaxTimeBeforeCryptoHandshake) {
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(
@@ -970,8 +957,7 @@ TEST(URLURLRequestContextConfigTest, SetQuicConnectionOptions) {
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(
@@ -1031,8 +1017,7 @@ TEST(URLURLRequestContextConfigTest, SetAcceptLanguageAndUserAgent) {
       base::Optional<double>());
 
   net::URLRequestContextBuilder builder;
-  net::NetLog net_log;
-  config.ConfigureURLRequestContextBuilder(&builder, &net_log);
+  config.ConfigureURLRequestContextBuilder(&builder);
   // Set a ProxyConfigService to avoid DCHECK failure when building.
   builder.set_proxy_config_service(
       std::make_unique<net::ProxyConfigServiceFixed>(

fuchsia/engine/BUILD.gn

@@ -152,8 +152,8 @@ component("web_engine_core") {
     "browser/web_engine_devtools_controller.h",
     "browser/web_engine_memory_pressure_evaluator.cc",
     "browser/web_engine_memory_pressure_evaluator.h",
-    "browser/web_engine_net_log.cc",
-    "browser/web_engine_net_log.h",
+    "browser/web_engine_net_log_observer.cc",
+    "browser/web_engine_net_log_observer.h",
     "browser/web_engine_permission_manager.cc",
     "browser/web_engine_permission_manager.h",
     "common/web_engine_content_client.cc",

fuchsia/engine/browser/web_engine_browser_context.cc

@@ -17,7 +17,7 @@
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/resource_context.h"
-#include "fuchsia/engine/browser/web_engine_net_log.h"
+#include "fuchsia/engine/browser/web_engine_net_log_observer.h"
 #include "fuchsia/engine/browser/web_engine_permission_manager.h"
 #include "services/network/public/cpp/network_switches.h"
 
@@ -31,22 +31,23 @@ class WebEngineBrowserContext::ResourceContext
   DISALLOW_COPY_AND_ASSIGN(ResourceContext);
 };
 
-std::unique_ptr<WebEngineNetLog> CreateNetLog() {
-  std::unique_ptr<WebEngineNetLog> result;
+std::unique_ptr<WebEngineNetLogObserver> CreateNetLogObserver() {
+  std::unique_ptr<WebEngineNetLogObserver> result;
 
   const base::CommandLine* command_line =
       base::CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(network::switches::kLogNetLog)) {
     base::FilePath log_path =
         command_line->GetSwitchValuePath(network::switches::kLogNetLog);
-    result = std::make_unique<WebEngineNetLog>(log_path);
+    result = std::make_unique<WebEngineNetLogObserver>(log_path);
   }
 
   return result;
 }
 
 WebEngineBrowserContext::WebEngineBrowserContext(bool force_incognito)
-    : net_log_(CreateNetLog()), resource_context_(new ResourceContext()) {
+    : net_log_observer_(CreateNetLogObserver()),
+      resource_context_(new ResourceContext()) {
   if (!force_incognito) {
     base::PathService::Get(base::DIR_APP_DATA, &data_dir_path_);
     if (!base::PathExists(data_dir_path_)) {

fuchsia/engine/browser/web_engine_browser_context.h

@@ -12,7 +12,7 @@
 #include "components/keyed_service/core/simple_factory_key.h"
 #include "content/public/browser/browser_context.h"
 
-class WebEngineNetLog;
+class WebEngineNetLogObserver;
 class WebEnginePermissionManager;
 
 class WebEngineBrowserContext : public content::BrowserContext {
@@ -49,7 +49,7 @@ class WebEngineBrowserContext : public content::BrowserContext {
 
   base::FilePath data_dir_path_;
 
-  std::unique_ptr<WebEngineNetLog> net_log_;
+  std::unique_ptr<WebEngineNetLogObserver> net_log_observer_;
   std::unique_ptr<SimpleFactoryKey> simple_factory_key_;
   std::unique_ptr<ResourceContext> resource_context_;
   std::unique_ptr<WebEnginePermissionManager> permission_manager_;