Make the NetworkService use Chrome's NetLog when created in-process.

This will let us start adding NetLog APIs to the network service, and
using them in production, without breaking logging. Eventually we'll
want to get rid of Chrome's NetLog entirely, but doing that all at once
would be too much for a single CL.

Bug: 767450
Change-Id: I1ce00dded81f83ce3e7d1e498f72d991a68d3e66
Reviewed-on: https://chromium-review.googlesource.com/723819Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509947}

chrome/browser/io_thread.cc

@@ -66,7 +66,6 @@
 #include "content/public/common/user_agent.h"
 #include "content/public/network/url_request_context_builder_mojo.h"
 #include "extensions/features/features.h"
-#include "net/base/logging_network_change_observer.h"
 #include "net/cert/caching_cert_verifier.h"
 #include "net/cert/cert_verifier.h"
 #include "net/cert/cert_verify_proc.h"
@@ -458,12 +457,6 @@ void IOThread::Init() {
   DCHECK(!globals_);
   globals_ = new Globals;
 
-  // Add an observer that will emit network change events to the ChromeNetLog.
-  // Assuming NetworkChangeNotifier dispatches in FIFO order, we should be
-  // logging the network change before other IO thread consumers respond to it.
-  network_change_observer_.reset(
-      new net::LoggingNetworkChangeObserver(net_log_));
-
   // Setup the HistogramWatcher to run on the IO thread.
   net::NetworkChangeNotifier::InitHistogramWatcher();
 
@@ -595,9 +588,6 @@ void IOThread::CleanUp() {
   // Shutdown the HistogramWatcher on the IO thread.
   net::NetworkChangeNotifier::ShutdownHistogramWatcher();
 
-  // This must be reset before the ChromeNetLog is destroyed.
-  network_change_observer_.reset();
-
   system_proxy_config_service_.reset();
   delete globals_;
   globals_ = NULL;
@@ -789,7 +779,6 @@ void IOThread::ConstructSystemRequestContext() {
   builder->set_network_delegate(
       globals_->data_use_ascriber->CreateNetworkDelegate(
           std::move(chrome_network_delegate), GetMetricsDataUseForwarder()));
-  builder->set_net_log(net_log_);
   std::unique_ptr<net::HostResolver> host_resolver(
       CreateGlobalHostResolver(net_log_));
 
@@ -830,7 +819,7 @@ void IOThread::ConstructSystemRequestContext() {
   SetUpProxyConfigService(builder.get(),
                           std::move(system_proxy_config_service_));
 
-  globals_->network_service = content::NetworkService::Create();
+  globals_->network_service = content::NetworkService::Create(net_log_);
   if (!is_quic_allowed_on_init_)
     globals_->network_service->DisableQuic();
 

chrome/browser/io_thread.h

@@ -77,7 +77,6 @@ class CTLogVerifier;
 class HostResolver;
 class HttpAuthHandlerFactory;
 class HttpAuthPreferences;
-class LoggingNetworkChangeObserver;
 class NetworkQualityEstimator;
 class ProxyConfigService;
 class RTTAndThroughputEstimatesObserver;
@@ -282,9 +281,6 @@ class IOThread : public content::BrowserThreadDelegate {
 
   Globals* globals_;
 
-  // Observer that logs network changes to the ChromeNetLog.
-  std::unique_ptr<net::LoggingNetworkChangeObserver> network_change_observer_;
-
   std::unique_ptr<certificate_transparency::TreeStateTracker> ct_tree_tracker_;
 
   BooleanPrefMember system_enable_referrers_;

chrome/browser/profiles/profile_io_data.cc

@@ -1009,7 +1009,6 @@ void ProfileIOData::Init(
   std::unique_ptr<content::URLRequestContextBuilderMojo> builder =
       base::MakeUnique<content::URLRequestContextBuilderMojo>();
 
-  builder->set_net_log(io_thread->net_log());
   builder->set_shared_http_user_agent_settings(
       chrome_http_user_agent_settings_.get());
   builder->set_ssl_config_service(profile_params_->ssl_config_service);

content/network/network_context.cc

@@ -238,7 +238,7 @@ void NetworkContext::ApplyContextParamsToBuilder(
     net::URLRequestContextBuilder* builder,
     mojom::NetworkContextParams* network_context_params) {
   // |network_service_| may be nullptr in tests.
-  if (!builder->net_log() && network_service_)
+  if (network_service_)
     builder->set_net_log(network_service_->net_log());
 
   builder->set_enable_brotli(network_context_params->enable_brotli);
@@ -279,7 +279,7 @@ void NetworkContext::ApplyContextParamsToBuilder(
         std::make_unique<net::HttpServerPropertiesManager>(
             std::make_unique<HttpServerPropertiesPrefDelegate>(
                 pref_service_.get()),
-            builder->net_log()));
+            network_service_->net_log()));
   }
 
   builder->set_data_enabled(network_context_params->enable_data_url_support);

content/network/network_service_impl.cc

@@ -12,14 +12,16 @@
 #include "content/network/network_context.h"
 #include "content/public/common/content_switches.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
+#include "net/base/logging_network_change_observer.h"
 #include "net/log/file_net_log_observer.h"
+#include "net/log/net_log.h"
 #include "net/log/net_log_util.h"
 #include "net/url_request/url_request_context_builder.h"
 
 namespace content {
 
-std::unique_ptr<NetworkService> NetworkService::Create() {
-  return base::MakeUnique<NetworkServiceImpl>(nullptr);
+std::unique_ptr<NetworkService> NetworkService::Create(net::NetLog* net_log) {
+  return base::MakeUnique<NetworkServiceImpl>(nullptr, net_log);
 }
 
 class NetworkServiceImpl::MojoNetLog : public net::NetLog {
@@ -55,8 +57,9 @@ class NetworkServiceImpl::MojoNetLog : public net::NetLog {
 };
 
 NetworkServiceImpl::NetworkServiceImpl(
-    std::unique_ptr<service_manager::BinderRegistry> registry)
-    : net_log_(new MojoNetLog), registry_(std::move(registry)), binding_(this) {
+    std::unique_ptr<service_manager::BinderRegistry> registry,
+    net::NetLog* net_log)
+    : registry_(std::move(registry)), binding_(this) {
   // |registry_| is nullptr when an in-process NetworkService is
   // created directly. The latter is done in concert with using
   // CreateNetworkContextWithBuilder to ease the transition to using the network
@@ -64,12 +67,24 @@ NetworkServiceImpl::NetworkServiceImpl(
   if (registry_) {
     registry_->AddInterface<mojom::NetworkService>(
         base::Bind(&NetworkServiceImpl::Create, base::Unretained(this)));
+  }
 
-    // Note: The command line switches are only checked when running out of
-    // process, since in in-process mode other code may already be writing to
-    // the destination log file.
-    net_log_->ProcessCommandLine(*base::CommandLine::ForCurrentProcess());
+  if (net_log) {
+    net_log_ = net_log;
+  } else {
+    owned_net_log_ = std::make_unique<MojoNetLog>();
+    // Note: The command line switches are only checked when not using the
+    // embedder's NetLog, as it may already be writing to the destination log
+    // file.
+    owned_net_log_->ProcessCommandLine(*base::CommandLine::ForCurrentProcess());
+    net_log_ = owned_net_log_.get();
   }
+
+  // Add an observer that will emit network change events to the ChromeNetLog.
+  // Assuming NetworkChangeNotifier dispatches in FIFO order, we should be
+  // logging the network change before other IO thread consumers respond to it.
+  network_change_observer_.reset(
+      new net::LoggingNetworkChangeObserver(net_log_));
 }
 
 NetworkServiceImpl::~NetworkServiceImpl() {
@@ -143,7 +158,7 @@ bool NetworkServiceImpl::HasRawHeadersAccess(uint32_t process_id) const {
 }
 
 net::NetLog* NetworkServiceImpl::net_log() const {
-  return net_log_.get();
+  return net_log_;
 }
 
 void NetworkServiceImpl::OnBindInterface(

content/network/network_service_impl.h

@@ -16,6 +16,8 @@
 #include "services/service_manager/public/cpp/service.h"
 
 namespace net {
+class NetLog;
+class LoggingNetworkChangeObserver;
 class URLRequestContext;
 class URLRequestContextBuilder;
 }  // namespace net
@@ -27,8 +29,13 @@ class NetworkContext;
 class CONTENT_EXPORT NetworkServiceImpl : public service_manager::Service,
                                           public NetworkService {
  public:
-  explicit NetworkServiceImpl(
-      std::unique_ptr<service_manager::BinderRegistry> registry);
+  // |net_log| is an optional shared NetLog, which will be used instead of the
+  // service's own NetLog. It must outlive the NetworkService.
+  //
+  // TODO(https://crbug.com/767450): Once the NetworkService can always create
+  // its own NetLog in production, remove the |net_log| argument.
+  NetworkServiceImpl(std::unique_ptr<service_manager::BinderRegistry> registry,
+                     net::NetLog* net_log = nullptr);
 
   ~NetworkServiceImpl() override;
 
@@ -68,7 +75,15 @@ class CONTENT_EXPORT NetworkServiceImpl : public service_manager::Service,
 
   void Create(mojom::NetworkServiceRequest request);
 
-  std::unique_ptr<MojoNetLog> net_log_;
+  std::unique_ptr<MojoNetLog> owned_net_log_;
+  // TODO(https://crbug.com/767450): Remove this, once Chrome no longer creates
+  // its own NetLog.
+  net::NetLog* net_log_;
+
+  // Observer that logs network changes to the NetLog. Must be below the NetLog
+  // and the NetworkChangeNotifier (Once this class creates it), so it's
+  // destroyed before them.
+  std::unique_ptr<net::LoggingNetworkChangeObserver> network_change_observer_;
 
   std::unique_ptr<service_manager::BinderRegistry> registry_;
 

content/public/network/network_service.h

@@ -11,6 +11,7 @@
 #include "content/public/common/network_service.mojom.h"
 
 namespace net {
+class NetLog;
 class URLRequestContext;
 class URLRequestContextBuilder;
 }  // namespace net
@@ -20,7 +21,13 @@ namespace content {
 // Allows an in-process NetworkService to be set up.
 class CONTENT_EXPORT NetworkService : public mojom::NetworkService {
  public:
-  static std::unique_ptr<NetworkService> Create();
+  // Creates a NetworkService instance on the current thread, optionally using
+  // the passed-in NetLog. Does not take ownership of |net_log|. Must be
+  // destroyed before |net_log|.
+  //
+  // TODO(https://crbug.com/767450): Make it so NetworkService can always create
+  // its own NetLog, instead of sharing one.
+  static std::unique_ptr<NetworkService> Create(net::NetLog* net_log = nullptr);
 
   // Can be used to seed a NetworkContext with a consumer-configured
   // URLRequestContextBuilder, which |params| will then be applied to. The

net/url_request/url_request_context_builder.h

@@ -351,11 +351,6 @@ class NET_EXPORT URLRequestContextBuilder {
       CreateHttpTransactionFactoryCallback
           create_http_network_transaction_factory);
 
-  // Returns the previous value passed to set_net_log(), or nullptr if it hasn't
-  // been called yet.
-  // TODO(mmenke): Remove this method.
-  net::NetLog* net_log() const { return net_log_; }
-
   // Creates a mostly self-contained URLRequestContext. May only be called once
   // per URLRequestContextBuilder. After this is called, the Builder can be
   // safely destroyed.