[Chromoting] Refactored worker process launching code and speeded up the desktop process launch.

remoting/host/win/worker_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 REMOTING_HOST_WIN_WORKER_PROCESS_LAUNCHER_H_
 #define REMOTING_HOST_WIN_WORKER_PROCESS_LAUNCHER_H_
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/timer.h"
+#include "base/win/object_watcher.h"
 #include "base/win/scoped_handle.h"
-#include "ipc/ipc_sender.h"
+#include "net/base/backoff_entry.h"
 
 namespace base {
 class SingleThreadTaskRunner;
 class TimeDelta;
 } // namespace base
 
 namespace IPC {
-class Listener;
 class Message;
 } // namespace IPC
 
 namespace tracked_objects {
 class Location;
 }  // namespace tracked_objects
 
 namespace remoting {
 
 class WorkerProcessIpcDelegate;
 
 // Launches a worker process that is controlled via an IPC channel. All
 // interaction with the spawned process is through WorkerProcessIpcDelegate and
 // Send() method. In case of error the channel is closed and the worker process
 // is terminated.
-class WorkerProcessLauncher {
+class WorkerProcessLauncher
+    : public base::NonThreadSafe,
+      public base::win::ObjectWatcher::Delegate {
  public:
-  class Delegate : public IPC::Sender {
+  class Delegate {
    public:
     virtual ~Delegate();
 
+    // Asynchronously starts the worker process and creates an IPC channel it
+    // can connect to. |event_handler| must remain valid until KillProcess() has
+    // been called.
+    virtual void LaunchProcess(WorkerProcessLauncher* event_handler) = 0;
+
+    // Sends an IPC message to the worker process. The message will be silently
+    // dropped if the channel is closed.
+    virtual void Send(IPC::Message* message) = 0;
+
     // Closes the IPC channel.
     virtual void CloseChannel() = 0;
 
-    // Returns PID of the worker process or 0 if it is not available.
-    virtual DWORD GetProcessId() const = 0;
-
-    // Returns true if the worker process should not be restarted any more.
-    virtual bool IsPermanentError(int failure_count) const = 0;
-
-    // Terminates the worker process with the given exit code. Destroys the IPC
-    // channel created by LaunchProcess().
-    virtual void KillProcess(DWORD exit_code) = 0;
-
-    // Starts the worker process and creates an IPC channel it can connect to.
-    // |delegate| specifies the object that will receive notifications from
-    // the IPC channel. |process_exit_event_out| receives a handle that becomes
-    // signalled once the launched process has been terminated.
-    virtual bool LaunchProcess(
-        IPC::Listener* delegate,
-        base::win::ScopedHandle* process_exit_event_out) = 0;
+    // Terminates the worker process and closes the IPC channel.
+    virtual void KillProcess() = 0;
   };
 
   // Creates the launcher that will use |launcher_delegate| to manage the worker
   // process and |worker_delegate| to handle IPCs. The caller must ensure that
-  // |worker_delegate| remains valid until Stoppable::Stop() method has been
-  // called.
-  //
-  // The caller should call all the methods on this class on
-  // the |caller_task_runner| thread. Methods of both delegate interfaces are
-  // called on the |caller_task_runner| thread as well.
+  // |worker_delegate| must outlive this object.
   WorkerProcessLauncher(
-      scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner,
       scoped_ptr<Delegate> launcher_delegate,
       WorkerProcessIpcDelegate* worker_delegate);
-  ~WorkerProcessLauncher();
+  virtual ~WorkerProcessLauncher();
 
   // Asks the worker process to crash and generate a dump, and closes the IPC
   // channel. |location| is passed to the worker so that it is on the stack in
   // the dump. Restarts the worker process forcefully, if it does
   // not exit on its own.
   void Crash(const tracked_objects::Location& location);
 
   // Sends an IPC message to the worker process. The message will be silently
   // dropped if Send() is called before Start() or after stutdown has been
   // initiated.
   void Send(IPC::Message* message);
 
+  // Notification methods invoked by |Delegate|.
+
+  // Invoked to pass a handle of the launched process back to the caller of
+  // Delegate::LaunchProcess(). The implementation has to make sure that this

[Wez, 2013/05/14 03:45:22]

nit: The WorkerProcessLauncher implementation, or the Delegate?  If the former
then rephrase as "WorkerProcessLauncher ensures that..."

[alexeypa (please no reviews), 2013/05/14 18:31:10]

It is the delegate. I updated the comment.

+  // method is called before OnChannelConnected().
+  void OnProcessLaunched(base::win::ScopedHandle worker_process);
+
+  // Called when a fatal error occurs (i.e. a failed process launch).
+  // The implementation must guarantee that no other notifications is
+  // delivered once OnFatalError() has been called.

[Wez, 2013/05/14 03:45:22]

Same here.

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+  void OnFatalError();
+
+  // Mirrors methods of IPC::Listener.

[Wez, 2013/05/14 03:45:22]

nit: Clarify why these need to be mirrored here.

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+  bool OnMessageReceived(const IPC::Message& message);
+  void OnChannelConnected(int32 peer_pid);
+  void OnChannelError();
+
  private:
   friend class WorkerProcessLauncherTest;
 
-  // Hooks that allow test code to call the corresponding methods of |Core|.
-  void ResetLaunchSuccessTimeoutForTest();
+  // base::win::ObjectWatcher::Delegate implementation.

[Wez, 2013/05/14 03:45:22]

nit: "... used to watch for the worker process exiting."?

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+  virtual void OnObjectSignaled(HANDLE object) OVERRIDE;
+
+  // Attempts to launch the worker process. Schedules next launch attempt if
+  // creation of the process fails.
+  void LaunchWorker();
+
+  // Called to record a successfull launch attempt.
+  void RecordSuccessfulLaunch();
+
+  // Used to emulate |start_process_timer_| expiration.

[Wez, 2013/05/14 03:45:22]

nit: Clarify what this method actually does that's different from
RecordSuccessfulLaunch() alone - seems to only call RecordSuccessfulLaunch iff
there's a launch currently in-progress?

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+  void RecordSuccessfulLaunchForTest();
+
+  // Set the desired timeout for |kill_process_timer_|.
   void SetKillProcessTimeoutForTest(const base::TimeDelta& timeout);
 
-  // The actual implementation resides in WorkerProcessLauncher::Core class.
-  class Core;
-  scoped_refptr<Core> core_;
+  // Stops the worker process and schedules next launch attempt unless the
+  // object is being destroyed already.
+  void StopWorker();
+
+  // Implements specifics of launching a worker process.
+  scoped_ptr<WorkerProcessLauncher::Delegate> launcher_delegate_;
+
+  // Handles IPC messages sent by the worker process.
+  WorkerProcessIpcDelegate* worker_delegate_;

[Wez, 2013/05/14 03:45:22]

nit: ipc_handler_?

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+
+  DWORD exit_code_;

[Wez, 2013/05/14 03:45:22]

nit: Add a comment to explain why we need to store this.

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+
+  // True if IPC messages should be passed to |worker_delegate_|.
+  bool ipc_enabled_;
+
+  // The timer used to delay termination of the worker process when an IPC error
+  // occured or when Crash() request is pending
+  base::OneShotTimer<WorkerProcessLauncher> kill_process_timer_;
+
+  // The default timeout for |kill_process_timer_|.

[Wez, 2013/05/14 03:45:22]

nit: Do you really mean "default", or "initial"?

[alexeypa (please no reviews), 2013/05/14 18:31:10]

I really meant "default".

+  base::TimeDelta kill_process_timeout_;
+
+  // Launch backoff state.

[Wez, 2013/05/14 03:45:22]

nit: Suggest "State used to backoff worker launch attempts on failure."

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+  net::BackoffEntry launch_backoff_;
+
+  // Timer used to schedule the next attempt to launch the process.
+  base::OneShotTimer<WorkerProcessLauncher> launch_timer_;
+
+  // Monitors |process_exit_event_| to detect when the launched process

[Wez, 2013/05/14 03:45:22]

nit: There is no |process_exit_event| - do you mean |worker_process_|?

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+  // terminates.
+  base::win::ObjectWatcher process_watcher_;
+
+  // Timer used to cancel a launch attempt if it is taking too long.
+  base::OneShotTimer<WorkerProcessLauncher> start_process_timer_;

[Wez, 2013/05/14 03:45:22]

nit: This is confusingly named wrt |launch_timer_| - consider
|launch_timeout_timer_|?

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+
+  // The handle of the worker process, if launched.
+  base::win::ScopedHandle worker_process_;
+
+  // True when the object is being destroyed.
+  bool stopping_;
 
   DISALLOW_COPY_AND_ASSIGN(WorkerProcessLauncher);
 };
 
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_WIN_WORKER_PROCESS_LAUNCHER_H_