SessionManagerClient::RestartJob: Use argv instead of string.

This changes the interface to session_manager to pass the command line
as an array instead of a string. The previous code path would
(incorrectly) quote arguments and combine them into a string, and then
again parse and unquote them on the session_manager side, which is
unnecessary and error-prone. This changes the code to pass an array of
strings in the DBus call, which simplifies things considerably.

BUG=chromium:433172
TEST=Guest session starts successfully.

Review URL: https://codereview.chromium.org/1268973004

Cr-Commit-Position: refs/heads/master@{#341873}

11 files changed, 58 insertions, 96 deletions

diff --git a/chrome/browser/chromeos/login/chrome_restart_request.cc b/chrome/browser/chromeos/login/chrome_restart_request.cc
index 9821fb8..e314914 100644
--- a/chrome/browser/chromeos/login/chrome_restart_request.cc
+++ b/chrome/browser/chromeos/login/chrome_restart_request.cc
@@ -14,6 +14,7 @@
 #include "base/prefs/pref_service.h"
 #include "base/process/launch.h"
 #include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/sys_info.h"
 #include "base/timer/timer.h"
@@ -55,17 +56,14 @@ namespace {
 // Increase logging level for Guest mode to avoid INFO messages in logs.
 const char kGuestModeLoggingLevel[] = "1";
 
-// Format of command line switch.
-const char kSwitchFormatString[] = " --%s=\"%s\"";
-
 // Derives the new command line from |base_command_line| by doing the following:
 // - Forward a given switches list to new command;
 // - Set start url if given;
 // - Append/override switches using |new_switches|;
-std::string DeriveCommandLine(const GURL& start_url,
-                              const base::CommandLine& base_command_line,
-                              const base::DictionaryValue& new_switches,
-                              base::CommandLine* command_line) {
+void DeriveCommandLine(const GURL& start_url,
+                       const base::CommandLine& base_command_line,
+                       const base::DictionaryValue& new_switches,
+                       base::CommandLine* command_line) {
   DCHECK_NE(&base_command_line, command_line);
 
   static const char* const kForwardSwitches[] = {
@@ -236,30 +234,12 @@ std::string DeriveCommandLine(const GURL& start_url,
     CHECK(it.value().GetAsString(&value));
     command_line->AppendSwitchASCII(it.key(), value);
   }
-
-  std::string cmd_line_str = command_line->GetCommandLineString();
-  // Special workaround for the arguments that should be quoted.
-  // Copying switches won't be needed when Guest mode won't need restart
-  // http://crosbug.com/6924
-  if (base_command_line.HasSwitch(::switches::kRegisterPepperPlugins)) {
-    cmd_line_str += base::StringPrintf(
-        kSwitchFormatString,
-        ::switches::kRegisterPepperPlugins,
-        base_command_line.GetSwitchValueNative(
-            ::switches::kRegisterPepperPlugins).c_str());
-  }
-
-  return cmd_line_str;
 }
 
 // Simulates a session manager restart by launching give command line
 // and exit current process.
-void ReLaunch(const std::string& command_line) {
-  // This is not a proper way to get |argv| but it's good enough for debugging.
-  std::vector<std::string> argv = base::SplitString(
-      command_line, " ", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
-
-  base::LaunchProcess(argv, base::LaunchOptions());
+void ReLaunch(const base::CommandLine& command_line) {
+  base::LaunchProcess(command_line.argv(), base::LaunchOptions());
   chrome::AttemptUserExit();
 }
 
@@ -273,7 +253,7 @@ void EnsureLocalStateIsWritten() {}
 class ChromeRestartRequest
     : public base::SupportsWeakPtr<ChromeRestartRequest> {
  public:
-  explicit ChromeRestartRequest(const std::string& command_line);
+  explicit ChromeRestartRequest(const std::vector<std::string>& argv);
   ~ChromeRestartRequest();
 
   // Starts the request.
@@ -283,22 +263,20 @@ class ChromeRestartRequest
   // Fires job restart request to session manager.
   void RestartJob();
 
-  const int pid_;
-  const std::string command_line_;
+  const std::vector<std::string> argv_;
   base::OneShotTimer<ChromeRestartRequest> timer_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeRestartRequest);
 };
 
-ChromeRestartRequest::ChromeRestartRequest(const std::string& command_line)
-    : pid_(getpid()),
-      command_line_(command_line) {}
+ChromeRestartRequest::ChromeRestartRequest(const std::vector<std::string>& argv)
+    : argv_(argv) {}
 
 ChromeRestartRequest::~ChromeRestartRequest() {}
 
 void ChromeRestartRequest::Start() {
-  VLOG(1) << "Requesting a restart with PID " << pid_
-          << " and command line: " << command_line_;
+  VLOG(1) << "Requesting a restart with command line: "
+          << base::JoinString(argv_, " ");
 
   // Session Manager may kill the chrome anytime after this point.
   // Write exit_cleanly and other stuff to the disk here.
@@ -333,19 +311,17 @@ void ChromeRestartRequest::Start() {
 void ChromeRestartRequest::RestartJob() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  DBusThreadManager::Get()->GetSessionManagerClient()->RestartJob(
-      pid_, command_line_);
+  DBusThreadManager::Get()->GetSessionManagerClient()->RestartJob(argv_);
 
   delete this;
 }
 
 }  // namespace
 
-std::string GetOffTheRecordCommandLine(
-    const GURL& start_url,
-    bool is_oobe_completed,
-    const base::CommandLine& base_command_line,
-    base::CommandLine* command_line) {
+void GetOffTheRecordCommandLine(const GURL& start_url,
+                                bool is_oobe_completed,
+                                const base::CommandLine& base_command_line,
+                                base::CommandLine* command_line) {
   base::DictionaryValue otr_switches;
   otr_switches.SetString(switches::kGuestSession, std::string());
   otr_switches.SetString(::switches::kIncognito, std::string());
@@ -362,13 +338,10 @@ std::string GetOffTheRecordCommandLine(
   if (!is_oobe_completed)
     otr_switches.SetString(switches::kOobeGuestSession, std::string());
 
-  return DeriveCommandLine(start_url,
-                           base_command_line,
-                           otr_switches,
-                           command_line);
+  DeriveCommandLine(start_url, base_command_line, otr_switches, command_line);
 }
 
-void RestartChrome(const std::string& command_line) {
+void RestartChrome(const base::CommandLine& command_line) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   BootTimesRecorder::Get()->set_restart_requested();
 
@@ -385,7 +358,7 @@ void RestartChrome(const std::string& command_line) {
   }
 
   // ChromeRestartRequest deletes itself after request sent to session manager.
-  (new ChromeRestartRequest(command_line))->Start();
+  (new ChromeRestartRequest(command_line.argv()))->Start();
 }
 
 }  // namespace chromeos
diff --git a/chrome/browser/chromeos/login/chrome_restart_request.h b/chrome/browser/chromeos/login/chrome_restart_request.h
index 788edb1..576b5fa 100644
--- a/chrome/browser/chromeos/login/chrome_restart_request.h
+++ b/chrome/browser/chromeos/login/chrome_restart_request.h
@@ -15,16 +15,14 @@ class CommandLine;
 
 namespace chromeos {
 
-// Returns the command line string to be used for the OTR process. Also modifies
-// the given command line.
-std::string GetOffTheRecordCommandLine(
-    const GURL& start_url,
-    bool is_oobe_completed,
-    const base::CommandLine& base_command_line,
-    base::CommandLine* command_line);
+// Determines the |command_line| to be used for the OTR process.
+void GetOffTheRecordCommandLine(const GURL& start_url,
+                                bool is_oobe_completed,
+                                const base::CommandLine& base_command_line,
+                                base::CommandLine* command_line);
 
 // Request session manager to restart chrome with a new command line.
-void RestartChrome(const std::string& command_line);
+void RestartChrome(const base::CommandLine& command_line);
 
 }  // namespace chromeos
 
diff --git a/chrome/browser/chromeos/login/screens/error_screen.cc b/chrome/browser/chromeos/login/screens/error_screen.cc
index 3b43664..5b36bad 100644
--- a/chrome/browser/chromeos/login/screens/error_screen.cc
+++ b/chrome/browser/chromeos/login/screens/error_screen.cc
@@ -229,13 +229,9 @@ void ErrorScreen::OnOffTheRecordAuthSuccess() {
   const base::CommandLine& browser_command_line =
       *base::CommandLine::ForCurrentProcess();
   base::CommandLine command_line(browser_command_line.GetProgram());
-  std::string cmd_line_str =
-      GetOffTheRecordCommandLine(GURL(),
-                                 StartupUtils::IsOobeCompleted(),
-                                 browser_command_line,
-                                 &command_line);
-
-  RestartChrome(cmd_line_str);
+  GetOffTheRecordCommandLine(GURL(), StartupUtils::IsOobeCompleted(),
+                             browser_command_line, &command_line);
+  RestartChrome(command_line);
 }
 
 void ErrorScreen::OnPasswordChangeDetected() {
diff --git a/chrome/browser/chromeos/login/session/user_session_manager.cc b/chrome/browser/chromeos/login/session/user_session_manager.cc
index 2675739..926d0d51 100644
--- a/chrome/browser/chromeos/login/session/user_session_manager.cc
+++ b/chrome/browser/chromeos/login/session/user_session_manager.cc
@@ -372,11 +372,8 @@ void UserSessionManager::CompleteGuestSessionLogin(const GURL& start_url) {
   const base::CommandLine& browser_command_line =
       *base::CommandLine::ForCurrentProcess();
   base::CommandLine command_line(browser_command_line.GetProgram());
-  std::string cmd_line_str =
-      GetOffTheRecordCommandLine(start_url,
-                                 StartupUtils::IsOobeCompleted(),
-                                 browser_command_line,
-                                 &command_line);
+  GetOffTheRecordCommandLine(start_url, StartupUtils::IsOobeCompleted(),
+                             browser_command_line, &command_line);
 
   // This makes sure that Chrome restarts with no per-session flags. The guest
   // profile will always have empty set of per-session flags. If this is not
@@ -391,7 +388,7 @@ void UserSessionManager::CompleteGuestSessionLogin(const GURL& start_url) {
         chromeos::login::kGuestUserName, base::CommandLine::StringVector());
   }
 
-  RestartChrome(cmd_line_str);
+  RestartChrome(command_line);
 }
 
 scoped_refptr<Authenticator> UserSessionManager::CreateAuthenticator(
diff --git a/chrome/browser/chromeos/settings/device_settings_test_helper.cc b/chrome/browser/chromeos/settings/device_settings_test_helper.cc
index abc8caf..45236d8 100644
--- a/chrome/browser/chromeos/settings/device_settings_test_helper.cc
+++ b/chrome/browser/chromeos/settings/device_settings_test_helper.cc
@@ -108,8 +108,8 @@ bool DeviceSettingsTestHelper::IsScreenLocked() const { return false; }
 
 void DeviceSettingsTestHelper::EmitLoginPromptVisible() {}
 
-void DeviceSettingsTestHelper::RestartJob(int pid,
-                                          const std::string& command_line) {}
+void DeviceSettingsTestHelper::RestartJob(
+    const std::vector<std::string>& argv) {}
 
 void DeviceSettingsTestHelper::StartSession(const std::string& user_email) {}
 
diff --git a/chrome/browser/chromeos/settings/device_settings_test_helper.h b/chrome/browser/chromeos/settings/device_settings_test_helper.h
index 0736342..823c088 100644
--- a/chrome/browser/chromeos/settings/device_settings_test_helper.h
+++ b/chrome/browser/chromeos/settings/device_settings_test_helper.h
@@ -88,7 +88,7 @@ class DeviceSettingsTestHelper : public SessionManagerClient {
   bool HasObserver(const Observer* observer) const override;
   bool IsScreenLocked() const override;
   void EmitLoginPromptVisible() override;
-  void RestartJob(int pid, const std::string& command_line) override;
+  void RestartJob(const std::vector<std::string>& argv) override;
   void StartSession(const std::string& user_email) override;
   void StopSession() override;
   void NotifySupervisedUserCreationStarted() override;
diff --git a/chromeos/dbus/fake_session_manager_client.cc b/chromeos/dbus/fake_session_manager_client.cc
index c1745cf..89261ea 100644
--- a/chromeos/dbus/fake_session_manager_client.cc
+++ b/chromeos/dbus/fake_session_manager_client.cc
@@ -47,9 +47,8 @@ bool FakeSessionManagerClient::IsScreenLocked() const {
 void FakeSessionManagerClient::EmitLoginPromptVisible() {
 }
 
-void FakeSessionManagerClient::RestartJob(int pid,
-                                          const std::string& command_line) {
-}
+void FakeSessionManagerClient::RestartJob(
+    const std::vector<std::string>& argv) {}
 
 void FakeSessionManagerClient::StartSession(const std::string& user_email) {
   DCHECK_EQ(0UL, user_sessions_.count(user_email));
diff --git a/chromeos/dbus/fake_session_manager_client.h b/chromeos/dbus/fake_session_manager_client.h
index ed4c5c1..e2ff7cc 100644
--- a/chromeos/dbus/fake_session_manager_client.h
+++ b/chromeos/dbus/fake_session_manager_client.h
@@ -31,7 +31,7 @@ class FakeSessionManagerClient : public SessionManagerClient {
   bool HasObserver(const Observer* observer) const override;
   bool IsScreenLocked() const override;
   void EmitLoginPromptVisible() override;
-  void RestartJob(int pid, const std::string& command_line) override;
+  void RestartJob(const std::vector<std::string>& argv) override;
   void StartSession(const std::string& user_email) override;
   void StopSession() override;
   void NotifySupervisedUserCreationStarted() override;
diff --git a/chromeos/dbus/mock_session_manager_client.h b/chromeos/dbus/mock_session_manager_client.h
index 3a88e52..17f0caf 100644
--- a/chromeos/dbus/mock_session_manager_client.h
+++ b/chromeos/dbus/mock_session_manager_client.h
@@ -24,7 +24,7 @@ class MockSessionManagerClient : public SessionManagerClient {
   MOCK_CONST_METHOD1(HasObserver, bool(const Observer*));
   MOCK_CONST_METHOD0(IsScreenLocked, bool(void));
   MOCK_METHOD0(EmitLoginPromptVisible, void(void));
-  MOCK_METHOD2(RestartJob, void(int, const std::string&));
+  MOCK_METHOD1(RestartJob, void(const std::vector<std::string>&));
   MOCK_METHOD1(StartSession, void(const std::string&));
   MOCK_METHOD0(StopSession, void(void));
   MOCK_METHOD0(NotifySupervisedUserCreationStarted, void(void));
diff --git a/chromeos/dbus/session_manager_client.cc b/chromeos/dbus/session_manager_client.cc
index 438ec05..36a4e28 100644
--- a/chromeos/dbus/session_manager_client.cc
+++ b/chromeos/dbus/session_manager_client.cc
@@ -115,15 +115,15 @@ class SessionManagerClientImpl : public SessionManagerClient {
     FOR_EACH_OBSERVER(Observer, observers_, EmitLoginPromptVisibleCalled());
   }
 
-  void RestartJob(int pid, const std::string& command_line) override {
+  void RestartJob(const std::vector<std::string>& argv) override {
     dbus::ScopedFileDescriptor local_auth_fd(new dbus::FileDescriptor);
     dbus::ScopedFileDescriptor remote_auth_fd(new dbus::FileDescriptor);
 
-    // The session_manager provides a new method to replace RestartJob, called
-    // RestartJobWithAuth, that is able to be used correctly within a PID
-    // namespace. To use it, the caller must create a unix domain socket pair
-    // and pass one end over dbus while holding the local end open for the
-    // duration of the call.
+    // session_manager's RestartJob call requires the caller to open a socket
+    // pair and pass one end over dbus while holding the local end open for the
+    // duration of the call. session_manager uses this to determine whether the
+    // PID the restart request originates from belongs to the browser itself.
+    //
     // Here, we call CreateValidCredConduit() to create the socket pair,
     // and then pass both ends along to CallRestartJobWithValidFd(), which
     // takes care of them from there.
@@ -146,7 +146,7 @@ class SessionManagerClientImpl : public SessionManagerClient {
         FROM_HERE, create_credentials_conduit_closure,
         base::Bind(&SessionManagerClientImpl::CallRestartJobWithValidFd,
                    weak_ptr_factory_.GetWeakPtr(), base::Passed(&local_auth_fd),
-                   base::Passed(&remote_auth_fd), command_line),
+                   base::Passed(&remote_auth_fd), argv),
         false);
   }
 
@@ -421,19 +421,18 @@ class SessionManagerClientImpl : public SessionManagerClient {
             callback));
   }
 
-  // Calls RestartJobWithAuth to tell the session manager to restart the
-  // browser using the contents of command_line, authorizing the call
-  // using credentials acquired via remote_auth_fd.
-  // Ownership of local_auth_fd is held for the duration of the dbus call.
+  // Calls RestartJob to tell the session manager to restart the browser using
+  // the contents of |argv| as the command line, authorizing the call using
+  // credentials acquired via |remote_auth_fd|. Ownership of |local_auth_fd| is
+  // held for the duration of the dbus call.
   void CallRestartJobWithValidFd(dbus::ScopedFileDescriptor local_auth_fd,
                                  dbus::ScopedFileDescriptor remote_auth_fd,
-                                 const std::string& command_line) {
-    dbus::MethodCall method_call(
-        login_manager::kSessionManagerInterface,
-        login_manager::kSessionManagerRestartJobWithAuth);
+                                 const std::vector<std::string>& argv) {
+    dbus::MethodCall method_call(login_manager::kSessionManagerInterface,
+                                 login_manager::kSessionManagerRestartJob);
     dbus::MessageWriter writer(&method_call);
     writer.AppendFileDescriptor(*remote_auth_fd);
-    writer.AppendString(command_line);
+    writer.AppendArrayOfStrings(argv);
 
     // 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
@@ -670,7 +669,7 @@ class SessionManagerClientStubImpl : public SessionManagerClient {
   }
   bool IsScreenLocked() const override { return screen_is_locked_; }
   void EmitLoginPromptVisible() override {}
-  void RestartJob(int pid, const std::string& command_line) override {}
+  void RestartJob(const std::vector<std::string>& argv) override {}
   void StartSession(const std::string& user_email) override {}
   void StopSession() override {}
   void NotifySupervisedUserCreationStarted() override {}
diff --git a/chromeos/dbus/session_manager_client.h b/chromeos/dbus/session_manager_client.h
index 254e762..ba21997 100644
--- a/chromeos/dbus/session_manager_client.h
+++ b/chromeos/dbus/session_manager_client.h
@@ -73,8 +73,8 @@ class CHROMEOS_EXPORT SessionManagerClient : public DBusClient {
   // Kicks off an attempt to emit the "login-prompt-visible" upstart signal.
   virtual void EmitLoginPromptVisible() = 0;
 
-  // Restarts a job referenced by |pid| with the provided command line.
-  virtual void RestartJob(int pid, const std::string& command_line) = 0;
+  // Restarts the browser job, passing |argv| as the updated command line.
+  virtual void RestartJob(const std::vector<std::string>& argv) = 0;
 
   // Starts the session for the user.
   virtual void StartSession(const std::string& user_email) = 0;