Enhance configuration of the NetworkService-owned NetLog

This CL moves code related to opening files out of the NetworkService
(which will eventually be sandboxed), as well as factoring out some of
the shared features of the NetLog subclasses, to make code deduplication
easier.

Implementations are provided for ChromeContentClient and
ShellContentClient classes.

Bug: 847961
Change-Id: I0a332b4429a2f20cbb38ae1671b8df0fbee28ba1
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1161470
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581201}

chrome/common/BUILD.gn

@@ -242,6 +242,7 @@ static_library("common") {
     "//components/nacl/common:buildflags",
     "//components/nacl/common:process_type",
     "//components/nacl/common:switches",
+    "//components/net_log",
     "//components/network_session_configurator/common",
     "//components/ntp_tiles",
     "//components/offline_pages/buildflags",

chrome/common/DEPS

@@ -26,6 +26,7 @@ include_rules = [
   "+components/metrics/client_info.h",
   "+components/metrics/metrics_pref_names.h",
   "+components/nacl/common",
+  "+components/net_log",
   "+components/network_session_configurator/common",
   "+components/ntp_tiles",
   "+components/nux_google_apps",

chrome/common/chrome_content_client.cc

@@ -26,6 +26,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/version.h"
 #include "build/build_config.h"
+#include "chrome/common/channel_info.h"
 #include "chrome/common/child_process_logging.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
@@ -37,6 +38,7 @@
 #include "chrome/grit/common_resources.h"
 #include "components/crash/core/common/crash_key.h"
 #include "components/dom_distiller/core/url_constants.h"
+#include "components/net_log/chrome_net_log.h"
 #include "components/services/heap_profiling/public/cpp/client.h"
 #include "components/version_info/version_info.h"
 #include "content/public/common/cdm_info.h"
@@ -679,6 +681,16 @@ gfx::Image& ChromeContentClient::GetNativeImageNamed(int resource_id) const {
       resource_id);
 }
 
+base::DictionaryValue ChromeContentClient::GetNetLogConstants() const {
+  auto platform_dict = net_log::ChromeNetLog::GetPlatformConstants(
+      base::CommandLine::ForCurrentProcess()->GetCommandLineString(),
+      chrome::GetChannelName());
+  if (platform_dict)
+    return std::move(*platform_dict);
+  else
+    return base::DictionaryValue();
+}
+
 std::string ChromeContentClient::GetProcessTypeNameInEnglish(int type) {
 #if BUILDFLAG(ENABLE_NACL)
   switch (type) {

chrome/common/chrome_content_client.h

@@ -87,6 +87,7 @@ class ChromeContentClient : public content::ContentClient {
   base::RefCountedMemory* GetDataResourceBytes(
       int resource_id) const override;
   gfx::Image& GetNativeImageNamed(int resource_id) const override;
+  base::DictionaryValue GetNetLogConstants() const override;
   std::string GetProcessTypeNameInEnglish(int type) override;
 
   bool AllowScriptExtensionForServiceWorker(const GURL& script_url) override;

content/browser/network_service_instance.cc

@@ -10,8 +10,10 @@
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/common/service_manager_connection.h"
 #include "content/public/common/service_names.mojom.h"
+#include "net/log/net_log_util.h"
 #include "services/network/network_service.h"
 #include "services/network/public/cpp/features.h"
+#include "services/network/public/cpp/network_switches.h"
 #include "services/service_manager/public/cpp/connector.h"
 
 namespace content {
@@ -59,6 +61,25 @@ network::mojom::NetworkService* GetNetworkService() {
     g_client = new NetworkServiceClient(mojo::MakeRequest(&client_ptr));
     (*g_network_service_ptr)->SetClient(std::move(client_ptr));
 
+    if (base::FeatureList::IsEnabled(network::features::kNetworkService)) {
+      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);
+
+        base::DictionaryValue client_constants =
+            GetContentClient()->GetNetLogConstants();
+
+        base::File file(
+            log_path, base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
+        LOG_IF(ERROR, !file.IsValid())
+            << "Failed opening: " << log_path.value();
+        (*g_network_service_ptr)
+            ->StartNetLog(std::move(file), std::move(client_constants));
+      }
+    }
+
     GetContentClient()->browser()->OnNetworkServiceCreated(
         g_network_service_ptr->get());
   }

content/public/common/content_client.cc

@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "base/strings/string_piece.h"
+#include "base/values.h"
 #include "build/build_config.h"
 #include "content/public/common/origin_util.h"
 #include "content/public/common/user_agent.h"
@@ -102,6 +103,10 @@ std::string ContentClient::GetProcessTypeNameInEnglish(int type) {
   return std::string();
 }
 
+base::DictionaryValue ContentClient::GetNetLogConstants() const {
+  return base::DictionaryValue();
+}
+
 blink::OriginTrialPolicy* ContentClient::GetOriginTrialPolicy() {
   return nullptr;
 }

content/public/common/content_client.h

@@ -20,6 +20,7 @@
 
 namespace base {
 class RefCountedMemory;
+class DictionaryValue;
 }
 
 namespace blink {
@@ -173,6 +174,14 @@ class CONTENT_EXPORT ContentClient {
   // doesn't know about because they're from the embedder.
   virtual std::string GetProcessTypeNameInEnglish(int type);
 
+  // Called once during initialization of NetworkService to provide constants
+  // to NetLog.  (Though it may be called multiples times if NetworkService
+  // crashes and needs to be reinitialized).  The return value is merged with
+  // |GetNetConstants()| and passed to FileNetLogObserver - see documentation
+  // of |FileNetLogObserver::CreateBounded()| for more information.  The
+  // convention is to put new constants under a subdict at the key "clientInfo".
+  virtual base::DictionaryValue GetNetLogConstants() const;
+
   // Returns whether or not V8 script extensions should be allowed for a
   // service worker.
   virtual bool AllowScriptExtensionForServiceWorker(const GURL& script_url);

content/shell/common/shell_content_client.cc

@@ -7,6 +7,7 @@
 #include "base/command_line.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
 #include "build/build_config.h"
 #include "content/app/resources/grit/content_resources.h"
 #include "content/app/strings/grit/content_strings.h"
@@ -87,6 +88,17 @@ gfx::Image& ShellContentClient::GetNativeImageNamed(int resource_id) const {
       resource_id);
 }
 
+base::DictionaryValue ShellContentClient::GetNetLogConstants() const {
+  base::DictionaryValue client_constants;
+  client_constants.SetString("name", "content_shell");
+  client_constants.SetString(
+      "command_line",
+      base::CommandLine::ForCurrentProcess()->GetCommandLineString());
+  base::DictionaryValue constants;
+  constants.SetKey("clientInfo", std::move(client_constants));
+  return constants;
+}
+
 blink::OriginTrialPolicy* ShellContentClient::GetOriginTrialPolicy() {
   return &origin_trial_policy_;
 }

content/shell/common/shell_content_client.h

@@ -29,6 +29,7 @@ class ShellContentClient : public ContentClient {
   base::RefCountedMemory* GetDataResourceBytes(
       int resource_id) const override;
   gfx::Image& GetNativeImageNamed(int resource_id) const override;
+  base::DictionaryValue GetNetLogConstants() const override;
   blink::OriginTrialPolicy* GetOriginTrialPolicy() override;
 
  private:

services/network/mojo_net_log.cc

@@ -29,19 +29,14 @@ MojoNetLog::~MojoNetLog() {
                                           base::OnceClosure());
 }
 
-void MojoNetLog::ProcessCommandLine(const base::CommandLine& command_line) {
-  if (!command_line.HasSwitch(switches::kLogNetLog))
-    return;
-
-  base::FilePath log_path =
-      command_line.GetSwitchValuePath(switches::kLogNetLog);
-
+void MojoNetLog::ObserveFileWithConstants(base::File file,
+                                          base::Value constants) {
   // TODO(eroman): Should get capture mode from the command line.
   net::NetLogCaptureMode capture_mode =
       net::NetLogCaptureMode::IncludeCookiesAndCredentials();
 
-  file_net_log_observer_ = net::FileNetLogObserver::CreateUnbounded(
-      log_path, nullptr /* constants */);
+  file_net_log_observer_ = net::FileNetLogObserver::CreateUnboundedPreExisting(
+      std::move(file), std::make_unique<base::Value>(std::move(constants)));
   file_net_log_observer_->StartObserving(this, capture_mode);
 }
 

services/network/mojo_net_log.h

@@ -12,10 +12,6 @@
 #include "net/log/net_log.h"
 #include "services/network/public/mojom/network_service.mojom.h"
 
-namespace base {
-class CommandLine;
-}  // namespace base
-
 namespace net {
 class FileNetLogObserver;
 }  // namespace net
@@ -35,7 +31,7 @@ class MojoNetLog : public net::NetLog {
 
   // If specified by the command line, stream network events (NetLog) to a
   // file on disk. This will last for the duration of the process.
-  void ProcessCommandLine(const base::CommandLine& command_line);
+  void ObserveFileWithConstants(base::File file, base::Value constants);
 
  private:
   std::unique_ptr<net::FileNetLogObserver> file_net_log_observer_;

services/network/network_service.cc

@@ -24,6 +24,7 @@
 #include "net/dns/host_resolver.h"
 #include "net/dns/mapped_host_resolver.h"
 #include "net/http/http_auth_handler_factory.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"
@@ -138,7 +139,6 @@ NetworkService::NetworkService(
     // 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(*command_line);
     net_log_ = owned_net_log_.get();
   }
 
@@ -237,6 +237,16 @@ void NetworkService::SetClient(mojom::NetworkServiceClientPtr client) {
   client_ = std::move(client);
 }
 
+void NetworkService::StartNetLog(base::File file,
+                                 base::Value client_constants) {
+  DCHECK(client_constants.is_dict());
+  std::unique_ptr<base::DictionaryValue> constants = net::GetNetConstants();
+  constants->MergeDictionary(&client_constants);
+
+  owned_net_log_->ObserveFileWithConstants(std::move(file),
+                                           std::move(*constants));
+}
+
 void NetworkService::CreateNetworkContext(
     mojom::NetworkContextRequest request,
     mojom::NetworkContextParamsPtr params) {

services/network/network_service.h

@@ -119,6 +119,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) NetworkService
 
   // mojom::NetworkService implementation:
   void SetClient(mojom::NetworkServiceClientPtr client) override;
+  void StartNetLog(base::File file, base::Value constants) override;
   void CreateNetworkContext(mojom::NetworkContextRequest request,
                             mojom::NetworkContextParamsPtr params) override;
   void ConfigureStubHostResolver(

services/network/network_service_unittest.cc

@@ -5,7 +5,10 @@
 #include <memory>
 #include <utility>
 
+#include "base/files/scoped_temp_dir.h"
+#include "base/json/json_file_value_serializer.h"
 #include "base/optional.h"
+#include "base/path_service.h"
 #include "base/run_loop.h"
 #include "base/strings/string_util.h"
 #include "base/test/scoped_task_environment.h"
@@ -729,6 +732,38 @@ TEST_F(NetworkServiceTestWithService, Basic) {
   EXPECT_EQ(net::OK, client()->completion_status().error_code);
 }
 
+// Verifies that a passed net log file is successfully opened and sane data
+// written to it.
+TEST_F(NetworkServiceTestWithService, StartsNetLog) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  base::FilePath log_dir = temp_dir.GetPath();
+  base::FilePath log_path = log_dir.Append(FILE_PATH_LITERAL("test_log.json"));
+
+  base::DictionaryValue dict;
+  dict.SetString("amiatest", "iamatest");
+
+  base::File log_file(log_path,
+                      base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
+  network_service_->StartNetLog(std::move(log_file), std::move(dict));
+  CreateNetworkContext();
+  LoadURL(test_server()->GetURL("/echo"));
+  EXPECT_EQ(net::OK, client()->completion_status().error_code);
+
+  // |log_file| is closed on destruction of the NetworkService.
+  Shutdown();
+
+  // |log_file| is closed on another thread, so have to wait for that to happen.
+  RunUntilIdle();
+
+  JSONFileValueDeserializer deserializer(log_path);
+  std::unique_ptr<base::Value> log_dict =
+      deserializer.Deserialize(nullptr, nullptr);
+  ASSERT_TRUE(log_dict);
+  ASSERT_EQ(log_dict->FindKey("constants")->FindKey("amiatest")->GetString(),
+            "iamatest");
+}
+
 // Verifies that raw headers are only reported if requested.
 TEST_F(NetworkServiceTestWithService, RawRequestHeadersAbsent) {
   CreateNetworkContext();

services/network/public/mojom/network_service.mojom

@@ -6,6 +6,7 @@ module network.mojom;
 
 import "mojo/public/mojom/base/file.mojom";
 import "mojo/public/mojom/base/file_path.mojom";
+import "mojo/public/mojom/base/values.mojom";
 import "services/network/public/mojom/cookie_manager.mojom";
 import "services/network/public/mojom/network_change_manager.mojom";
 import "services/network/public/mojom/network_context.mojom";
@@ -183,6 +184,15 @@ struct CryptConfig {
 interface NetworkService {
   SetClient(NetworkServiceClient client);
 
+  // Starts observing the NetLog event stream and writing entries to |file|.
+  // |constants| is a legend used for decoding constant values in the log; it
+  // will be merged with the |GetNetConstants()| dictionary before being passed
+  // through to the FileObserver. (See |FileNetLogObserver::CreateBounded()|
+  // for more details). Most clients will just be adding a dictionary under
+  // the key "clientInfo".
+  StartNetLog(mojo_base.mojom.File file,
+              mojo_base.mojom.DictionaryValue constants);
+
   // Creates a new network context with the given parameters.
   CreateNetworkContext(NetworkContext& context,
                        NetworkContextParams params);

services/service_manager/public/cpp/service_test.cc

@@ -60,6 +60,15 @@ void ServiceTest::OnStartCalled(Connector* connector,
   initialize_called_.Run();
 }
 
+void ServiceTest::Shutdown() {
+  background_service_manager_.reset();
+  context_.reset();
+}
+
+void ServiceTest::RunUntilIdle() {
+  scoped_task_environment_.RunUntilIdle();
+}
+
 void ServiceTest::SetUp() {
   background_service_manager_ =
       std::make_unique<service_manager::BackgroundServiceManager>(
@@ -80,8 +89,7 @@ void ServiceTest::SetUp() {
 }
 
 void ServiceTest::TearDown() {
-  background_service_manager_.reset();
-  context_.reset();
+  Shutdown();
 }
 
 }  // namespace test

services/service_manager/public/cpp/service_test.h

@@ -91,6 +91,15 @@ class ServiceTest : public testing::Test {
                      const std::string& name,
                      const std::string& userid);
 
+  // Explicitly shuts down the ServiceManager and |context_|. This is called
+  // from TearDown(), but may be called explicitly to test shutdown behavior.
+  void Shutdown();
+
+  // Calls RunUntilIdle() on the current process's ScopedTaskEnvironment. Does
+  // not wait until the task environments of other processes, if there are any,
+  // are idle.
+  void RunUntilIdle();
+
   // testing::Test:
   void SetUp() override;
   void TearDown() override;