Adding a unit test to verify the IPC channel between the network and desktop processes.

remoting/host/desktop_session_proxy.cc

 // 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.
 
 #include "remoting/host/desktop_session_proxy.h"
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/platform_file.h"
+#include "base/process_util.h"
 #include "base/single_thread_task_runner.h"
 #include "ipc/ipc_channel_proxy.h"
 #include "ipc/ipc_message_macros.h"
 #include "media/video/capture/screen/screen_capture_data.h"
 #include "remoting/host/audio_capturer.h"
 #include "remoting/host/chromoting_messages.h"
 #include "remoting/host/client_session.h"
 #include "remoting/host/ipc_audio_capturer.h"
 #include "remoting/host/ipc_event_executor.h"
 #include "remoting/host/ipc_video_frame_capturer.h"
 #include "remoting/proto/audio.pb.h"
 #include "remoting/proto/control.pb.h"
 #include "remoting/proto/event.pb.h"
 
 #if defined(OS_WIN)
 #include "base/win/scoped_handle.h"
 #endif  // defined(OS_WIN)
 
 namespace remoting {
 
 DesktopSessionProxy::DesktopSessionProxy(
     scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     const std::string& client_jid,
     const base::Closure& disconnect_callback)
     : caller_task_runner_(caller_task_runner),
+      io_task_runner_(io_task_runner),
       audio_capturer_(NULL),
       client_jid_(client_jid),
       disconnect_callback_(disconnect_callback),
+      desktop_process_(base::kNullProcessHandle),
       pending_capture_frame_requests_(0),
       video_capturer_(NULL) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
   DCHECK(!client_jid_.empty());
   DCHECK(!disconnect_callback_.is_null());
 }
 
 scoped_ptr<AudioCapturer> DesktopSessionProxy::CreateAudioCapturer(
     scoped_refptr<base::SingleThreadTaskRunner> audio_task_runner) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
@@ skipping 51 matching lines @@
   VLOG(1) << "IPC: network <- desktop (" << peer_pid << ")";
 }
 
 void DesktopSessionProxy::OnChannelError() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
   DetachFromDesktop();
 }
 
 bool DesktopSessionProxy::AttachToDesktop(
-    IPC::PlatformFileForTransit desktop_process,
+    base::ProcessHandle desktop_process,
     IPC::PlatformFileForTransit desktop_pipe) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
   DCHECK(!client_jid_.empty());
   DCHECK(!desktop_channel_);
+  DCHECK(desktop_process_ == base::kNullProcessHandle);

[Sergey Ulanov, 2013/02/04 21:04:41]

nit: DCHECK_EQ

[alexeypa (please no reviews), 2013/02/04 23:45:58]

Done.

   DCHECK(!disconnect_callback_.is_null());
 
+  desktop_process_ = desktop_process;
+
 #if defined(OS_WIN)
   // On Windows: |desktop_process| is a valid handle, but |desktop_pipe| needs
   // to be duplicated from the desktop process.
-  desktop_process_.Set(desktop_process);
-
   base::win::ScopedHandle pipe;
   if (!DuplicateHandle(desktop_process_, desktop_pipe, GetCurrentProcess(),
                        pipe.Receive(), 0, FALSE, DUPLICATE_SAME_ACCESS)) {
     LOG_GETLASTERROR(ERROR) << "Failed to duplicate the desktop-to-network"
                                " pipe handle";
     return false;
   }
 
   // Connect to the desktop process.
   desktop_channel_.reset(new IPC::ChannelProxy(IPC::ChannelHandle(pipe),
                                                IPC::Channel::MODE_CLIENT,
                                                this,
-                                               caller_task_runner_));
+                                               io_task_runner_));
 #elif defined(OS_POSIX)
-  // On posix: both |desktop_process| and |desktop_pipe| are valid file
-  // descriptors.
-  DCHECK(desktop_process.auto_close);
+  // On posix: |desktop_pipe| is a valid file descriptor.
   DCHECK(desktop_pipe.auto_close);
 
-  base::ClosePlatformFile(desktop_process.fd);
-
   // Connect to the desktop process.
   desktop_channel_.reset(new IPC::ChannelProxy(

[Sergey Ulanov, 2013/02/04 21:04:41]

I think you can move this out of the ifdef. It's only ChannelHandle() creation
that's platform-specific here, so it will need to be kept in ifdef, but
everything elase is not.

[alexeypa (please no reviews), 2013/02/04 23:45:58]

Done.

       IPC::ChannelHandle("", desktop_pipe),
       IPC::Channel::MODE_CLIENT,
       this,
       caller_task_runner_));

[Sergey Ulanov, 2013/02/04 21:04:41]

Should this be changed to io_task_runner_ too?

[alexeypa (please no reviews), 2013/02/04 23:45:58]

Done.

 #else
 #error Unsupported platform.
 #endif
 
   // Pass ID of the client (which is authenticated at this point) to the desktop
   // session agent and start the agent.
   SendToDesktop(new ChromotingNetworkDesktopMsg_StartSessionAgent(client_jid_));
   return true;
 }
 
 void DesktopSessionProxy::DetachFromDesktop() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
   desktop_channel_.reset();
 
-#if defined(OS_WIN)
-  desktop_process_.Close();
-#endif  // defined(OS_WIN)
+  if (desktop_process_ != base::kNullProcessHandle) {
+    base::CloseProcessHandle(desktop_process_);
+    desktop_process_ = base::kNullProcessHandle;
+  }
 
   shared_buffers_.clear();
 
   // Generate fake responses to keep the video capturer in sync.
   while (pending_capture_frame_requests_) {
     --pending_capture_frame_requests_;
     PostCaptureCompleted(scoped_refptr<media::ScreenCaptureData>());
   }
 }
 
@@ skipping 11 matching lines @@
 }
 
 void DesktopSessionProxy::InvalidateRegion(const SkRegion& invalid_region) {
   if (!caller_task_runner_->BelongsToCurrentThread()) {
     caller_task_runner_->PostTask(
         FROM_HERE, base::Bind(&DesktopSessionProxy::InvalidateRegion, this,
                               invalid_region));
     return;
   }
 
-  std::vector<SkIRect> invalid_rects;
-  for (SkRegion::Iterator i(invalid_region); !i.done(); i.next())
-    invalid_rects.push_back(i.rect());
+  if (desktop_channel_) {
+    std::vector<SkIRect> invalid_rects;
+    for (SkRegion::Iterator i(invalid_region); !i.done(); i.next())
+      invalid_rects.push_back(i.rect());
 
-  SendToDesktop(
-      new ChromotingNetworkDesktopMsg_InvalidateRegion(invalid_rects));
+    SendToDesktop(
+        new ChromotingNetworkDesktopMsg_InvalidateRegion(invalid_rects));
+  }
 }
 
 void DesktopSessionProxy::CaptureFrame() {
   if (!caller_task_runner_->BelongsToCurrentThread()) {
     caller_task_runner_->PostTask(
         FROM_HERE, base::Bind(&DesktopSessionProxy::CaptureFrame, this));
     return;
   }
 
-  ++pending_capture_frame_requests_;
-  SendToDesktop(new ChromotingNetworkDesktopMsg_CaptureFrame());
+  if (desktop_channel_) {
+    ++pending_capture_frame_requests_;
+    SendToDesktop(new ChromotingNetworkDesktopMsg_CaptureFrame());
+  } else {
+    PostCaptureCompleted(scoped_refptr<media::ScreenCaptureData>());

[Sergey Ulanov, 2013/02/04 21:04:41]

Why do we need to do this? I don't see anything in IpcVideoFrameCapturer that
can handle the case when capture_data is NULL.

[alexeypa (please no reviews), 2013/02/04 23:45:58]

VideoFrameCapturer::Delegate requires OnCaptureCompleted() to be called as many
time as CaptureFrame() was called. VideoScheduler relies on this to shutdown
correctly.

+  }
 }
 
 void DesktopSessionProxy::StartVideoCapturer(
     IpcVideoFrameCapturer* video_capturer) {
   DCHECK(video_capture_task_runner_->BelongsToCurrentThread());
   DCHECK(video_capturer_ == NULL);
 
   video_capturer_ = video_capturer;
 }
 
@@ skipping 51 matching lines @@
 }
 
 void DesktopSessionProxy::StartEventExecutor(
     scoped_ptr<protocol::ClipboardStub> client_clipboard) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
   client_clipboard_ = client_clipboard.Pass();
 }
 
 DesktopSessionProxy::~DesktopSessionProxy() {
+  if (desktop_process_ != base::kNullProcessHandle) {
+    base::CloseProcessHandle(desktop_process_);
+    desktop_process_ = base::kNullProcessHandle;
+  }
 }
 
 scoped_refptr<media::SharedBuffer> DesktopSessionProxy::GetSharedBuffer(
     int id) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
   SharedBuffers::const_iterator i = shared_buffers_.find(id);
   if (i != shared_buffers_.end()) {
     return i->second;
   } else {
@@ skipping 158 matching lines @@
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
   if (desktop_channel_) {
     desktop_channel_->Send(message);
   } else {
     delete message;
   }
 }
 
 }  // namespace remoting