Fixing SecurityKeySocket unit test issues and cleanup

remoting/host/security_key/security_key_auth_handler_posix.cc

Index: remoting/host/security_key/security_key_auth_handler_posix.cc
diff --git a/remoting/host/security_key/security_key_auth_handler_posix.cc b/remoting/host/security_key/security_key_auth_handler_posix.cc
index 245a6c913128f8205287052321ffc7b9545374ae..5d7e64d693cf023811472767c980422076ba1cc4 100644
--- a/remoting/host/security_key/security_key_auth_handler_posix.cc
+++ b/remoting/host/security_key/security_key_auth_handler_posix.cc
@@ -54,6 +54,8 @@ unsigned int GetCommandCode(const std::string& data) {
   return data.empty() ? -1 : static_cast<unsigned int>(data[0]);
 }
 
+void DeleteSocket(std::unique_ptr<remoting::SecurityKeySocket> socket) {}

[Sergey Ulanov, 2016/12/07 20:41:36]

base::DeletePointer() can be used instead of this function

[joedow, 2016/12/13 20:30:44]

Acknowledged.

+
 }  // namespace
 
 namespace remoting {
@@ -94,7 +96,7 @@ class SecurityKeyAuthHandlerPosix : public SecurityKeyAuthHandler {
       int security_key_connection_id) const;
 
   // Send an error and closes an active socket.
-  void SendErrorAndCloseActiveSocket(const ActiveSockets::const_iterator& iter);
+  void SendErrorAndCloseActiveSocket(const ActiveSockets::iterator& iter);
 
   // A request timed out.
   void RequestTimedOut(int security_key_connection_id);
@@ -204,23 +206,20 @@ void SecurityKeyAuthHandlerPosix::SendClientResponse(
   ActiveSockets::const_iterator iter =
       GetSocketForConnectionId(security_key_connection_id);
   if (iter != active_sockets_.end()) {
-    iter->second->SendResponse(response);
+    iter->second->SendResponse(response, base::Closure());
   } else {
     LOG(WARNING) << "Unknown gnubby-auth data connection: '"
                  << security_key_connection_id << "'";
   }
 }
 
-void SecurityKeyAuthHandlerPosix::SendErrorAndCloseConnection(
-    int security_key_connection_id) {
-  ActiveSockets::const_iterator iter =
-      GetSocketForConnectionId(security_key_connection_id);
+void SecurityKeyAuthHandlerPosix::SendErrorAndCloseConnection(int id) {
+  ActiveSockets::iterator iter = active_sockets_.find(id);
   if (iter != active_sockets_.end()) {
     HOST_LOG << "Sending security key error";
     SendErrorAndCloseActiveSocket(iter);
   } else {
-    LOG(WARNING) << "Unknown gnubby-auth data connection: '"
-                 << security_key_connection_id << "'";
+    LOG(WARNING) << "Unknown gnubby-auth data connection: '" << id << "'";
   }
 }
 
@@ -243,8 +242,9 @@ void SecurityKeyAuthHandlerPosix::DoAccept() {
   int result = auth_socket_->Accept(
       &accept_socket_, base::Bind(&SecurityKeyAuthHandlerPosix::OnAccepted,
                                   base::Unretained(this)));
-  if (result != net::ERR_IO_PENDING)
+  if (result != net::ERR_IO_PENDING) {
     OnAccepted(result);
+  }
 }
 
 void SecurityKeyAuthHandlerPosix::OnAccepted(int result) {
@@ -270,12 +270,10 @@ void SecurityKeyAuthHandlerPosix::OnAccepted(int result) {
   DoAccept();
 }
 
-void SecurityKeyAuthHandlerPosix::OnReadComplete(
-    int security_key_connection_id) {
+void SecurityKeyAuthHandlerPosix::OnReadComplete(int connection_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  ActiveSockets::const_iterator iter =
-      active_sockets_.find(security_key_connection_id);
+  ActiveSockets::iterator iter = active_sockets_.find(connection_id);
   DCHECK(iter != active_sockets_.end());
   std::string request_data;
   if (!iter->second->GetAndClearRequestData(&request_data)) {
@@ -284,11 +282,11 @@ void SecurityKeyAuthHandlerPosix::OnReadComplete(
   }
 
   HOST_LOG << "Received security key request: " << GetCommandCode(request_data);
-  send_message_callback_.Run(security_key_connection_id, request_data);
+  send_message_callback_.Run(connection_id, request_data);
 
   iter->second->StartReadingRequest(
       base::Bind(&SecurityKeyAuthHandlerPosix::OnReadComplete,
-                 base::Unretained(this), security_key_connection_id));
+                 base::Unretained(this), connection_id));
 }
 
 SecurityKeyAuthHandlerPosix::ActiveSockets::const_iterator
@@ -298,18 +296,18 @@ SecurityKeyAuthHandlerPosix::GetSocketForConnectionId(
 }
 
 void SecurityKeyAuthHandlerPosix::SendErrorAndCloseActiveSocket(
-    const ActiveSockets::const_iterator& iter) {
-  iter->second->SendSshError();
+    const ActiveSockets::iterator& iter) {
+  iter->second->SendSshError(
+      base::Bind(&DeleteSocket, base::Passed(&iter->second)));

[Sergey Ulanov, 2016/12/07 20:41:36]

I don't think there is anything that guarantees that we will ever succeed to
send the error message, so it's possible that the the socket will never be
deleted, so I'm not sure that just releasing it here is the right thing to do.
Also it allows for other subtle bugs, e.g. can we be sure that SecurityKeySocket
will not call timeout callback later, after SecurityKeyAuthHandlerPosix is
destroyed?

[joedow, 2016/12/13 20:30:44]

That's true.  I had a follow-up CL which used this mechanism but I think this
isn't a great approach so I will scrap it for this CL and just fix the unit
tests and comments/code alignment.

   active_sockets_.erase(iter);
 }
 
-void SecurityKeyAuthHandlerPosix::RequestTimedOut(
-    int security_key_connection_id) {
+void SecurityKeyAuthHandlerPosix::RequestTimedOut(int connection_id) {
   HOST_LOG << "SecurityKey request timed out";
-  ActiveSockets::const_iterator iter =
-      active_sockets_.find(security_key_connection_id);
-  if (iter != active_sockets_.end())
+  ActiveSockets::iterator iter = active_sockets_.find(connection_id);
+  if (iter != active_sockets_.end()) {
     SendErrorAndCloseActiveSocket(iter);
+  }
 }
 
 }  // namespace remoting