Removing 'AllowScopedIO' exception from SecurityKeyAuthHandlerLinux

remoting/host/security_key/security_key_auth_handler_linux.cc

Index: remoting/host/security_key/security_key_auth_handler_linux.cc
diff --git a/remoting/host/security_key/security_key_auth_handler_linux.cc b/remoting/host/security_key/security_key_auth_handler_linux.cc
index 346a2febb396c3ed14d25a80041e02610ca61be1..4375ffd6b2527e2f3126aa1c310e2741de33e621 100644
--- a/remoting/host/security_key/security_key_auth_handler_linux.cc
+++ b/remoting/host/security_key/security_key_auth_handler_linux.cc
@@ -4,19 +4,26 @@
 
 #include "remoting/host/security_key/security_key_auth_handler.h"
 
-#include <stdint.h>
 #include <unistd.h>
 
+#include <cstdint>
+#include <map>
 #include <memory>
+#include <string>
+#include <utility>
 
 #include "base/bind.h"
+#include "base/callback.h"
+#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/lazy_instance.h"
+#include "base/location.h"
 #include "base/logging.h"
-#include "base/stl_util.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/single_thread_task_runner.h"
 #include "base/threading/thread_checker.h"
-#include "base/threading/thread_restrictions.h"
-#include "base/values.h"
 #include "net/base/completion_callback.h"
 #include "net/base/net_errors.h"
 #include "net/socket/stream_socket.h"
@@ -32,6 +39,11 @@ const int64_t kDefaultRequestTimeoutSeconds = 60;
 base::LazyInstance<base::FilePath>::Leaky g_security_key_socket_name =
     LAZY_INSTANCE_INITIALIZER;
 
+void DeleteExistingSocketFile() {
+  // If the file already exists, a socket in use error is returned.
+  base::DeleteFile(g_security_key_socket_name.Get(), false);
+}
+
 // Socket authentication function that only allows connections from callers with
 // the current uid.
 bool MatchUid(const net::UnixDomainServerSocket::Credentials& credentials) {
@@ -53,11 +65,12 @@ namespace remoting {
 
 class SecurityKeyAuthHandlerLinux : public SecurityKeyAuthHandler {
  public:
-  SecurityKeyAuthHandlerLinux();
+  explicit SecurityKeyAuthHandlerLinux(
+      scoped_refptr<base::SingleThreadTaskRunner> file_task_runner);
   ~SecurityKeyAuthHandlerLinux() override;
 
  private:
-  typedef std::map<int, SecurityKeySocket*> ActiveSockets;
+  typedef std::map<int, std::unique_ptr<SecurityKeySocket>> ActiveSockets;
 
   // SecurityKeyAuthHandler interface.
   void CreateSecurityKeyConnection() override;
@@ -69,6 +82,9 @@ class SecurityKeyAuthHandlerLinux : public SecurityKeyAuthHandler {
   size_t GetActiveConnectionCountForTest() const override;
   void SetRequestTimeoutForTest(base::TimeDelta timeout) override;
 
+  // Sets up the socket used for accepting new connections.
+  void CreateSocket();
+
   // Starts listening for connection.
   void DoAccept();
 
@@ -101,22 +117,28 @@ class SecurityKeyAuthHandlerLinux : public SecurityKeyAuthHandler {
   SendMessageCallback send_message_callback_;
 
   // The last assigned security key connection id.
-  int last_connection_id_;
+  int last_connection_id_ = 0;
 
   // Sockets by connection id used to process gnubbyd requests.
   ActiveSockets active_sockets_;
 
+  // Used to perform File IO via Unix Domain Sockets.
+  scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_;
+
   // Timeout used for a request.
   base::TimeDelta request_timeout_;
 
+  base::WeakPtrFactory<SecurityKeyAuthHandlerLinux> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(SecurityKeyAuthHandlerLinux);
 };
 
 std::unique_ptr<SecurityKeyAuthHandler> SecurityKeyAuthHandler::Create(
     ClientSessionDetails* client_session_details,
-    const SendMessageCallback& send_message_callback) {
+    const SendMessageCallback& send_message_callback,
+    scoped_refptr<base::SingleThreadTaskRunner> file_task_runner) {
   std::unique_ptr<SecurityKeyAuthHandler> auth_handler(
-      new SecurityKeyAuthHandlerLinux());
+      new SecurityKeyAuthHandlerLinux(file_task_runner));
   auth_handler->SetSendMessageCallback(send_message_callback);
   return auth_handler;
 }
@@ -126,31 +148,27 @@ void SecurityKeyAuthHandler::SetSecurityKeySocketName(
   g_security_key_socket_name.Get() = security_key_socket_name;
 }
 
-SecurityKeyAuthHandlerLinux::SecurityKeyAuthHandlerLinux()
-    : last_connection_id_(0),
+SecurityKeyAuthHandlerLinux::SecurityKeyAuthHandlerLinux(
+    scoped_refptr<base::SingleThreadTaskRunner> file_task_runner)
+    : file_task_runner_(file_task_runner),
       request_timeout_(
-          base::TimeDelta::FromSeconds(kDefaultRequestTimeoutSeconds)) {}
+          base::TimeDelta::FromSeconds(kDefaultRequestTimeoutSeconds)),
+      weak_factory_(this) {}
 
-SecurityKeyAuthHandlerLinux::~SecurityKeyAuthHandlerLinux() {
-  STLDeleteValues(&active_sockets_);
-}
+SecurityKeyAuthHandlerLinux::~SecurityKeyAuthHandlerLinux() {}
 
 void SecurityKeyAuthHandlerLinux::CreateSecurityKeyConnection() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!g_security_key_socket_name.Get().empty());
 
-  {
-    // DeleteFile() is a blocking operation, but so is creation of the unix
-    // socket below. Consider moving this class to a different thread if this
-    // causes any problems. See crbug.com/509807.
-    // TODO(joedow): Since this code now runs as a host extension, we should
-    //               perform our IO on a separate thread: crbug.com/591739
-    base::ThreadRestrictions::ScopedAllowIO allow_io;
-
-    // If the file already exists, a socket in use error is returned.
-    base::DeleteFile(g_security_key_socket_name.Get(), false);
-  }
+  file_task_runner_->PostTaskAndReply(

[Sergey Ulanov, 2016/07/25 18:29:55]

add a comment to explain why this is necessary.

[joedow, 2016/07/25 22:39:25]

Done.

+      FROM_HERE, base::Bind(&DeleteExistingSocketFile),

[Sergey Ulanov, 2016/07/25 18:29:55]

nit: here you can post task for base::DeleteFile(), then you wouldn't need
DeleteExistingSocketFile().

[joedow, 2016/07/25 22:39:25]

Done.

+      base::Bind(&SecurityKeyAuthHandlerLinux::CreateSocket,
+                 weak_factory_.GetWeakPtr()));
+}
 
+void SecurityKeyAuthHandlerLinux::CreateSocket() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   HOST_LOG << "Listening for security key requests on "
            << g_security_key_socket_name.Get().value();
 
@@ -212,9 +230,10 @@ void SecurityKeyAuthHandlerLinux::SetRequestTimeoutForTest(
 }
 
 void SecurityKeyAuthHandlerLinux::DoAccept() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   int result = auth_socket_->Accept(
       &accept_socket_, base::Bind(&SecurityKeyAuthHandlerLinux::OnAccepted,
-                                  base::Unretained(this)));
+                                  weak_factory_.GetWeakPtr()));
   if (result != net::ERR_IO_PENDING)
     OnAccepted(result);
 }
@@ -232,11 +251,11 @@ void SecurityKeyAuthHandlerLinux::OnAccepted(int result) {
   SecurityKeySocket* socket = new SecurityKeySocket(
       std::move(accept_socket_), request_timeout_,
       base::Bind(&SecurityKeyAuthHandlerLinux::RequestTimedOut,
-                 base::Unretained(this), security_key_connection_id));
-  active_sockets_[security_key_connection_id] = socket;
+                 weak_factory_.GetWeakPtr(), security_key_connection_id));

[Sergey Ulanov, 2016/07/25 18:29:55]

nit: don't really need to use weak-ptrs here because this object owns the
socket, but feel free to keep it for consistency.

[joedow, 2016/07/25 22:39:25]

Done.  I was thinking this would be safer in case something was changed later,
but the important methods already use a thread checker so this isn't as helpful
as I had first thought.

+  active_sockets_[security_key_connection_id] = base::WrapUnique(socket);
   socket->StartReadingRequest(
       base::Bind(&SecurityKeyAuthHandlerLinux::OnReadComplete,
-                 base::Unretained(this), security_key_connection_id));
+                 weak_factory_.GetWeakPtr(), security_key_connection_id));
 
   // Continue accepting new connections.
   DoAccept();
@@ -260,7 +279,7 @@ void SecurityKeyAuthHandlerLinux::OnReadComplete(
 
   iter->second->StartReadingRequest(
       base::Bind(&SecurityKeyAuthHandlerLinux::OnReadComplete,
-                 base::Unretained(this), security_key_connection_id));
+                 weak_factory_.GetWeakPtr(), security_key_connection_id));
 }
 
 SecurityKeyAuthHandlerLinux::ActiveSockets::const_iterator
@@ -272,7 +291,6 @@ SecurityKeyAuthHandlerLinux::GetSocketForConnectionId(
 void SecurityKeyAuthHandlerLinux::SendErrorAndCloseActiveSocket(
     const ActiveSockets::const_iterator& iter) {
   iter->second->SendSshError();
-  delete iter->second;
   active_sockets_.erase(iter);
 }