The worker process launcher can now ask the worker to crash.

remoting/host/win/worker_process_launcher.cc

Index: remoting/host/win/worker_process_launcher.cc
diff --git a/remoting/host/win/worker_process_launcher.cc b/remoting/host/win/worker_process_launcher.cc
index 6ca563cf8939e86084b4f42474b1eae250510f3b..c2ea19e46e23cb01cdd0878c9405d53f4bfa33d0 100644
--- a/remoting/host/win/worker_process_launcher.cc
+++ b/remoting/host/win/worker_process_launcher.cc
@@ -4,6 +4,7 @@
 
 #include "remoting/host/win/worker_process_launcher.h"
 
+#include "base/location.h"
 #include "base/logging.h"
 #include "base/single_thread_task_runner.h"
 #include "base/time.h"
@@ -13,6 +14,7 @@
 #include "ipc/ipc_listener.h"
 #include "ipc/ipc_message.h"
 #include "net/base/backoff_entry.h"
+#include "remoting/host/chromoting_messages.h"
 #include "remoting/host/worker_process_ipc_delegate.h"
 
 using base::TimeDelta;
@@ -44,6 +46,7 @@ const net::BackoffEntry::Policy kDefaultBackoffPolicy = {
   false,
 };
 
+const int kDelayTimeoutSeconds = 5;

[Wez, 2013/03/07 01:54:35]

nit: Naming.  e.g. kCrashRequestTimeoutSeconds?

[Wez, 2013/03/07 01:54:35]

5s is probably too short for many systems, BTW.

[alexeypa (please no reviews), 2013/03/07 21:28:30]

Done.

[alexeypa (please no reviews), 2013/03/07 21:28:30]

It is enough because the timeout is, strictly speaking, optional. We can kill
the process at the spot. It is just waiting a little bit can give us a bit of
useful information.

[Wez, 2013/03/08 01:45:15]

Right, but if the timeout is short then on slower, older systems we lose that
data more easily. Hopefully not a big deal.

 
 namespace remoting {
 
@@ -52,7 +55,7 @@ namespace remoting {
 // method. In case of error the channel is closed and the worker process is
 // terminated.
 class WorkerProcessLauncher::Core
-    : public base::RefCountedThreadSafe<WorkerProcessLauncher::Core>,
+    : public base::RefCountedThreadSafe<Core, Core>,
       public base::win::ObjectWatcher::Delegate,
       public IPC::Listener {
  public:
@@ -75,9 +78,10 @@ class WorkerProcessLauncher::Core
   // to |this| as soon as Stop() returns.
   void Stop();
 
-  // 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.
+  // See WorkerProcessLauncher::Crash().
+  void Crash(const tracked_objects::Location& location);
+
+  // See WorkerProcessLauncher::Send().
   void Send(IPC::Message* message);
 
   // base::win::ObjectWatcher::Delegate implementation.
@@ -88,8 +92,13 @@ class WorkerProcessLauncher::Core
   virtual void OnChannelConnected(int32 peer_pid) OVERRIDE;
   virtual void OnChannelError() OVERRIDE;
 
+  // Destroys |Core| instances on the |caller_task_runner_| thread. This is
+  // needed because base::OneShotTimer instances must be destroyed on the same
+  // thread they were  created on.
+  static void Destruct(const Core* core);

[Wez, 2013/03/07 01:54:35]

We use an explicit separate struct e.g. CoreTraits to achieve this in other
places.

[alexeypa (please no reviews), 2013/03/07 21:28:30]

WorkerProcessLauncher::Core is a private class declared in
WorkerProcessLauncher. Having a separate Traits structure will pollute
WorkerProcessLauncher with unnecessary "friend" declarations.

[Wez, 2013/03/08 01:45:15]

Surely the friend declaration would be in the Core definition, here, though?

[alexeypa (please no reviews), 2013/03/08 17:43:19]

No, there will be one in WorkerProcessLauncher and another in
WorkerProcessLauncher::Core.

+
  private:
-  friend class base::RefCountedThreadSafe<Core>;
+  friend class base::DeleteHelper<Core>;
   virtual ~Core();
 
   // Attempts to launch the worker process. Schedules next launch attempt if
@@ -116,21 +125,21 @@ class WorkerProcessLauncher::Core
   typedef BOOL (WINAPI * GetNamedPipeClientProcessIdFn)(HANDLE, DWORD*);
   GetNamedPipeClientProcessIdFn get_named_pipe_client_pid_;
 
+  // The timer used to delay termination of the worker process when an IPC error
+  // occured or when Crash() request is pending
+  base::OneShotTimer<Core> delay_timer_;

[Wez, 2013/03/07 01:54:35]

nit: Naming.

[alexeypa (please no reviews), 2013/03/07 21:28:30]

Done.

+
   // True if IPC messages should be passed to |worker_delegate_|.
   bool ipc_enabled_;
 
-  // The timer used to delay termination of the process in the case of an IPC
-  // error.
-  scoped_ptr<base::OneShotTimer<Core> > ipc_error_timer_;
-
   // Launch backoff state.
   net::BackoffEntry launch_backoff_;
 
   // Timer used to delay recording a successfull launch.
-  scoped_ptr<base::OneShotTimer<Core> > launch_success_timer_;
+  base::OneShotTimer<Core> launch_success_timer_;
 
   // Timer used to schedule the next attempt to launch the process.
-  scoped_ptr<base::OneShotTimer<Core> > launch_timer_;
+  base::OneShotTimer<Core> launch_timer_;
 
   // Used to determine when the launched process terminates.
   base::win::ObjectWatcher process_watcher_;
@@ -160,11 +169,6 @@ WorkerProcessLauncher::Core::Core(
       launch_backoff_(&kDefaultBackoffPolicy),
       stopping_(false) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
-
-  // base::OneShotTimer must be destroyed on the same thread it was created on.
-  ipc_error_timer_.reset(new base::OneShotTimer<Core>());
-  launch_success_timer_.reset(new base::OneShotTimer<Core>());
-  launch_timer_.reset(new base::OneShotTimer<Core>());
 }
 
 void WorkerProcessLauncher::Core::Start() {
@@ -184,6 +188,29 @@ void WorkerProcessLauncher::Core::Stop() {
   }
 }
 
+void WorkerProcessLauncher::Core::Crash(
+    const tracked_objects::Location& location) {
+  DCHECK(caller_task_runner_->BelongsToCurrentThread());
+
+  // Ask the worker process to crash voluntarily if it is still connected.
+  if (ipc_enabled_) {
+    Send(new ChromotingDaemonMsg_Crash(location.function_name(),
+                                       location.file_name(),
+                                       location.line_number()));
+  }
+
+  // Close the channel and ignore any not yet processed messages.
+  launcher_delegate_->CloseChannel();
+  ipc_enabled_ = false;
+
+  // Give the worker process some time to crash.
+  if (!delay_timer_.IsRunning()) {
+    delay_timer_.Start(FROM_HERE,
+                       base::TimeDelta::FromSeconds(kDelayTimeoutSeconds), this,
+                       &Core::StopWorker);

[Wez, 2013/03/07 01:54:35]

StopWorker "stops the worker asynchronously". Is it tought enough for our needs?
 Should it be KillWorker? :)

[alexeypa (please no reviews), 2013/03/07 21:28:30]

It is tougher than Chuck Norris (as long as we are in user mode).

+  }
+}
+
 void WorkerProcessLauncher::Core::Send(IPC::Message* message) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
@@ -241,19 +268,28 @@ void WorkerProcessLauncher::Core::OnChannelError() {
   // here allows the worker to exit completely and so, notify
   // |process_watcher_|. As the result KillProcess() will not be called and
   // the original exit code reported by the worker process will be retrieved.
-  ipc_error_timer_->Start(FROM_HERE, base::TimeDelta::FromSeconds(5),
-                          this, &Core::StopWorker);
+  if (!delay_timer_.IsRunning()) {
+    delay_timer_.Start(FROM_HERE,
+                       base::TimeDelta::FromSeconds(kDelayTimeoutSeconds), this,
+                       &Core::StopWorker);
+  }
+}
+
+// static
+void WorkerProcessLauncher::Core::Destruct(const Core* core) {
+  core->caller_task_runner_->DeleteSoon(FROM_HERE, core);
 }
 
 WorkerProcessLauncher::Core::~Core() {
+  DCHECK(caller_task_runner_->BelongsToCurrentThread());
   DCHECK(stopping_);
 }
 
 void WorkerProcessLauncher::Core::LaunchWorker() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
   DCHECK(!ipc_enabled_);
-  DCHECK(!launch_success_timer_->IsRunning());
-  DCHECK(!launch_timer_->IsRunning());
+  DCHECK(!launch_success_timer_.IsRunning());
+  DCHECK(!launch_timer_.IsRunning());
   DCHECK(!process_exit_event_.IsValid());
   DCHECK(process_watcher_.GetWatchedObject() == NULL);
 
@@ -264,8 +300,8 @@ void WorkerProcessLauncher::Core::LaunchWorker() {
       ipc_enabled_ = true;
       // Record a successful launch once the process has been running for at
       // least two seconds.
-      launch_success_timer_->Start(FROM_HERE, base::TimeDelta::FromSeconds(2),
-                                   this, &Core::RecordSuccessfulLaunch);
+      launch_success_timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(2),
+                                  this, &Core::RecordSuccessfulLaunch);
       return;
     }
 
@@ -289,8 +325,8 @@ void WorkerProcessLauncher::Core::StopWorker() {
   scoped_refptr<Core> self = this;
 
   // Record a launch failure if the process exited too soon.
-  if (launch_success_timer_->IsRunning()) {
-    launch_success_timer_->Stop();
+  if (launch_success_timer_.IsRunning()) {
+    launch_success_timer_.Stop();
     launch_backoff_.InformOfRequest(false);
   }
 
@@ -310,23 +346,20 @@ void WorkerProcessLauncher::Core::StopWorker() {
     process_watcher_.StopWatching();
   }
 
-  ipc_error_timer_->Stop();
+  delay_timer_.Stop();
   process_exit_event_.Close();
 
   // Do not relaunch the worker process if the caller has asked us to stop.
-  if (stopping_) {
-    ipc_error_timer_.reset();
-    launch_timer_.reset();
+  if (stopping_)
     return;
-  }
 
   if (launcher_delegate_->IsPermanentError(launch_backoff_.failure_count())) {
     if (!stopping_)
       worker_delegate_->OnPermanentError();
   } else {
     // Schedule the next attempt to launch the worker process.
-    launch_timer_->Start(FROM_HERE, launch_backoff_.GetTimeUntilRelease(),
-                         this, &Core::LaunchWorker);
+    launch_timer_.Start(FROM_HERE, launch_backoff_.GetTimeUntilRelease(), this,
+                        &Core::LaunchWorker);
   }
 }
 
@@ -344,6 +377,10 @@ WorkerProcessLauncher::~WorkerProcessLauncher() {
   core_ = NULL;
 }
 
+void WorkerProcessLauncher::Crash(const tracked_objects::Location& location) {
+  core_->Crash(location);
+}
+
 void WorkerProcessLauncher::Send(IPC::Message* message) {
   core_->Send(message);
 }