Clean up Linux zygote creation code

content/browser/child_process_launcher_helper_linux.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/path_service.h"
 #include "base/posix/global_descriptors.h"
 #include "content/browser/child_process_launcher.h"
 #include "content/browser/child_process_launcher_helper.h"
 #include "content/browser/child_process_launcher_helper_posix.h"
 #include "content/browser/renderer_host/render_sandbox_host_linux.h"
@@ skipping 48 matching lines @@
 }
 
 ChildProcessLauncherHelper::Process
 ChildProcessLauncherHelper::LaunchProcessOnLauncherThread(
     const base::LaunchOptions& options,
     std::unique_ptr<FileMappedForLaunch> files_to_register,
     bool* is_synchronous_launch,
     int* launch_result) {
   *is_synchronous_launch = true;
 
-  ZygoteHandle* zygote_handle =
-      base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoZygote) ?
-      nullptr : delegate_->GetZygote();
+  ZygoteHandle zygote_handle =
+      base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoZygote)
+          ? nullptr
+          : delegate_->GetZygote();
   if (zygote_handle) {
-    // This code runs on the PROCESS_LAUNCHER thread so race conditions are not
-    // an issue with the lazy initialization.
-    if (*zygote_handle == nullptr) {

[James Cook, 2017/04/28 20:07:52]

This bit is a little subtle, and is the thing I'm worried could be causing the
zygote check failure I saw.

In the old code, delegate_->GetZygote() can return:
* null, which means "don't use a zygote"
* a pointer to a non-null ZygoteHandle (aka ZygoteCommunication*), which means
"use this zygote for launching". I think right now that ZygoteHandle is always
the "generic zygote"
* a pointer to a null ZygoteHandle, which means "lazily create a zygote for this
process". I don't think that is currently used. However, if it was used it could
result in extra zygotes being launched, which could cause the problem I saw.

Since the current intent of the code is to use a single zygote, I changed
GetZygote() to always return the handle, so the return value only has 2 possible
meanings.

-      *zygote_handle = CreateZygote();
-    }
-    base::ProcessHandle handle = (*zygote_handle)->ForkRequest(
-        command_line()->argv(),
-        std::move(files_to_register),
-        GetProcessType());
+    // TODO(crbug.com/569191): If chrome supported multiple zygotes they could
+    // be created lazily here, or in the delegate GetZygote() implementations.
+    // Additionally, the delegate could provide a UseGenericZygote() method.
+    base::ProcessHandle handle = zygote_handle->ForkRequest(
+        command_line()->argv(), std::move(files_to_register), GetProcessType());
     *launch_result = LAUNCH_RESULT_SUCCESS;
     Process process;
     process.process = base::Process(handle);
-    process.zygote = *zygote_handle;
+    process.zygote = zygote_handle;
     return process;
   }
 
   Process process;
   process.process = base::LaunchProcess(*command_line(), options);
   *launch_result = process.process.IsValid() ? LAUNCH_RESULT_SUCCESS
                                              : LAUNCH_RESULT_FAILURE;
   return process;
 }
 
@@ skipping 64 matching lines @@
   bool result = base::PathService::Get(base::BasePathKey::DIR_EXE, &exe_dir);
   DCHECK(result);
   base::File file(exe_dir.Append(path),
                   base::File::FLAG_OPEN | base::File::FLAG_READ);
   *region = base::MemoryMappedFile::Region::kWholeFile;
   return file;
 }
 
 }  // namespace internal
 }  // namespace content