Files required by a service now listed in manifest.

content/browser/child_process_launcher_helper_posix.cc

Index: content/browser/child_process_launcher_helper_posix.cc
diff --git a/content/browser/child_process_launcher_helper_posix.cc b/content/browser/child_process_launcher_helper_posix.cc
index 9c3583ea6333501c26475c5c82177de78999cd5c..f3ec09bbe79b8506598194a4cec8981eb880577b 100644
--- a/content/browser/child_process_launcher_helper_posix.cc
+++ b/content/browser/child_process_launcher_helper_posix.cc
@@ -5,20 +5,59 @@
 #include "content/browser/child_process_launcher_helper_posix.h"
 
 #include "base/command_line.h"
+#include "base/lazy_instance.h"
 #include "base/metrics/field_trial.h"
+#include "base/posix/global_descriptors.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
 #include "content/browser/file_descriptor_info_impl.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_descriptors.h"
+#include "content/public/common/content_switches.h"
 #include "mojo/edk/embedder/platform_handle.h"
+#include "services/catalog/public/cpp/manifest_parsing_util.h"
 
 namespace content {
 namespace internal {
 
+namespace {
+
+using OpenedFileMap =
+    std::map<base::FilePath,
+             std::pair<base::PlatformFile, base::MemoryMappedFile::Region>>;
+
+base::LazyInstance<
+    std::map<std::string, std::unique_ptr<catalog::RequiredFileMap>>>

[dcheng, 2017/02/15 08:05:27]

Nit: no unique_ptr indirection, using RequiredFileMap directly with std::move()
should work

[Jay Civelli, 2017/02/15 19:53:47]

Done.

+    g_required_files_by_service = LAZY_INSTANCE_INITIALIZER;
+
+base::LazyInstance<OpenedFileMap> g_opened_files = LAZY_INSTANCE_INITIALIZER;

[dcheng, 2017/02/15 08:05:27]

Nit: use function-local statics instead of LazyInstance?

[Jay Civelli, 2017/02/15 19:53:47]

Done.

+
+base::PlatformFile OpenFileIfNecessary(const base::FilePath& path,
+                                       base::MemoryMappedFile::Region* region) {
+  const auto& iter = g_opened_files.Get().find(path);
+  if (iter != g_opened_files.Get().end()) {
+    *region = iter->second.second;
+    return iter->second.first;
+  }
+  base::File file = OpenFileToShare(path, region);
+  if (!file.IsValid()) {
+    return base::kInvalidPlatformFile;
+  }
+  // g_opened_files becomes the owner of the file descriptor.
+  base::PlatformFile fd = file.TakePlatformFile();
+  g_opened_files.Get()[path] = std::make_pair(fd, *region);
+  return fd;
+}
+
+}  // namespace
+
 std::unique_ptr<FileDescriptorInfo> CreateDefaultPosixFilesToMap(
-    const base::CommandLine& command_line,
     int child_process_id,
-    const mojo::edk::PlatformHandle& mojo_client_handle) {
+    const mojo::edk::PlatformHandle& mojo_client_handle,
+    bool include_service_required_files,
+    const std::string& process_type,
+    base::CommandLine* command_line) {
   std::unique_ptr<FileDescriptorInfo> files_to_register(
       FileDescriptorInfoImpl::Create());
 
@@ -33,11 +72,77 @@ std::unique_ptr<FileDescriptorInfo> CreateDefaultPosixFilesToMap(
   // GetAdditionalMappedFilesForChildProcess a no op on Mac.
 #if !defined(OS_MACOSX)
   GetContentClient()->browser()->GetAdditionalMappedFilesForChildProcess(
-      command_line, child_process_id, files_to_register.get());
+      *command_line, child_process_id, files_to_register.get());
 #endif
 
+  if (!include_service_required_files)
+    return files_to_register;
+
+  static std::map<std::string, std::string>* service_name_resolver = nullptr;

[dcheng, 2017/02/15 08:05:27]

Another alternative:

static auto* service_name_resolver =
    new std::map<std::string, std::string>()({
  {switches::kGpuProcess, "content_gpu"},
  {switches::kPpapiPluginProcess, "content_plugin"},
  ...
});

As a bonus, it's automatically thread-safe.

[Jay Civelli, 2017/02/15 19:53:47]

Nice! Done.

+  if (!service_name_resolver) {
+    service_name_resolver = new std::map<std::string, std::string>();
+    // The service names are defined in the JSON manifests, so we don't have a
+    // constant accessible for them.
+    // TODO(jcivelli): remove this map once the service name is accessible from
+    //                 the command line crbug.com/687250
+    (*service_name_resolver)[switches::kGpuProcess] = "content_gpu";
+    (*service_name_resolver)[switches::kPpapiPluginProcess] = "content_plugin";
+    (*service_name_resolver)[switches::kRendererProcess] = "content_renderer";
+    (*service_name_resolver)[switches::kUtilityProcess] = "content_utility";
+    (*service_name_resolver)["ppapi-broker"] = "ppapi_broker";
+    (*service_name_resolver)["nacl-loader"] = "nacl_loader";
+    (*service_name_resolver)["nacl-loader-nonsfi"] = "nacl_loader_nonsfi";
+  }
+
+  // Also include the files specified in the services' manifests.
+  const auto& service_name_iter = service_name_resolver->find(process_type);
+  CHECK(service_name_iter != service_name_resolver->end())

[dcheng, 2017/02/15 08:05:27]

DCHECK, unless the input can be controlled by arbitrary content (which I would
imagine shouldn't be the case)

[Jay Civelli, 2017/02/15 19:53:47]

Done.

+      << "No service found for process type " << process_type;
+  std::string service_name = service_name_iter->second;

[dcheng, 2017/02/15 08:05:27]

Nit: const std::string& to save a copy

[Jay Civelli, 2017/02/15 19:53:47]

Done.

+  const auto& files_iter = g_required_files_by_service.Get().find(service_name);

[dcheng, 2017/02/15 08:05:27]

But this can just be auto, since iterators are lightweight value types
(relatively)--and in this case, lifetime extension applies anyway.

[Jay Civelli, 2017/02/15 19:53:47]

Done.

+  if (files_iter != g_required_files_by_service.Get().end()) {
+    catalog::RequiredFileMap* required_files_map = files_iter->second.get();
+    base::GlobalDescriptors::Key key = kContentDynamicDescriptorStart;
+    std::string switch_value;
+    for (const auto& key_path_iter : *required_files_map) {
+      base::MemoryMappedFile::Region region;
+      base::PlatformFile file =
+          OpenFileIfNecessary(base::FilePath(key_path_iter.second), &region);

[dcheng, 2017/02/15 08:05:27]

Can the map just hold a FilePath?

[Jay Civelli, 2017/02/15 19:53:47]

Good idea, done.

+      if (file == base::kInvalidPlatformFile) {
+        LOG(ERROR) << "Ignoring invalid file " << key_path_iter.second;

[dcheng, 2017/02/15 08:05:27]

DLOG instead of LOG? This comment applies to many of the other newly added
logging messages.

[Jay Civelli, 2017/02/15 19:53:47]

Went through all of them and changed them to DLOG.

+        continue;
+      }
+      if (!switch_value.empty()) {
+        switch_value += ",";
+      }
+      switch_value += key_path_iter.first;
+      switch_value += ":";
+      switch_value += base::IntToString(key);
+      files_to_register->ShareWithRegion(key, file, region);
+      key++;
+      DCHECK(key < kContentDynamicDescriptorMax);
+    }
+    command_line->AppendSwitchASCII(switches::kSharedFiles, switch_value);
+  }
+
   return files_to_register;
 }
 
+void SetFilesToShareForServicePosix(
+    const std::string& service_name,
+    std::unique_ptr<catalog::RequiredFileMap> required_files) {

[dcheng, 2017/02/15 08:05:27]

Consider just passing by value and moving below, rather than indirecting through
unique_ptr

[Jay Civelli, 2017/02/15 19:53:47]

Done.

+  if (required_files->empty())
+    return;
+
+  if (!base::StartsWith(service_name, "content",

[dcheng, 2017/02/15 08:05:27]

Maybe content_ to be safe?

[Jay Civelli, 2017/02/15 19:53:47]

Done.

+                        base::CompareCase::INSENSITIVE_ASCII)) {
+    // Not a content child service, ignore.
+    return;
+  }
+
+  DCHECK(g_required_files_by_service.Get().count(service_name) == 0);
+  g_required_files_by_service.Get()[service_name] = std::move(required_files);
+}
+
 }  // namespace internal
 }  // namespace content