Unifying ChildProcessLauncher across platforms.

content/browser/child_process_launcher.h

Index: content/browser/child_process_launcher.h
diff --git a/content/browser/child_process_launcher.h b/content/browser/child_process_launcher.h
index 8e1e1b319e33352c1c209f4de0c8ef495306f3dc..dad582289f04fc1c12631f8900a1f02f20680877 100644
--- a/content/browser/child_process_launcher.h
+++ b/content/browser/child_process_launcher.h
@@ -5,23 +5,27 @@
 #ifndef CONTENT_BROWSER_CHILD_PROCESS_LAUNCHER_H_
 #define CONTENT_BROWSER_CHILD_PROCESS_LAUNCHER_H_
 
-#include "base/files/scoped_file.h"
+#include <memory>
+
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/shared_memory.h"
 #include "base/memory/weak_ptr.h"
 #include "base/process/kill.h"
-#include "base/process/launch.h"
 #include "base/process/process.h"
 #include "base/threading/non_thread_safe.h"
 #include "build/build_config.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/common/sandboxed_process_launcher_delegate.h"
+#include "content/public/common/result_codes.h"
+#include "content/public/common/zygote_handle.h"
 #include "mojo/edk/embedder/embedder.h"
 #include "mojo/edk/embedder/scoped_platform_handle.h"
 
 #if defined(OS_WIN)
 #include "sandbox/win/src/sandbox_types.h"
+#else
+#include "content/public/browser/file_descriptor_info.h"
 #endif
 
 namespace base {
@@ -30,6 +34,15 @@ class CommandLine;
 
 namespace content {
 
+class SandboxedProcessLauncherDelegate;
+
+#if defined(OS_WIN)
+using FileMappedForLaunch = base::HandlesToInheritVector ;
+#else
+class FileDescriptorInfo;
+using FileMappedForLaunch = FileDescriptorInfo;
+#endif
+
 // Note: These codes are listed in a histogram and any new codes should be added
 // at the end.
 enum LaunchResultCode {
@@ -66,6 +79,114 @@ class CONTENT_EXPORT ChildProcessLauncher : public base::NonThreadSafe {
     virtual ~Client() {}
   };
 
+  // The Helper class provides the platform specific implementation of the child
+  // process launching and decouples the lifecycle of ChildProcessLauncher from
+  // the process launch itself (and the task posting it involves) so that
+  // ChildProcessLauncher can be deleted at any point.
+  class Helper : public base::RefCountedThreadSafe<Helper> {
+   public:
+    Helper(int child_process_id,
+           BrowserThread::ID client_thread_id,
+           std::unique_ptr<base::CommandLine> command_line,
+           std::unique_ptr<SandboxedProcessLauncherDelegate> delegate,
+           const base::WeakPtr<ChildProcessLauncher>& child_process_launcher,
+           bool terminate_on_shutdown);
+
+    // The methods below are defined in the order they are called.
+
+    // Starts the flow of launching the process.
+    void StartLaunchOnClientThread();
+
+    // Platform specific.
+    void BeforeLaunchOnClientThread();
+
+    // Called in to give implementors a chance at creating a server pipe.
+    // Platform specific.
+    mojo::edk::ScopedPlatformHandle PrepareMojoPipeHandlesOnClientThread();
+
+    // Returns the list of files that should be mapped in the child process.
+    // Platform specific.
+    std::unique_ptr<FileMappedForLaunch> GetFilesToMap();
+
+    // Should return true when a zygote should be used, in which case
+    // ForkAsZygote() will be called (so far Linux only).
+    // Platform specific.
+    bool ShouldForkAsZygote();
+
+    // Starts the child as a zygote.
+    // Platform specific.
+    ZygoteHandle ForkAsZygote(
+        std::unique_ptr<FileMappedForLaunch> files_to_register,
+        base::Process* process);
+
+    // Platform specific.
+    void BeforeLaunchOnLauncherThread(
+        const FileMappedForLaunch& files_to_register,
+        base::LaunchOptions* options);
+
+    // Does the actual starting of the process.
+    // |is_synchronous_launch| is set to true if the starting of the process is

[boliu, 2017/01/13 19:00:55]

this comment is backwards

[Jay Civelli, 2017/01/17 17:50:02]

Fixed.

+    // asynchonous (this is the case on Android), in which case the retuned
+    // Process is not valid (and PostLaunchOnLauncherThread() will provide the
+    // process once it is available).
+    // Platform specific.
+    base::Process LaunchProcessOnLauncherThread(
+        const base::LaunchOptions& options,
+        FileMappedForLaunch* files_to_register,
+        bool* is_synchronous_launch,
+        int* launch_result);
+
+    // Called right after the process has been launched, whether it was created
+    // yet or not.
+    // Platform specific.
+    void AfterLaunchOnLauncherThread(const base::Process& process,
+                                     const base::LaunchOptions& options);
+
+    // Called once the process has been created with a valid |process| if the
+    // process wasn't started successfully.

[boliu, 2017/01/13 19:00:55]

|process| validity, or whether launch was successful doesn't affect whether this
is called though, right?

[Jay Civelli, 2017/01/17 17:50:02]

Yes, attempted to clarify.

+    // if |post_launch_on_client_thread_called| is false,
+    // PostLaunchOnClientThread is called on the client thread.
+    void PostLaunchOnLauncherThread(
+        base::Process process,
+        ZygoteHandle zygote,
+        int launch_result,
+        bool post_launch_on_client_thread_called);
+
+    // Note that this could be called before PostLaunchOnLauncherThread() is
+    // called.
+    void PostLaunchOnClientThread(base::Process process,
+                                  ZygoteHandle zygote,
+                                  int error_code);
+
+    int client_thread_id() const { return client_thread_id_; }
+
+   protected:
+    virtual ~Helper();

[boliu, 2017/01/13 19:00:55]

no subclasses, so doesn't need to be virtual or protected

[Jay Civelli, 2017/01/17 17:50:02]

Done.

+
+   private:
+    friend class base::RefCountedThreadSafe<Helper>;
+
+    void LaunchOnLauncherThread();
+
+    const mojo::edk::PlatformHandle& mojo_client_handle() const {
+      return mojo_client_handle_.get();
+    }
+    base::CommandLine* command_line() { return command_line_.get(); }
+    int child_process_id() const { return child_process_id_; }
+
+    std::string GetProcessType();
+
+    const int child_process_id_;
+    const BrowserThread::ID client_thread_id_;
+    base::TimeTicks begin_launch_time_;
+    std::unique_ptr<base::CommandLine> command_line_;
+    std::unique_ptr<SandboxedProcessLauncherDelegate> delegate_;
+    base::WeakPtr<ChildProcessLauncher> child_process_launcher_;
+    mojo::edk::ScopedPlatformHandle mojo_client_handle_;
+    mojo::edk::ScopedPlatformHandle mojo_server_handle_;
+    bool terminate_on_shutdown_;
+  };
+
   // Launches the process asynchronously, calling the client when the result is
   // ready.  Deleting this object before the process is created is safe, since
   // the callback won't be called.  If the process is still running by the time
@@ -109,32 +230,31 @@ class CONTENT_EXPORT ChildProcessLauncher : public base::NonThreadSafe {
   // this after the process has started.
   void SetProcessBackgrounded(bool background);
 
+  // Terminates the process.
+  // Returns true if the process was stopped, false if the process had not been
+  // started yet or could not be stopped.
+  // Note that |exit_code| and |wait| are not used on Android.
+  bool Terminate(int exit_code, bool wait);
+
+  // Similar to Terminate() but takes in a |process|, expected to have been
+  // started by ChildProcessLauncher.
+  static bool TerminateProcess(const base::Process& process,
+                               int exit_code,
+                               bool wait);
+
   // Replaces the ChildProcessLauncher::Client for testing purposes. Returns the
   // previous  client.
   Client* ReplaceClientForTest(Client* client);
 
  private:
-  // Posts a task to the launcher thread to do the actual work.
-  void Launch(std::unique_ptr<SandboxedProcessLauncherDelegate> delegate,
-              std::unique_ptr<base::CommandLine> cmd_line,
-              int child_process_id);
+  static void Terminate(ZygoteHandle zygote, base::Process process);

[boliu, 2017/01/13 19:00:55]

nit, same name as method on line 237, which is a bit confusing, give it a
different name, DoTerminate?

[Jay Civelli, 2017/01/17 17:50:02]

Renamed to the more explicit:
ForceNormalProcessTerminationAsync()

+  static void TerminateOnLauncherThread(
+      ZygoteHandle zygote, base::Process process);

[boliu, 2017/01/13 19:00:55]

these two are also platform, should they move to helper class to be consistent?

or leave a comment

[Jay Civelli, 2017/01/17 17:50:02]

Moved to the helper class.

+  static void SetProcessBackgroundedOnLauncherThread(
+      base::Process process, bool background);
 
   void UpdateTerminationStatus(bool known_dead);
 
-  // This is always called on the client thread after an attempt
-  // to launch the child process on the launcher thread.
-  // It makes sure we always perform the necessary cleanup if the
-  // client went away.
-  static void DidLaunch(base::WeakPtr<ChildProcessLauncher> instance,
-                        bool terminate_on_shutdown,
-                        mojo::edk::ScopedPlatformHandle server_handle,
-                        ZygoteHandle zygote,
-#if defined(OS_ANDROID)
-                        base::ScopedFD mojo_fd,
-#endif
-                        base::Process process,
-                        int error_code);
-
   // Notifies the client about the result of the operation.
   void Notify(ZygoteHandle zygote,
               mojo::edk::ScopedPlatformHandle server_handle,
@@ -156,6 +276,8 @@ class CONTENT_EXPORT ChildProcessLauncher : public base::NonThreadSafe {
 
   const std::string mojo_child_token_;
 
+  scoped_refptr<Helper> helper_;
+
   base::WeakPtrFactory<ChildProcessLauncher> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ChildProcessLauncher);