chromeos: Remove dbus::FileDescriptor from SessionManagerClient

chrome/browser/chromeos/login/chrome_restart_request.cc

Index: chrome/browser/chromeos/login/chrome_restart_request.cc
diff --git a/chrome/browser/chromeos/login/chrome_restart_request.cc b/chrome/browser/chromeos/login/chrome_restart_request.cc
index 51482d93a10d4362d61877b6d88a5997edc37f36..e24576d0e7e2c47004995686c94fc44b4a0ac4eb 100644
--- a/chrome/browser/chromeos/login/chrome_restart_request.cc
+++ b/chrome/browser/chromeos/login/chrome_restart_request.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/login/chrome_restart_request.h"
 
+#include <sys/socket.h>
 #include <vector>
 
 #include "ash/common/ash_switches.h"
@@ -269,6 +270,9 @@ class ChromeRestartRequest
   // Fires job restart request to session manager.
   void RestartJob();
 
+  // Called when RestartJob D-Bus method call is complete.
+  void OnRestartJob(base::ScopedFD local_auth_fd, DBusMethodCallStatus status);
+
   const std::vector<std::string> argv_;
   base::OneShotTimer timer_;
 
@@ -318,8 +322,31 @@ void ChromeRestartRequest::RestartJob() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   VLOG(1) << "ChromeRestartRequest::RestartJob";
 
-  DBusThreadManager::Get()->GetSessionManagerClient()->RestartJob(argv_);
+  // The session manager requires a RestartJob caller to open a socket pair and
+  // pass one end over D-Bus while holding the local end open for the duration
+  // of the call.
+  int sockets[2] = {-1, -1};
+  if (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) < 0) {

[stevenjb, 2016/09/06 15:59:28]

I'm not familiar with socketpair(), is this safe to run on the UI thread?

[hashimoto, 2016/09/07 06:05:46]

AssertIOAllowed fails on the UI thread, and it's meant to avoid blocking the UI
thread waiting for a disk which is usually much slower than CPU and RAM.
IIUC using socketpair() here is OK because:
 1. socketpair() shouldn't result in disk IO.
 2. Here we're about to kill this process to start a new Chrome process. So
having janky UI here shouldn't matter much. At least,
ChromeRestartRequest::Start() is already calling BrowserProcessImpl::EndSession
on the UI thread.

WDYT?

[stevenjb, 2016/09/08 16:40:11]

AssertIOAllowed will fail in places where we added an assert. If socketpair() is
a system call then it won't have an assert wrapper, will it? (I'm not at all
familiar with it).

socketpair() *shouldn't* result in disk IO or *won't*? If it *won't* than I am
OK with it here, but if we are uncertain then in principal I think we should use
a worker thread.

That said, if we are only ever calling this from code where we might be blocking
on the UI thread anyway, then this is probably fine, but we should have a
comment explaining that it is OK.

+    PLOG(ERROR) << "Failed to create a unix domain socketpair";
+    delete this;
+    return;
+  }
+  base::ScopedFD local_auth_fd(sockets[0]);
+  base::ScopedFD remote_auth_fd(sockets[1]);
+  // Ownership of local_auth_fd is passed to the callback that is to be
+  // called on completion of this method call. This keeps the browser end
+  // of the socket-pair alive for the duration of the RPC.
+  DBusThreadManager::Get()->GetSessionManagerClient()->RestartJob(
+      remote_auth_fd.get(), argv_,
+      base::Bind(&ChromeRestartRequest::OnRestartJob, AsWeakPtr(),
+                 base::Passed(&local_auth_fd)));
+}
 
+void ChromeRestartRequest::OnRestartJob(base::ScopedFD local_auth_fd,
+                                        DBusMethodCallStatus status) {
+  // Now that the call is complete, local_auth_fd can be closed and discarded,
+  // which will happen automatically when it goes out of scope.
+  VLOG(1) << "OnRestartJob";
   delete this;
 }