diff options
| -rw-r--r-- | remoting/host/chromoting_messages.h | 23 | ||||
| -rw-r--r-- | remoting/host/daemon_process.cc | 5 | ||||
| -rw-r--r-- | remoting/host/daemon_process.h | 9 | ||||
| -rw-r--r-- | remoting/host/daemon_process_unittest.cc | 16 | ||||
| -rw-r--r-- | remoting/host/daemon_process_win.cc | 10 | ||||
| -rw-r--r-- | remoting/host/desktop_process.cc | 12 | ||||
| -rw-r--r-- | remoting/host/desktop_process.h | 2 | ||||
| -rw-r--r-- | remoting/host/desktop_process_unittest.cc | 2 | ||||
| -rw-r--r-- | remoting/host/desktop_session_win.cc | 11 | ||||
| -rw-r--r-- | remoting/host/desktop_session_win.h | 4 | ||||
| -rw-r--r-- | remoting/host/remoting_me2me_host.cc | 14 | ||||
| -rw-r--r-- | remoting/host/win/unprivileged_process_delegate.cc | 6 | ||||
| -rw-r--r-- | remoting/host/win/unprivileged_process_delegate.h | 1 | ||||
| -rw-r--r-- | remoting/host/win/worker_process_launcher.cc | 134 | ||||
| -rw-r--r-- | remoting/host/win/worker_process_launcher.h | 20 | ||||
| -rw-r--r-- | remoting/host/win/worker_process_launcher_unittest.cc | 217 | ||||
| -rw-r--r-- | remoting/host/win/wts_session_process_delegate.cc | 15 | ||||
| -rw-r--r-- | remoting/host/win/wts_session_process_delegate.h | 1 |
18 files changed, 382 insertions, 120 deletions
diff --git a/remoting/host/chromoting_messages.h b/remoting/host/chromoting_messages.h index 7bad9238..4faed33 100644 --- a/remoting/host/chromoting_messages.h +++ b/remoting/host/chromoting_messages.h @@ -22,17 +22,20 @@ #define IPC_MESSAGE_START ChromotingMsgStart //----------------------------------------------------------------------------- -// Chromoting messages sent from the daemon to the network process. +// Chromoting messages sent from the daemon. -// Requests the network process to crash producing a crash dump. The daemon +// Requests the receiving process to crash producing a crash dump. The daemon // sends this message when a fatal error has been detected indicating that -// the network process misbehaves. The daemon passes the location of the code +// the receiving process misbehaves. The daemon passes the location of the code // that detected the error. -IPC_MESSAGE_CONTROL3(ChromotingDaemonNetworkMsg_Crash, +IPC_MESSAGE_CONTROL3(ChromotingDaemonMsg_Crash, std::string /* function_name */, std::string /* file_name */, int /* line_number */) +//----------------------------------------------------------------------------- +// Chromoting messages sent from the daemon to the network process. + // Delivers the host configuration (and updates) to the network process. IPC_MESSAGE_CONTROL1(ChromotingDaemonNetworkMsg_Configuration, std::string) @@ -112,18 +115,6 @@ IPC_MESSAGE_CONTROL1(ChromotingNetworkDaemonMsg_HostStarted, IPC_MESSAGE_CONTROL0(ChromotingNetworkDaemonMsg_HostShutdown) //----------------------------------------------------------------------------- -// Chromoting messages sent from the daemon to the desktop process. - -// Requests the desktop process to crash producing a crash dump. The daemon -// sends this message when a fatal error has been detected indicating that -// the desktop process misbehaves. The daemon passes the location of the code -// that detected the error. -IPC_MESSAGE_CONTROL3(ChromotingDaemonDesktopMsg_Crash, - std::string /* function_name */, - std::string /* file_name */, - int /* line_number */) - -//----------------------------------------------------------------------------- // Chromoting messages sent from the desktop to the daemon process. // Notifies the daemon that a desktop integration process has been initialized. diff --git a/remoting/host/daemon_process.cc b/remoting/host/daemon_process.cc index 8c7cf67..c63051c 100644 --- a/remoting/host/daemon_process.cc +++ b/remoting/host/daemon_process.cc @@ -9,6 +9,7 @@ #include "base/command_line.h" #include "base/file_util.h" #include "base/files/file_path.h" +#include "base/location.h" #include "base/single_thread_task_runner.h" #include "net/base/net_util.h" #include "remoting/base/auto_thread_task_runner.h" @@ -218,9 +219,9 @@ void DaemonProcess::CreateDesktopSession(int terminal_id, void DaemonProcess::CrashNetworkProcess( const tracked_objects::Location& location) { - SendToNetwork(new ChromotingDaemonNetworkMsg_Crash( - location.function_name(), location.file_name(), location.line_number())); + DCHECK(caller_task_runner()->BelongsToCurrentThread()); + DoCrashNetworkProcess(location); DeleteAllDesktopSessions(); } diff --git a/remoting/host/daemon_process.h b/remoting/host/daemon_process.h index 9877f43..aed640c 100644 --- a/remoting/host/daemon_process.h +++ b/remoting/host/daemon_process.h @@ -9,7 +9,6 @@ #include "base/basictypes.h" #include "base/compiler_specific.h" -#include "base/location.h" #include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" @@ -25,6 +24,10 @@ struct SerializedTransportRoute; +namespace tracked_objects { +class Location; +} // namespace tracked_objects + namespace remoting { class AutoThreadTaskRunner; @@ -126,6 +129,10 @@ class DaemonProcess const DesktopSessionParams& params, bool virtual_terminal) = 0; + // Requests the network process to crash. + virtual void DoCrashNetworkProcess( + const tracked_objects::Location& location) = 0; + // Launches the network process and establishes an IPC channel with it. virtual void LaunchNetworkProcess() = 0; diff --git a/remoting/host/daemon_process_unittest.cc b/remoting/host/daemon_process_unittest.cc index 6c3844a..033a49f 100644 --- a/remoting/host/daemon_process_unittest.cc +++ b/remoting/host/daemon_process_unittest.cc @@ -4,6 +4,7 @@ #include "base/bind.h" #include "base/bind_helpers.h" +#include "base/location.h" #include "base/memory/ref_counted.h" #include "base/process.h" #include "ipc/ipc_message.h" @@ -26,7 +27,7 @@ namespace remoting { namespace { enum Messages { - kMessageCrash = ChromotingDaemonNetworkMsg_Crash::ID, + kMessageCrash = ChromotingDaemonMsg_Crash::ID, kMessageConfiguration = ChromotingDaemonNetworkMsg_Configuration::ID, kMessageConnectTerminal = ChromotingNetworkHostMsg_ConnectTerminal::ID, kMessageDisconnectTerminal = ChromotingNetworkHostMsg_DisconnectTerminal::ID, @@ -68,6 +69,7 @@ class MockDaemonProcess : public DaemonProcess { bool(int, base::ProcessHandle, IPC::PlatformFileForTransit)); MOCK_METHOD1(DoCreateDesktopSessionPtr, DesktopSession*(int)); + MOCK_METHOD1(DoCrashNetworkProcess, void(const tracked_objects::Location&)); MOCK_METHOD0(LaunchNetworkProcess, void()); private: @@ -124,6 +126,7 @@ class DaemonProcessTest : public testing::Test { // DaemonProcess mocks DesktopSession* DoCreateDesktopSession(int terminal_id); + void DoCrashNetworkProcess(const tracked_objects::Location& location); void LaunchNetworkProcess(); // Deletes |daemon_process_|. @@ -168,6 +171,9 @@ void DaemonProcessTest::SetUp() { EXPECT_CALL(*daemon_process_, DoCreateDesktopSessionPtr(_)) .Times(AnyNumber()) .WillRepeatedly(Invoke(this, &DaemonProcessTest::DoCreateDesktopSession)); + EXPECT_CALL(*daemon_process_, DoCrashNetworkProcess(_)) + .Times(AnyNumber()) + .WillRepeatedly(Invoke(this, &DaemonProcessTest::DoCrashNetworkProcess)); EXPECT_CALL(*daemon_process_, LaunchNetworkProcess()) .Times(AnyNumber()) .WillRepeatedly(Invoke(this, &DaemonProcessTest::LaunchNetworkProcess)); @@ -182,6 +188,14 @@ DesktopSession* DaemonProcessTest::DoCreateDesktopSession(int terminal_id) { return new FakeDesktopSession(daemon_process_.get(), terminal_id); } +void DaemonProcessTest::DoCrashNetworkProcess( + const tracked_objects::Location& location) { + daemon_process_->SendToNetwork( + new ChromotingDaemonMsg_Crash(location.function_name(), + location.file_name(), + location.line_number())); +} + void DaemonProcessTest::LaunchNetworkProcess() { terminal_id_ = 0; daemon_process_->OnChannelConnected(0); diff --git a/remoting/host/daemon_process_win.cc b/remoting/host/daemon_process_win.cc index 6c81200..1a042d5 100644 --- a/remoting/host/daemon_process_win.cc +++ b/remoting/host/daemon_process_win.cc @@ -7,6 +7,7 @@ #include "base/base_switches.h" #include "base/bind.h" #include "base/bind_helpers.h" +#include "base/location.h" #include "base/logging.h" #include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" @@ -67,6 +68,8 @@ class DaemonProcessWin : public DaemonProcess { int terminal_id, const DesktopSessionParams& params, bool virtual_terminal) OVERRIDE; + virtual void DoCrashNetworkProcess( + const tracked_objects::Location& location) OVERRIDE; virtual void LaunchNetworkProcess() OVERRIDE; private: @@ -153,6 +156,13 @@ scoped_ptr<DesktopSession> DaemonProcessWin::DoCreateDesktopSession( params, virtual_terminal, HostService::GetInstance())); } +void DaemonProcessWin::DoCrashNetworkProcess( + const tracked_objects::Location& location) { + DCHECK(caller_task_runner()->BelongsToCurrentThread()); + + network_launcher_->Crash(location); +} + void DaemonProcessWin::LaunchNetworkProcess() { DCHECK(caller_task_runner()->BelongsToCurrentThread()); DCHECK(!network_launcher_); diff --git a/remoting/host/desktop_process.cc b/remoting/host/desktop_process.cc index f740a2b..9e2fc75 100644 --- a/remoting/host/desktop_process.cc +++ b/remoting/host/desktop_process.cc @@ -9,9 +9,11 @@ #include "base/bind.h" #include "base/bind_helpers.h" +#include "base/debug/alias.h" #include "base/logging.h" #include "base/memory/ref_counted.h" #include "base/message_loop.h" +#include "base/string_util.h" #include "ipc/ipc_channel_proxy.h" #include "remoting/base/auto_thread.h" #include "remoting/base/auto_thread_task_runner.h" @@ -58,7 +60,7 @@ bool DesktopProcess::OnMessageReceived(const IPC::Message& message) { bool handled = true; IPC_BEGIN_MESSAGE_MAP(DesktopProcess, message) - IPC_MESSAGE_HANDLER(ChromotingDaemonDesktopMsg_Crash, OnCrash) + IPC_MESSAGE_HANDLER(ChromotingDaemonMsg_Crash, OnCrash) IPC_MESSAGE_UNHANDLED(handled = false) IPC_END_MESSAGE_MAP() @@ -149,8 +151,14 @@ bool DesktopProcess::Start( void DesktopProcess::OnCrash(const std::string& function_name, const std::string& file_name, const int& line_number) { + char message[1024]; + base::snprintf(message, sizeof(message), + "Requested by %s at %s, line %d.", + function_name.c_str(), file_name.c_str(), line_number); + base::debug::Alias(message); + // The daemon requested us to crash the process. - CHECK(false); + CHECK(false) << message; } } // namespace remoting diff --git a/remoting/host/desktop_process.h b/remoting/host/desktop_process.h index b7e5e17..8db886e 100644 --- a/remoting/host/desktop_process.h +++ b/remoting/host/desktop_process.h @@ -55,7 +55,7 @@ class DesktopProcess : public DesktopSessionAgent::Delegate, private: // Crashes the process in response to a daemon's request. The daemon passes // the location of the code that detected the fatal error resulted in this - // request. See the declaration of ChromotingDaemonDesktopMsg_Crash message. + // request. See the declaration of ChromotingDaemonMsg_Crash message. void OnCrash(const std::string& function_name, const std::string& file_name, const int& line_number); diff --git a/remoting/host/desktop_process_unittest.cc b/remoting/host/desktop_process_unittest.cc index de639f1..a299e47 100644 --- a/remoting/host/desktop_process_unittest.cc +++ b/remoting/host/desktop_process_unittest.cc @@ -281,7 +281,7 @@ void DesktopProcessTest::RunDeathTest() { void DesktopProcessTest::SendCrashRequest() { tracked_objects::Location location = FROM_HERE; - daemon_channel_->Send(new ChromotingDaemonDesktopMsg_Crash( + daemon_channel_->Send(new ChromotingDaemonMsg_Crash( location.function_name(), location.file_name(), location.line_number())); } diff --git a/remoting/host/desktop_session_win.cc b/remoting/host/desktop_session_win.cc index 10fb011..fdefc28 100644 --- a/remoting/host/desktop_session_win.cc +++ b/remoting/host/desktop_session_win.cc @@ -67,7 +67,7 @@ void DesktopSessionWin::OnChannelConnected(int32 peer_pid) { // from the desktop process. desktop_process_.Set(OpenProcess(PROCESS_DUP_HANDLE, false, peer_pid)); if (!desktop_process_.IsValid()) { - RestartDesktopProcess(FROM_HERE); + CrashDesktopProcess(FROM_HERE); return; } @@ -88,7 +88,7 @@ bool DesktopSessionWin::OnMessageReceived(const IPC::Message& message) { if (!handled) { LOG(ERROR) << "Received unexpected IPC type: " << message.type(); - RestartDesktopProcess(FROM_HERE); + CrashDesktopProcess(FROM_HERE); } return handled; @@ -143,7 +143,7 @@ void DesktopSessionWin::OnDesktopSessionAgentAttached( if (!daemon_process()->OnDesktopSessionAgentAttached(id(), desktop_process_, desktop_pipe)) { - RestartDesktopProcess(FROM_HERE); + CrashDesktopProcess(FROM_HERE); } } @@ -161,12 +161,11 @@ void DesktopSessionWin::OnInjectSas() { LOG(ERROR) << "Failed to inject Secure Attention Sequence."; } -void DesktopSessionWin::RestartDesktopProcess( +void DesktopSessionWin::CrashDesktopProcess( const tracked_objects::Location& location) { DCHECK(main_task_runner_->BelongsToCurrentThread()); - launcher_->Send(new ChromotingDaemonDesktopMsg_Crash( - location.function_name(), location.file_name(), location.line_number())); + launcher_->Crash(location); } } // namespace remoting diff --git a/remoting/host/desktop_session_win.h b/remoting/host/desktop_session_win.h index 976da9e..d174019 100644 --- a/remoting/host/desktop_session_win.h +++ b/remoting/host/desktop_session_win.h @@ -67,8 +67,8 @@ class DesktopSessionWin // ChromotingDesktopDaemonMsg_InjectSas handler. void OnInjectSas(); - // Restarts the desktop process. - void RestartDesktopProcess(const tracked_objects::Location& location); + // Requests the desktop process to crash. + void CrashDesktopProcess(const tracked_objects::Location& location); // Task runner on which public methods of this class should be called. scoped_refptr<AutoThreadTaskRunner> main_task_runner_; diff --git a/remoting/host/remoting_me2me_host.cc b/remoting/host/remoting_me2me_host.cc index 35289fa..d243839 100644 --- a/remoting/host/remoting_me2me_host.cc +++ b/remoting/host/remoting_me2me_host.cc @@ -10,6 +10,7 @@ #include "base/bind.h" #include "base/callback.h" #include "base/command_line.h" +#include "base/debug/alias.h" #include "base/file_util.h" #include "base/files/file_path.h" #include "base/logging.h" @@ -18,6 +19,7 @@ #include "base/single_thread_task_runner.h" #include "base/string_number_conversions.h" #include "base/string_util.h" +#include "base/string_util.h" #include "base/synchronization/waitable_event.h" #include "base/threading/thread.h" #include "base/utf_string_conversions.h" @@ -535,8 +537,7 @@ bool HostProcess::OnMessageReceived(const IPC::Message& message) { #if defined(REMOTING_MULTI_PROCESS) bool handled = true; IPC_BEGIN_MESSAGE_MAP(HostProcess, message) - IPC_MESSAGE_HANDLER(ChromotingDaemonNetworkMsg_Crash, - OnCrash) + IPC_MESSAGE_HANDLER(ChromotingDaemonMsg_Crash, OnCrash) IPC_MESSAGE_HANDLER(ChromotingDaemonNetworkMsg_Configuration, OnConfigUpdated) IPC_MESSAGE_FORWARD( @@ -1102,7 +1103,14 @@ void HostProcess::ShutdownOnNetworkThread() { void HostProcess::OnCrash(const std::string& function_name, const std::string& file_name, const int& line_number) { - CHECK(false); + char message[1024]; + base::snprintf(message, sizeof(message), + "Requested by %s at %s, line %d.", + function_name.c_str(), file_name.c_str(), line_number); + base::debug::Alias(message); + + // The daemon requested us to crash the process. + CHECK(false) << message; } int HostProcessMain() { diff --git a/remoting/host/win/unprivileged_process_delegate.cc b/remoting/host/win/unprivileged_process_delegate.cc index 2a5d20e..ef597d1 100644 --- a/remoting/host/win/unprivileged_process_delegate.cc +++ b/remoting/host/win/unprivileged_process_delegate.cc @@ -232,6 +232,12 @@ bool UnprivilegedProcessDelegate::Send(IPC::Message* message) { return channel_->Send(message); } +void UnprivilegedProcessDelegate::CloseChannel() { + DCHECK(main_task_runner_->BelongsToCurrentThread()); + + channel_.reset(); +} + DWORD UnprivilegedProcessDelegate::GetProcessId() const { if (worker_process_.IsValid()) { return ::GetProcessId(worker_process_); diff --git a/remoting/host/win/unprivileged_process_delegate.h b/remoting/host/win/unprivileged_process_delegate.h index bdf851b..f256c6f 100644 --- a/remoting/host/win/unprivileged_process_delegate.h +++ b/remoting/host/win/unprivileged_process_delegate.h @@ -41,6 +41,7 @@ class UnprivilegedProcessDelegate : public WorkerProcessLauncher::Delegate { virtual bool Send(IPC::Message* message) OVERRIDE; // WorkerProcessLauncher::Delegate implementation. + virtual void CloseChannel() OVERRIDE; virtual DWORD GetProcessId() const OVERRIDE; virtual bool IsPermanentError(int failure_count) const OVERRIDE; virtual void KillProcess(DWORD exit_code) OVERRIDE; diff --git a/remoting/host/win/worker_process_launcher.cc b/remoting/host/win/worker_process_launcher.cc index 6ca563c..704ebcd 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,8 @@ const net::BackoffEntry::Policy kDefaultBackoffPolicy = { false, }; +const int kKillProcessTimeoutSeconds = 5; +const int kLaunchSuccessTimeoutSeconds = 2; namespace remoting { @@ -52,7 +56,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,11 +79,18 @@ 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); + // Used to emulate |launch_success_timer_| expiration. + void ResetLaunchSuccessTimeoutForTest(); + + // Set the desired timeout for |kill_process_timer_|. + void SetKillProcessTimeoutForTest(const base::TimeDelta& timeout); + // base::win::ObjectWatcher::Delegate implementation. virtual void OnObjectSignaled(HANDLE object) OVERRIDE; @@ -88,8 +99,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); + private: - friend class base::RefCountedThreadSafe<Core>; + friend class base::DeleteHelper<Core>; virtual ~Core(); // Attempts to launch the worker process. Schedules next launch attempt if @@ -119,18 +135,21 @@ class WorkerProcessLauncher::Core // 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_; + // The timer used to delay termination of the worker process when an IPC error + // occured or when Crash() request is pending + base::OneShotTimer<Core> kill_process_timer_; + + // The default timeout for |kill_process_timer_|. + base::TimeDelta kill_process_timeout_; // 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_; @@ -157,14 +176,11 @@ WorkerProcessLauncher::Core::Core( worker_delegate_(worker_delegate), get_named_pipe_client_pid_(NULL), ipc_enabled_(false), + kill_process_timeout_( + base::TimeDelta::FromSeconds(kKillProcessTimeoutSeconds)), 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 +200,28 @@ 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 (!kill_process_timer_.IsRunning()) { + kill_process_timer_.Start(FROM_HERE, kill_process_timeout_, this, + &Core::StopWorker); + } +} + void WorkerProcessLauncher::Core::Send(IPC::Message* message) { DCHECK(caller_task_runner_->BelongsToCurrentThread()); @@ -194,6 +232,22 @@ void WorkerProcessLauncher::Core::Send(IPC::Message* message) { } } +void WorkerProcessLauncher::Core::ResetLaunchSuccessTimeoutForTest() { + DCHECK(caller_task_runner_->BelongsToCurrentThread()); + + if (launch_success_timer_.IsRunning()) { + launch_success_timer_.Stop(); + RecordSuccessfulLaunch(); + } +} + +void WorkerProcessLauncher::Core::SetKillProcessTimeoutForTest( + const base::TimeDelta& timeout) { + DCHECK(caller_task_runner_->BelongsToCurrentThread()); + + kill_process_timeout_ = timeout; +} + void WorkerProcessLauncher::Core::OnObjectSignaled(HANDLE object) { DCHECK(caller_task_runner_->BelongsToCurrentThread()); DCHECK(process_watcher_.GetWatchedObject() == NULL); @@ -241,19 +295,27 @@ 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 (!kill_process_timer_.IsRunning()) { + kill_process_timer_.Start(FROM_HERE, kill_process_timeout_, 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 +326,9 @@ 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(kLaunchSuccessTimeoutSeconds), + this, &Core::RecordSuccessfulLaunch); return; } @@ -289,8 +352,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 +373,20 @@ void WorkerProcessLauncher::Core::StopWorker() { process_watcher_.StopWatching(); } - ipc_error_timer_->Stop(); + kill_process_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,8 +404,22 @@ WorkerProcessLauncher::~WorkerProcessLauncher() { core_ = NULL; } +void WorkerProcessLauncher::Crash(const tracked_objects::Location& location) { + core_->Crash(location); +} + void WorkerProcessLauncher::Send(IPC::Message* message) { core_->Send(message); } +void WorkerProcessLauncher::ResetLaunchSuccessTimeoutForTest() { + core_->ResetLaunchSuccessTimeoutForTest(); +} + +void WorkerProcessLauncher::SetKillProcessTimeoutForTest( + const base::TimeDelta& timeout) { + core_->SetKillProcessTimeoutForTest(timeout); +} + + } // namespace remoting diff --git a/remoting/host/win/worker_process_launcher.h b/remoting/host/win/worker_process_launcher.h index 1eea8cd..4f3711f 100644 --- a/remoting/host/win/worker_process_launcher.h +++ b/remoting/host/win/worker_process_launcher.h @@ -14,6 +14,7 @@ namespace base { class SingleThreadTaskRunner; +class TimeDelta; } // namespace base namespace IPC { @@ -21,6 +22,10 @@ class Listener; class Message; } // namespace IPC +namespace tracked_objects { +class Location; +} // namespace tracked_objects + namespace remoting { class WorkerProcessIpcDelegate; @@ -35,6 +40,9 @@ class WorkerProcessLauncher { public: virtual ~Delegate(); + // 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; @@ -68,12 +76,24 @@ class WorkerProcessLauncher { WorkerProcessIpcDelegate* worker_delegate); ~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); private: + friend class WorkerProcessLauncherTest; + + // Hooks that allow test code to call the corresponding methods of |Core|. + void ResetLaunchSuccessTimeoutForTest(); + void SetKillProcessTimeoutForTest(const base::TimeDelta& timeout); + // The actual implementation resides in WorkerProcessLauncher::Core class. class Core; scoped_refptr<Core> core_; diff --git a/remoting/host/win/worker_process_launcher_unittest.cc b/remoting/host/win/worker_process_launcher_unittest.cc index c5a6c85..e106179 100644 --- a/remoting/host/win/worker_process_launcher_unittest.cc +++ b/remoting/host/win/worker_process_launcher_unittest.cc @@ -12,12 +12,13 @@ #include "ipc/ipc_listener.h" #include "ipc/ipc_message.h" #include "remoting/base/auto_thread_task_runner.h" +#include "remoting/host/chromoting_messages.h" #include "remoting/host/host_exit_codes.h" #include "remoting/host/win/launch_process_with_token.h" #include "remoting/host/win/worker_process_launcher.h" #include "remoting/host/worker_process_ipc_delegate.h" -#include "testing/gmock_mutant.h" #include "testing/gmock/include/gmock/gmock.h" +#include "testing/gmock_mutant.h" #include "testing/gtest/include/gtest/gtest.h" using base::win::ScopedHandle; @@ -37,27 +38,18 @@ namespace { const char kIpcSecurityDescriptor[] = "D:(A;;GA;;;AU)"; -class MockProcessLauncherDelegate - : public WorkerProcessLauncher::Delegate { +class MockProcessLauncherDelegate : public WorkerProcessLauncher::Delegate { public: MockProcessLauncherDelegate() {} virtual ~MockProcessLauncherDelegate() {} - virtual DWORD GetProcessId() const OVERRIDE { - return const_cast<MockProcessLauncherDelegate*>(this)->GetProcessId(); - } - - virtual bool IsPermanentError(int failure_count) const OVERRIDE { - return const_cast<MockProcessLauncherDelegate*>(this)->IsPermanentError( - failure_count); - } - // IPC::Sender implementation. MOCK_METHOD1(Send, bool(IPC::Message*)); - // WorkerProcessLauncher::Delegate implementation - MOCK_METHOD0(GetProcessId, DWORD()); - MOCK_METHOD1(IsPermanentError, bool(int)); + // WorkerProcessLauncher::Delegate interface. + MOCK_METHOD0(CloseChannel, void()); + MOCK_CONST_METHOD0(GetProcessId, DWORD()); + MOCK_CONST_METHOD1(IsPermanentError, bool(int)); MOCK_METHOD1(KillProcess, void(DWORD)); MOCK_METHOD2(LaunchProcess, bool(IPC::Listener*, ScopedHandle*)); @@ -65,13 +57,12 @@ class MockProcessLauncherDelegate DISALLOW_COPY_AND_ASSIGN(MockProcessLauncherDelegate); }; -class MockIpcDelegate - : public WorkerProcessIpcDelegate { +class MockIpcDelegate : public WorkerProcessIpcDelegate { public: MockIpcDelegate() {} virtual ~MockIpcDelegate() {} - // WorkerProcessIpcDelegate implementation + // WorkerProcessIpcDelegate interface. MOCK_METHOD1(OnChannelConnected, void(int32)); MOCK_METHOD1(OnMessageReceived, bool(const IPC::Message&)); MOCK_METHOD0(OnPermanentError, void()); @@ -80,11 +71,35 @@ class MockIpcDelegate DISALLOW_COPY_AND_ASSIGN(MockIpcDelegate); }; +class MockWorkerListener : public IPC::Listener { + public: + MockWorkerListener() {} + virtual ~MockWorkerListener() {} + + MOCK_METHOD3(OnCrash, void(const std::string&, const std::string&, int)); + + // IPC::Listener implementation + virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE; + + private: + DISALLOW_COPY_AND_ASSIGN(MockWorkerListener); +}; + +bool MockWorkerListener::OnMessageReceived(const IPC::Message& message) { + bool handled = true; + IPC_BEGIN_MESSAGE_MAP(MockWorkerListener, message) + IPC_MESSAGE_HANDLER(ChromotingDaemonMsg_Crash, OnCrash) + IPC_MESSAGE_UNHANDLED(handled = false) + IPC_END_MESSAGE_MAP() + + EXPECT_TRUE(handled); + + return handled; +} + } // namespace -class WorkerProcessLauncherTest - : public testing::Test, - public IPC::Listener { +class WorkerProcessLauncherTest : public testing::Test { public: WorkerProcessLauncherTest(); virtual ~WorkerProcessLauncherTest(); @@ -92,9 +107,6 @@ class WorkerProcessLauncherTest virtual void SetUp() OVERRIDE; virtual void TearDown() OVERRIDE; - // IPC::Listener implementation - virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE; - // WorkerProcessLauncher::Delegate mocks void KillProcess(DWORD exit_code); bool LaunchProcess(IPC::Listener* delegate, @@ -104,8 +116,23 @@ class WorkerProcessLauncherTest bool FailLaunchAndStopWorker(IPC::Listener* delegate, ScopedHandle* process_exit_event_out); - void ConnectChannel(); - void DisconnectChannel(); + // Connects the client end of the channel (the worker process's end). + void ConnectClient(); + + // Disconnects the client end of the channel. + void DisconnectClient(); + + // Disconnects the server end of the channel (the launcher's end). + void DisconnectServer(); + + // Sends a message to the worker process. + bool SendToProcess(IPC::Message* message); + + // Sends a fake message to the launcher. + void SendFakeMessageToLauncher(); + + // Requests the worker to crash. + void CrashWorker(); // Starts the worker. void StartWorker(); @@ -120,7 +147,13 @@ class WorkerProcessLauncherTest MessageLoop message_loop_; scoped_refptr<AutoThreadTaskRunner> task_runner_; - MockIpcDelegate ipc_delegate_; + // Receives messages sent to the worker process. + MockWorkerListener client_listener_; + + // Receives messages sent from the worker process. + MockIpcDelegate server_listener_; + + // Implements WorkerProcessLauncher::Delegate. scoped_ptr<MockProcessLauncherDelegate> launcher_delegate_; // The name of the IPC channel. @@ -163,7 +196,11 @@ void WorkerProcessLauncherTest::SetUp() { launcher_delegate_.reset(new MockProcessLauncherDelegate()); EXPECT_CALL(*launcher_delegate_, Send(_)) .Times(AnyNumber()) - .WillRepeatedly(Return(false)); + .WillRepeatedly(Invoke(this, &WorkerProcessLauncherTest::SendToProcess)); + EXPECT_CALL(*launcher_delegate_, CloseChannel()) + .Times(AnyNumber()) + .WillRepeatedly(Invoke(this, + &WorkerProcessLauncherTest::DisconnectServer)); EXPECT_CALL(*launcher_delegate_, GetProcessId()) .Times(AnyNumber()) .WillRepeatedly(ReturnPointee(&client_pid_)); @@ -175,18 +212,13 @@ void WorkerProcessLauncherTest::SetUp() { .WillRepeatedly(Invoke(this, &WorkerProcessLauncherTest::KillProcess)); // Set up IPC delegate. - EXPECT_CALL(ipc_delegate_, OnMessageReceived(_)) - .Times(AnyNumber()) - .WillRepeatedly(Return(false)); + EXPECT_CALL(server_listener_, OnMessageReceived(_)) + .Times(0); } void WorkerProcessLauncherTest::TearDown() { } -bool WorkerProcessLauncherTest::OnMessageReceived(const IPC::Message& message) { - return false; -} - void WorkerProcessLauncherTest::KillProcess(DWORD exit_code) { BOOL result = SetEvent(process_exit_event_); EXPECT_TRUE(result); @@ -229,7 +261,7 @@ bool WorkerProcessLauncherTest::LaunchProcessAndConnect( task_runner_->PostTask( FROM_HERE, - base::Bind(&WorkerProcessLauncherTest::ConnectChannel, + base::Bind(&WorkerProcessLauncherTest::ConnectClient, base::Unretained(this))); return true; } @@ -244,26 +276,54 @@ bool WorkerProcessLauncherTest::FailLaunchAndStopWorker( return false; } -void WorkerProcessLauncherTest::ConnectChannel() { +void WorkerProcessLauncherTest::ConnectClient() { channel_client_.reset(new IPC::ChannelProxy( IPC::ChannelHandle(channel_name_), IPC::Channel::MODE_CLIENT, - this, + &client_listener_, task_runner_)); + + // Pretend that |kLaunchSuccessTimeoutSeconds| passed since launching + // the worker process. This will make the backoff algorithm think that this + // launch attempt was successful and it will not delay the next launch. + launcher_->ResetLaunchSuccessTimeoutForTest(); } -void WorkerProcessLauncherTest::DisconnectChannel() { +void WorkerProcessLauncherTest::DisconnectClient() { channel_client_.reset(); } +void WorkerProcessLauncherTest::DisconnectServer() { + channel_server_.reset(); +} + +bool WorkerProcessLauncherTest::SendToProcess(IPC::Message* message) { + if (channel_server_) + return channel_server_->Send(message); + + delete message; + return false; +} + +void WorkerProcessLauncherTest::SendFakeMessageToLauncher() { + if (channel_client_) + channel_client_->Send(new ChromotingDesktopNetworkMsg_DisconnectSession()); +} + +void WorkerProcessLauncherTest::CrashWorker() { + launcher_->Crash(FROM_HERE); +} + void WorkerProcessLauncherTest::StartWorker() { launcher_.reset(new WorkerProcessLauncher( - task_runner_, launcher_delegate_.Pass(), &ipc_delegate_)); + task_runner_, launcher_delegate_.Pass(), &server_listener_)); + + launcher_->SetKillProcessTimeoutForTest(base::TimeDelta::FromMilliseconds(0)); } void WorkerProcessLauncherTest::StopWorker() { launcher_.reset(); - DisconnectChannel(); + DisconnectClient(); channel_name_.clear(); channel_server_.reset(); task_runner_ = NULL; @@ -278,9 +338,9 @@ TEST_F(WorkerProcessLauncherTest, Start) { .Times(1) .WillRepeatedly(Invoke(this, &WorkerProcessLauncherTest::LaunchProcess)); - EXPECT_CALL(ipc_delegate_, OnChannelConnected(_)) + EXPECT_CALL(server_listener_, OnChannelConnected(_)) .Times(0); - EXPECT_CALL(ipc_delegate_, OnPermanentError()) + EXPECT_CALL(server_listener_, OnPermanentError()) .Times(0); StartWorker(); @@ -296,11 +356,11 @@ TEST_F(WorkerProcessLauncherTest, StartAndConnect) { .WillRepeatedly(Invoke( this, &WorkerProcessLauncherTest::LaunchProcessAndConnect)); - EXPECT_CALL(ipc_delegate_, OnChannelConnected(_)) + EXPECT_CALL(server_listener_, OnChannelConnected(_)) .Times(1) .WillOnce(InvokeWithoutArgs(this, &WorkerProcessLauncherTest::StopWorker)); - EXPECT_CALL(ipc_delegate_, OnPermanentError()) + EXPECT_CALL(server_listener_, OnPermanentError()) .Times(0); StartWorker(); @@ -315,14 +375,14 @@ TEST_F(WorkerProcessLauncherTest, Restart) { .WillRepeatedly(Invoke( this, &WorkerProcessLauncherTest::LaunchProcessAndConnect)); Expectation first_connect = - EXPECT_CALL(ipc_delegate_, OnChannelConnected(_)) + EXPECT_CALL(server_listener_, OnChannelConnected(_)) .Times(2) .WillOnce(InvokeWithoutArgs(CreateFunctor( this, &WorkerProcessLauncherTest::KillProcess, CONTROL_C_EXIT))) .WillOnce(InvokeWithoutArgs(this, &WorkerProcessLauncherTest::StopWorker)); - EXPECT_CALL(ipc_delegate_, OnPermanentError()) + EXPECT_CALL(server_listener_, OnPermanentError()) .Times(0); StartWorker(); @@ -338,14 +398,14 @@ TEST_F(WorkerProcessLauncherTest, DropIpcChannel) { this, &WorkerProcessLauncherTest::LaunchProcessAndConnect)); Expectation first_connect = - EXPECT_CALL(ipc_delegate_, OnChannelConnected(_)) + EXPECT_CALL(server_listener_, OnChannelConnected(_)) .Times(2) .WillOnce(InvokeWithoutArgs( - this, &WorkerProcessLauncherTest::DisconnectChannel)) + this, &WorkerProcessLauncherTest::DisconnectClient)) .WillOnce(InvokeWithoutArgs( this, &WorkerProcessLauncherTest::StopWorker)); - EXPECT_CALL(ipc_delegate_, OnPermanentError()) + EXPECT_CALL(server_listener_, OnPermanentError()) .Times(0); StartWorker(); @@ -360,11 +420,11 @@ TEST_F(WorkerProcessLauncherTest, PermanentError) { .WillRepeatedly(Invoke( this, &WorkerProcessLauncherTest::LaunchProcessAndConnect)); - EXPECT_CALL(ipc_delegate_, OnChannelConnected(_)) + EXPECT_CALL(server_listener_, OnChannelConnected(_)) .Times(1) .WillOnce(InvokeWithoutArgs(CreateFunctor( this, &WorkerProcessLauncherTest::KillProcess, CONTROL_C_EXIT))); - EXPECT_CALL(ipc_delegate_, OnPermanentError()) + EXPECT_CALL(server_listener_, OnPermanentError()) .Times(1) .WillOnce(InvokeWithoutArgs(this, &WorkerProcessLauncherTest::StopWorker)); @@ -383,9 +443,9 @@ TEST_F(WorkerProcessLauncherTest, InvalidClientPid) { .WillOnce(Invoke( this, &WorkerProcessLauncherTest::FailLaunchAndStopWorker)); - EXPECT_CALL(ipc_delegate_, OnChannelConnected(_)) + EXPECT_CALL(server_listener_, OnChannelConnected(_)) .Times(0); - EXPECT_CALL(ipc_delegate_, OnPermanentError()) + EXPECT_CALL(server_listener_, OnPermanentError()) .Times(0); client_pid_ = GetCurrentProcessId() + 4; @@ -393,4 +453,53 @@ TEST_F(WorkerProcessLauncherTest, InvalidClientPid) { message_loop_.Run(); } +// Requests the worker to crash and expects it to honor the request. +TEST_F(WorkerProcessLauncherTest, Crash) { + EXPECT_CALL(*launcher_delegate_, LaunchProcess(_, _)) + .Times(2) + .WillRepeatedly(Invoke( + this, &WorkerProcessLauncherTest::LaunchProcessAndConnect)); + + EXPECT_CALL(server_listener_, OnChannelConnected(_)) + .Times(2) + .WillOnce(InvokeWithoutArgs(this, + &WorkerProcessLauncherTest::CrashWorker)) + .WillOnce(InvokeWithoutArgs(this, + &WorkerProcessLauncherTest::StopWorker)); + + EXPECT_CALL(client_listener_, OnCrash(_, _, _)) + .Times(1) + .WillOnce(InvokeWithoutArgs(CreateFunctor( + this, &WorkerProcessLauncherTest::KillProcess, + EXCEPTION_BREAKPOINT))); + + StartWorker(); + message_loop_.Run(); +} + +// Requests the worker to crash and terminates the worker even if it does not +// comply. +TEST_F(WorkerProcessLauncherTest, CrashAnyway) { + EXPECT_CALL(*launcher_delegate_, LaunchProcess(_, _)) + .Times(2) + .WillRepeatedly(Invoke( + this, &WorkerProcessLauncherTest::LaunchProcessAndConnect)); + + EXPECT_CALL(server_listener_, OnChannelConnected(_)) + .Times(2) + .WillOnce(InvokeWithoutArgs(this, + &WorkerProcessLauncherTest::CrashWorker)) + .WillOnce(InvokeWithoutArgs(this, + &WorkerProcessLauncherTest::StopWorker)); + + // Ignore the crash request and try send another message to the launcher. + EXPECT_CALL(client_listener_, OnCrash(_, _, _)) + .Times(1) + .WillOnce(InvokeWithoutArgs( + this, &WorkerProcessLauncherTest::SendFakeMessageToLauncher)); + + StartWorker(); + message_loop_.Run(); +} + } // namespace remoting diff --git a/remoting/host/win/wts_session_process_delegate.cc b/remoting/host/win/wts_session_process_delegate.cc index 74659f7..434e5df 100644 --- a/remoting/host/win/wts_session_process_delegate.cc +++ b/remoting/host/win/wts_session_process_delegate.cc @@ -62,6 +62,7 @@ class WtsSessionProcessDelegate::Core virtual bool Send(IPC::Message* message) OVERRIDE; // WorkerProcessLauncher::Delegate implementation. + virtual void CloseChannel() OVERRIDE; virtual DWORD GetProcessId() const OVERRIDE; virtual bool IsPermanentError(int failure_count) const OVERRIDE; virtual void KillProcess(DWORD exit_code) OVERRIDE; @@ -173,7 +174,7 @@ void WtsSessionProcessDelegate::Core::OnIOCompleted( bool WtsSessionProcessDelegate::Core::Send(IPC::Message* message) { DCHECK(main_task_runner_->BelongsToCurrentThread()); - if (channel_.get()) { + if (channel_) { return channel_->Send(message); } else { delete message; @@ -181,6 +182,13 @@ bool WtsSessionProcessDelegate::Core::Send(IPC::Message* message) { } } +void WtsSessionProcessDelegate::Core::CloseChannel() { + DCHECK(main_task_runner_->BelongsToCurrentThread()); + + channel_.reset(); + pipe_.Close(); +} + DWORD WtsSessionProcessDelegate::Core::GetProcessId() const { DWORD pid = 0; if (launch_elevated_ && pipe_.IsValid() && @@ -496,6 +504,11 @@ bool WtsSessionProcessDelegate::Send(IPC::Message* message) { return core_->Send(message); } +void WtsSessionProcessDelegate::CloseChannel() { + if (core_) + core_->CloseChannel(); +} + DWORD WtsSessionProcessDelegate::GetProcessId() const { if (!core_) return 0; diff --git a/remoting/host/win/wts_session_process_delegate.h b/remoting/host/win/wts_session_process_delegate.h index 7c637c1..7529c88 100644 --- a/remoting/host/win/wts_session_process_delegate.h +++ b/remoting/host/win/wts_session_process_delegate.h @@ -43,6 +43,7 @@ class WtsSessionProcessDelegate virtual bool Send(IPC::Message* message) OVERRIDE; // WorkerProcessLauncher::Delegate implementation. + virtual void CloseChannel() OVERRIDE; virtual DWORD GetProcessId() const OVERRIDE; virtual bool IsPermanentError(int failure_count) const OVERRIDE; virtual void KillProcess(DWORD exit_code) OVERRIDE; |