Simpler logic to setup/restore Chrome command line

Simplified the logic to save, set, and restore the command line used to
launch a browser. For efficiency, functions are implemented as single
shell commands to run on the device.

This reduces about 20% of the time taken to start a new browser, and
overall 5% of the total running time of telemety unit tests.

BUG=379378

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

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

build/android/pylib/android_commands.py

@@ -701,8 +701,7 @@ class AndroidCommands(object):
     """Send a command to the adb shell and return the result.
 
     Args:
-      command: String containing the shell command to send. Must not include
-               the single quotes as we use them to escape the whole command.
+      command: String containing the shell command to send.
       timeout_time: Number of seconds to wait for command to respond before
         retrying, used by AdbInterface.SendShellCommand.
       log_result: Boolean to indicate whether we should log the result of the

build/android/pylib/device/device_utils.py

@@ -8,6 +8,7 @@ Eventually, this will be based on adb_wrapper.
 """
 # pylint: disable=W0613
 
+import pipes
 import sys
 import time
 
@@ -634,6 +635,31 @@ class DeviceUtils(object):
     else:
       self.old_interface.SetFileContents(device_path, contents)
 
+  @decorators.WithTimeoutAndRetriesFromInstance()
+  def WriteTextFile(self, device_path, text, as_root=False, timeout=None,
+                    retries=None):
+    """Writes |text| to a file on the device.
+
+    Assuming that |text| is a small string, this is typically more efficient
+    than |WriteFile|, as no files are pushed into the device.
+
+    Args:
+      device_path: A string containing the absolute path to the file to write
+                   on the device.
+      text: A short string of text to write to the file on the device.
+      as_root: A boolean indicating whether the write should be executed with
+               root privileges.
+      timeout: timeout in seconds
+      retries: number of retries
+
+    Raises:
+      CommandFailedError if the file could not be written on the device.
+      CommandTimeoutError on timeout.
+      DeviceUnreachableError on missing device.
+    """
+    self._RunShellCommandImpl('echo {1} > {0}'.format(device_path,
+        pipes.quote(text)), check_return=True, as_root=as_root)
+
   @decorators.WithTimeoutAndRetriesFromInstance()
   def Ls(self, device_path, timeout=None, retries=None):
     """Lists the contents of a directory on the device.

build/android/pylib/device/device_utils_test.py

@@ -1196,6 +1196,44 @@ class DeviceUtilsWriteFileTest(DeviceUtilsOldImplTest):
       self.device.WriteFile('/test/file/no.permissions.to.write',
                             'new test file contents', as_root=True)
 
+class DeviceUtilsWriteTextFileTest(DeviceUtilsOldImplTest):
+
+  def testWriteTextFileTest_basic(self):
+    with self.assertCalls(
+        "adb -s 0123456789abcdef shell 'echo some.string"
+        " > /test/file/to.write; echo %$?'", '%0\r\n'):
+      self.device.WriteTextFile('/test/file/to.write', 'some.string')
+
+  def testWriteTextFileTest_stringWithSpaces(self):
+    with self.assertCalls(
+        "adb -s 0123456789abcdef shell 'echo '\\''some other string'\\''"
+        " > /test/file/to.write; echo %$?'", '%0\r\n'):
+      self.device.WriteTextFile('/test/file/to.write', 'some other string')
+
+  def testWriteTextFileTest_asRoot_withSu(self):
+    with self.assertCallsSequence([
+        ("adb -s 0123456789abcdef shell 'ls /root'", 'Permission denied\r\n'),
+        ("adb -s 0123456789abcdef shell 'su -c echo some.string"
+          " > /test/file/to.write; echo %$?'", '%0\r\n')]):
+      self.device.WriteTextFile('/test/file/to.write', 'some.string',
+                                as_root=True)
+
+  def testWriteTextFileTest_asRoot_withRoot(self):
+    with self.assertCallsSequence([
+        ("adb -s 0123456789abcdef shell 'ls /root'", 'hello\r\nworld\r\n'),
+        ("adb -s 0123456789abcdef shell 'echo some.string"
+          " > /test/file/to.write; echo %$?'", '%0\r\n')]):
+      self.device.WriteTextFile('/test/file/to.write', 'some.string',
+                                as_root=True)
+
+  def testWriteTextFileTest_asRoot_rejected(self):
+    with self.assertCallsSequence([
+        ("adb -s 0123456789abcdef shell 'ls /root'", 'Permission denied\r\n'),
+        ("adb -s 0123456789abcdef shell 'su -c echo some.string"
+          " > /test/file/to.write; echo %$?'", '%1\r\n')]):
+      with self.assertRaises(device_errors.CommandFailedError):
+        self.device.WriteTextFile('/test/file/to.write', 'some.string',
+                                  as_root=True)
 
 class DeviceUtilsLsTest(DeviceUtilsOldImplTest):
 

tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py

@@ -4,6 +4,7 @@
 
 import logging
 import os
+import pipes
 import re
 import subprocess
 import sys
@@ -238,8 +239,8 @@ class AndroidBrowserBackend(chrome_browser_backend.ChromeBrowserBackend):
 
   def _SetUpCommandLine(self):
     def QuoteIfNeeded(arg):
-      # Escape 'key=valueA valueB' to 'key="valueA valueB"'
-      # Already quoted values, or values without space are left untouched.
+      # Properly escape "key=valueA valueB" to "key='valueA valueB'"
+      # Values without spaces, or that seem to be quoted are left untouched.
       # This is required so CommandLine.java can parse valueB correctly rather
       # than as a separate switch.
       params = arg.split('=', 1)
@@ -250,39 +251,32 @@ class AndroidBrowserBackend(chrome_browser_backend.ChromeBrowserBackend):
         return arg
       if values[0] in '"\'' and values[-1] == values[0]:
         return arg
-      return '%s="%s"' % (key, values)
+      return '%s=%s' % (key, pipes.quote(values))
     args = [self._backend_settings.pseudo_exec_name]
     args.extend(self.GetBrowserStartupArgs())
-    args = ' '.join(map(QuoteIfNeeded, args))
-
-    self._SetCommandLineFile(args)
-
-  def _SetCommandLineFile(self, file_contents):
-    logging.debug('Using command line: ' + file_contents)
-    def IsProtectedFile(name):
-      if self._adb.device().old_interface.FileExistsOnDevice(name):
-        return not self._adb.device().old_interface.IsFileWritableOnDevice(name)
-      else:
-        parent_name = os.path.dirname(name)
-        if parent_name != '':
-          return IsProtectedFile(parent_name)
-        else:
-          return True
-
-    protected = IsProtectedFile(self._backend_settings.cmdline_file)
+    content = ' '.join(QuoteIfNeeded(arg) for arg in args)
+    cmdline_file = self._backend_settings.cmdline_file
+    as_root = self._adb.device().old_interface.CanAccessProtectedFileContents()
+
     try:
-      self._saved_cmdline = ''.join(
-          self._adb.device().ReadFile(
-              self._backend_settings.cmdline_file, as_root=protected)
-          or [])
-      self._adb.device().WriteFile(
-          self._backend_settings.cmdline_file, file_contents,
-          as_root=protected)
+      # Save the current command line to restore later, except if it appears to
+      # be a  Telemetry created one. This is to prevent a common bug where
+      # --host-resolver-rules borks people's browsers if something goes wrong
+      # with Telemetry.
+      self._saved_cmdline = ''.join(self._adb.device().ReadFile(cmdline_file))
+      if '--host-resolver-rules' in self._saved_cmdline:
+        self._saved_cmdline = ''
+      self._adb.device().WriteTextFile(cmdline_file, content, as_root=as_root)
     except device_errors.CommandFailedError:
       logging.critical('Cannot set Chrome command line. '
                        'Fix this by flashing to a userdebug build.')
       sys.exit(1)
 
+  def _RestoreCommandLine(self):
+    as_root = self._adb.device().old_interface.CanAccessProtectedFileContents()
+    self._adb.device().WriteTextFile(self._backend_settings.cmdline_file,
+      self._saved_cmdline, as_root=as_root)
+
   def Start(self):
     self._SetUpCommandLine()
 
@@ -326,13 +320,7 @@ class AndroidBrowserBackend(chrome_browser_backend.ChromeBrowserBackend):
       self.Close()
       raise
     finally:
-      # Restore the saved command line if it appears to have come from a user.
-      # If it appears to be a Telemetry created command line, then don't restore
-      # it. This is to prevent a common bug where --host-resolver-rules borks
-      # people's browsers if something goes wrong with Telemetry.
-      self._SetCommandLineFile(
-          self._saved_cmdline
-          if '--host-resolver-rules' not in self._saved_cmdline else '')
+      self._RestoreCommandLine()
 
   def GetBrowserStartupArgs(self):
     args = super(AndroidBrowserBackend, self).GetBrowserStartupArgs()