Use ChannelMojo for remote security key channels.

remoting/host/security_key/security_key_ipc_client.cc

Index: remoting/host/security_key/security_key_ipc_client.cc
diff --git a/remoting/host/security_key/security_key_ipc_client.cc b/remoting/host/security_key/security_key_ipc_client.cc
index 98dcaf1fcf5c6fa900a1d65a91b463a0d863cca1..17b9309f792eb9b8d390236fa90539bee1bfc880 100644
--- a/remoting/host/security_key/security_key_ipc_client.cc
+++ b/remoting/host/security_key/security_key_ipc_client.cc
@@ -14,6 +14,8 @@
 #include "ipc/ipc_listener.h"
 #include "ipc/ipc_message.h"
 #include "ipc/ipc_message_macros.h"
+#include "mojo/edk/embedder/embedder.h"
+#include "mojo/edk/embedder/named_platform_handle_utils.h"
 #include "remoting/host/chromoting_messages.h"
 #include "remoting/host/ipc_constants.h"
 #include "remoting/host/security_key/security_key_ipc_constants.h"
@@ -21,7 +23,7 @@
 namespace remoting {
 
 SecurityKeyIpcClient::SecurityKeyIpcClient()
-    : initial_ipc_channel_name_(remoting::GetSecurityKeyIpcChannelName()),
+    : named_channel_handle_(remoting::GetSecurityKeyIpcChannel()),
       weak_factory_(this) {}
 
 SecurityKeyIpcClient::~SecurityKeyIpcClient() {}
@@ -29,26 +31,9 @@ SecurityKeyIpcClient::~SecurityKeyIpcClient() {}
 bool SecurityKeyIpcClient::WaitForSecurityKeyIpcServerChannel() {

[joedow, 2016/11/03 22:25:18]

Since this method doesn't wait anymore, it should be renamed from
WaitForSecurityKeyIpcServerChannel() to CheckForSecurityKeyIpcServerChannel().

Also, I think removing the wait should be OK but we should do some additional
testing to make sure. 

[Sam McNally, 2016/11/04 02:51:09]

On Windows, CreateClientHandle calls WaitNamedPipe to wait for the pipe to be
ready. This can wait up to the timeout (which defaults to 5s) if a pipe with the
right name exists, but all instances with that name are busy, or fail
immediately if no pipes with the right name exist.

The former could occur if a client connects but doesn't establish the
IPC::Channel over that connection (until the server times out and recreates the
pipe). It's possible that if a client disconnects before the channel for the
next connection is created, the old channel could be closed on the IO thread,
leaving a window of no pipe with the right name existing.

   DCHECK(thread_checker_.CalledOnValidThread());
 
-  // The retry loop is needed as the IPC Servers we connect to are reset (torn
-  // down and recreated) and we should be resilient in that case.  We need to
-  // strike a balance between resilience and speed as we do not want to add
-  // un-necessary delay to the local scenario when no session is active.
-  // 500ms was chosen as a reasonable balance between reliability of remote
-  // session detection and overhead added to the local security key operation
-  // when no remote session is present.
-  const base::TimeDelta kTotalWaitTime = base::TimeDelta::FromMilliseconds(500);
-  const base::TimeDelta kPerIterationWaitTime =
-      base::TimeDelta::FromMilliseconds(10);
-  const int kLoopIterations = kTotalWaitTime / kPerIterationWaitTime;
-  for (int i = 0; i < kLoopIterations; i++) {
-    if (IPC::Channel::IsNamedServerInitialized(initial_ipc_channel_name_)) {
-      return true;
-    }
-
-    base::PlatformThread::Sleep(kPerIterationWaitTime);
-  }
-
-  return false;
+  if (!channel_handle_.is_valid())
+    channel_handle_ = mojo::edk::CreateClientHandle(named_channel_handle_);

[joedow, 2016/11/03 22:25:18]

nit: Use braces for single line conditions here, and elsewhere in the CL.

[Sam McNally, 2016/11/04 02:51:09]

Done.

+  return channel_handle_.is_valid();
 }
 
 void SecurityKeyIpcClient::EstablishIpcConnection(
@@ -62,7 +47,7 @@ void SecurityKeyIpcClient::EstablishIpcConnection(
   connection_ready_callback_ = connection_ready_callback;
   connection_error_callback_ = connection_error_callback;
 
-  ConnectToIpcChannel(initial_ipc_channel_name_);
+  ConnectToIpcChannel();
 }
 
 bool SecurityKeyIpcClient::SendSecurityKeyRequest(
@@ -93,9 +78,9 @@ void SecurityKeyIpcClient::CloseIpcConnection() {
   ipc_channel_.reset();
 }
 
-void SecurityKeyIpcClient::SetInitialIpcChannelNameForTest(
-    const std::string& initial_ipc_channel_name) {
-  initial_ipc_channel_name_ = initial_ipc_channel_name;
+void SecurityKeyIpcClient::SetIpcChannelHandleForTest(
+    const mojo::edk::NamedPlatformHandle& channel_handle) {
+  named_channel_handle_ = channel_handle;
 }
 
 void SecurityKeyIpcClient::SetExpectedIpcServerSessionIdForTest(
@@ -108,9 +93,6 @@ bool SecurityKeyIpcClient::OnMessageReceived(const IPC::Message& message) {
 
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(SecurityKeyIpcClient, message)
-    IPC_MESSAGE_HANDLER(
-        ChromotingNetworkToRemoteSecurityKeyMsg_ConnectionDetails,
-        OnConnectionDetails)
     IPC_MESSAGE_HANDLER(ChromotingNetworkToRemoteSecurityKeyMsg_Response,
                         OnSecurityKeyResponse)
     IPC_MESSAGE_UNHANDLED(handled = false)
@@ -126,8 +108,7 @@ void SecurityKeyIpcClient::OnChannelConnected(int32_t peer_pid) {
 #if defined(OS_WIN)
   DWORD peer_session_id;
   if (!ProcessIdToSessionId(peer_pid, &peer_session_id)) {
-    uint32_t last_error = GetLastError();
-    LOG(ERROR) << "ProcessIdToSessionId failed with error code: " << last_error;
+    PLOG(ERROR) << "ProcessIdToSessionId failed";
     base::ResetAndReturn(&connection_error_callback_).Run();
     return;
   }
@@ -141,13 +122,7 @@ void SecurityKeyIpcClient::OnChannelConnected(int32_t peer_pid) {
   }
 #endif  // defined(OS_WIN)
 
-  // If we have received the connection details already (i.e.
-  // |ipc_channel_name_| is populated) then we signal that the connection is
-  // ready for use.  Otherwise this is the initial connection and we will wait
-  // to receive the ConnectionDetails message before proceeding.
-  if (!ipc_channel_name_.empty()) {
-    base::ResetAndReturn(&connection_ready_callback_).Run();
-  }
+  base::ResetAndReturn(&connection_ready_callback_).Run();
 }
 
 void SecurityKeyIpcClient::OnChannelError() {
@@ -158,23 +133,6 @@ void SecurityKeyIpcClient::OnChannelError() {
   }
 }
 
-void SecurityKeyIpcClient::OnConnectionDetails(
-    const std::string& channel_name) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  ipc_channel_name_ = channel_name;
-
-  // Now that we have received the name for the IPC channel we will use for our
-  // security key request, we want to disconnect from the intial IPC channel
-  // and then connect to the new one.
-  // NOTE: We do not want to perform these tasks now as we are in the middle of
-  // existing IPC message handler, thus we post the tasks so they will be
-  // handled after this method completes.
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(&SecurityKeyIpcClient::ConnectToIpcChannel,
-                            weak_factory_.GetWeakPtr(),
-                            base::ConstRef(ipc_channel_name_)));
-}
-
 void SecurityKeyIpcClient::OnSecurityKeyResponse(
     const std::string& response_data) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -188,31 +146,26 @@ void SecurityKeyIpcClient::OnSecurityKeyResponse(
   }
 }
 
-void SecurityKeyIpcClient::ConnectToIpcChannel(
-    const std::string& channel_name) {
+void SecurityKeyIpcClient::ConnectToIpcChannel() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Verify that any existing IPC connection has been closed.
   CloseIpcConnection();
 
-  // The retry loop is needed as the IPC Servers we connect to are reset (torn
-  // down and recreated) and we should be resilient in that case.
-  const base::TimeDelta kTotalWaitTime =
-      base::TimeDelta::FromMilliseconds(1000);
-  const base::TimeDelta kPerIterationWaitTime =
-      base::TimeDelta::FromMilliseconds(25);
-  const int kLoopIterations = kTotalWaitTime / kPerIterationWaitTime;
-  IPC::ChannelHandle channel_handle(channel_name);
-  for (int i = 0; i < kLoopIterations; i++) {
-    ipc_channel_ = IPC::Channel::CreateNamedClient(channel_handle, this);
-    if (ipc_channel_->Connect()) {
-      return;
-    }
-
-    ipc_channel_.reset();
-    base::PlatformThread::Sleep(kPerIterationWaitTime);
+  if (!channel_handle_.is_valid() && !WaitForSecurityKeyIpcServerChannel()) {
+    if (!connection_error_callback_.is_null())
+      base::ResetAndReturn(&connection_error_callback_).Run();
+    return;
   }
 
+  ipc_channel_ = IPC::Channel::CreateClient(
+      mojo::edk::ConnectToPeerProcess(std::move(channel_handle_)).release(),
+      this);
+  if (ipc_channel_->Connect())
+    return;
+
+  ipc_channel_.reset();
+
   if (!connection_error_callback_.is_null()) {
     base::ResetAndReturn(&connection_error_callback_).Run();
   }