Unifying ChildProcessLauncher across platforms.

content/browser/child_process_launcher.h

 // Copyright (c) 2012 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.
 
 #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 {
 class CommandLine;
 }
 
 namespace content {
 
+class SandboxedProcessLauncherDelegate;
+
+#if defined(OS_WIN)
+using FileMappedForLaunch = base::HandlesToInheritVector ;

[jam, 2017/01/13 18:42:20]

nit: space at end

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

Done.

+#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 {
   // Launch start code, to not overlap with sandbox::ResultCode.
   LAUNCH_RESULT_START = 1001,
   // Launch success.
   LAUNCH_RESULT_SUCCESS,
   // Generic launch failure.
   LAUNCH_RESULT_FAILURE,
   // Placeholder for last item of the enum.
@@ skipping 16 matching lines @@
     // Will be called on the thread that the ChildProcessLauncher was
     // constructed on.
     virtual void OnProcessLaunched() = 0;
 
     virtual void OnProcessLaunchFailed(int error_code) {};
 
    protected:
     virtual ~Client() {}
   };
 
+  // The Helper class provides the platform specific implementation of the child

[jam, 2017/01/13 18:42:19]

nit: can we put this in the end of the public section? that way someone reading
this can just look at what they need as a consumer.

or even perhaps should this be declared in a separate
child_process_launcher_internal.h to make this separation clearer?

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

Moved to a new child_process_launcher_helper file.

+  // 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> {

[jam, 2017/01/13 18:42:19]

hmm, implementation inheritance is something I've learnt to avoid. I'm curious
why you chose to go down this route vs keep the member variables on
ChildProcessLauncher, and have this strictly be an interface?

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

From our discussion, there is no inheritance, the methods are simply implemented
in platform specific files.

+   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();

[jam, 2017/01/13 18:42:19]

why not add ifdefs here to make it clear these methods aren't called on win/mac?


i realize you're trying to reduce the ifdef soup, which is a good improvement.
but seems like most of that can come from the separate implementation files?

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

I created a custom CLP Process struct that contains the base::Process and on
Windows, also the zygote. And I got rid of this zygote specific methods.

+
+    // 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
+    // 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.
+    // 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();
+
+   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
   // this object destructs, it will be terminated.
   // Takes ownership of cmd_line.
   //
   // If |process_error_callback| is provided, it will be called if a Mojo error
   // is encountered when processing messages from the child process. This
   // callback must be safe to call from any thread.
   ChildProcessLauncher(
@@ skipping 23 matching lines @@
   // |exit_code| is the exit code of the process if it exited (e.g. status from
   // waitpid if on posix, from GetExitCodeProcess on Windows). |exit_code| may
   // be NULL.
   base::TerminationStatus GetChildTerminationStatus(bool known_dead,
                                                     int* exit_code);
 
   // Changes whether the process runs in the background or not.  Only call
   // 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);
+  static void TerminateOnLauncherThread(
+      ZygoteHandle zygote, base::Process process);
+  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,
               base::Process process,
               int error_code);
 
   Client* client_;
   BrowserThread::ID client_thread_id_;
   base::Process process_;
   base::TerminationStatus termination_status_;
   int exit_code_;
   ZygoteHandle zygote_;
   bool starting_;
   const mojo::edk::ProcessErrorCallback process_error_callback_;
 
   // Controls whether the child process should be terminated on browser
   // shutdown. Default behavior is to terminate the child.
   const bool terminate_child_on_shutdown_;
 
   const std::string mojo_child_token_;
 
+  scoped_refptr<Helper> helper_;
+
   base::WeakPtrFactory<ChildProcessLauncher> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ChildProcessLauncher);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_CHILD_PROCESS_LAUNCHER_H_