[Android] Redirect std{in,out,err} to sockets for layout tests.

third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py

 # Copyright (C) 2012 Google Inc. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 # notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 # copyright notice, this list of conditions and the following disclaimer
 # in the documentation and/or other materials provided with the
 # distribution.
 #     * Neither the name of Google Inc. nor the names of its
 # contributors may be used to endorse or promote products derived from
 # this software without specific prior written permission.
 #
 # THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
 # "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
 # LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
 # A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
 # OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
 # SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
 # LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
 # DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
 # THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+import itertools
 import logging
 import os
 import re
 import sys
 import threading
 import time
 
 from multiprocessing.pool import ThreadPool
 
 from webkitpy.common.system.executive import ScriptError
@@ skipping 221 matching lines @@
 
     BUILD_REQUIREMENTS_URL = 'https://www.chromium.org/developers/how-tos/android-build-instructions'
 
     def __init__(self, host, port_name, **kwargs):
         super(AndroidPort, self).__init__(host, port_name, **kwargs)
 
         self._operating_system = 'android'
         self._version = 'icecreamsandwich'
 
         self._host_port = factory.PortFactory(host).get(**kwargs)
-        self._server_process_constructor = self._android_server_process_constructor
+        self.server_process_constructor = self._android_server_process_constructor
 
         if not self.get_option('disable_breakpad'):
             self._dump_reader = DumpReaderAndroid(host, self._build_path())
 
         if self.driver_name() != self.CONTENT_SHELL_NAME:
             raise AssertionError('Layout tests on Android only support content_shell as the driver.')
 
         self._driver_details = ContentShellDriverDetails()
 
         # Initialize the AndroidDevices class which tracks available devices.
@@ skipping 321 matching lines @@
 
         perfhost_display_patch = perfhost_path if perfhost_path else 'perfhost_linux'
         _log.debug("To view the full profile, run:")
         _log.debug(' '.join([perfhost_display_patch] + perfhost_report_command))
 
 
 class ChromiumAndroidDriver(driver.Driver):
 
     def __init__(self, port, worker_number, pixel_tests, driver_details, android_devices, no_timeout=False):
         super(ChromiumAndroidDriver, self).__init__(port, worker_number, pixel_tests, no_timeout)
-        self._in_fifo_path = driver_details.device_fifo_directory() + 'stdin.fifo'
-        self._out_fifo_path = driver_details.device_fifo_directory() + 'test.fifo'
-        self._err_fifo_path = driver_details.device_fifo_directory() + 'stderr.fifo'
+        self._write_stdin_process = None
         self._read_stdout_process = None
         self._read_stderr_process = None
         self._original_kptr_restrict = None
 
         self._android_devices = android_devices
         self._device = android_devices.get_device(port._executive, worker_number)  # pylint: disable=protected-access
         self._driver_details = driver_details
         self._debug_logging = self._port._debug_logging
         self._created_cmd_line = False
         self._device_failed = False
@@ skipping 194 matching lines @@
             return ''
 
         # Use Android tool vendor/google/tools/stack to convert the raw
         # stack trace into a human readable format, if needed.
         # It takes a long time, so don't do it here.
         tombstone_contents = self._device.ReadFile(
             '/data/tombstones/%s' % last_tombstone[6])
         return '%s\n%s' % (' '.join(last_tombstone), tombstone_contents)
 
     def _get_logcat(self):
-        return list(self._device.adb.Logcat(dump=True, logcat_format='threadtime'))
+        return '\n'.join(self._device.adb.Logcat(dump=True, logcat_format='threadtime'))
 
     def _setup_performance(self):
         # Disable CPU scaling and drop ram cache to reduce noise in tests
         perf_control.PerfControl(self._device).SetPerfProfilingMode()
 
     def _teardown_performance(self):
         perf_control.PerfControl(self._device).SetDefaultPerfMode()
 
     def _get_crash_log(self, stdout, stderr, newer_than):
         if not stdout:
@@ skipping 30 matching lines @@
         return driver.Driver.cmd_line(self, pixel_tests, per_test_args)
 
     @staticmethod
     def _loop_with_timeout(condition, timeout_secs):
         deadline = time.time() + timeout_secs
         while time.time() < deadline:
             if condition():
                 return True
         return False
 
-    def _all_pipes_created(self):
-        return self._device.PathExists(
-            [self._in_fifo_path, self._out_fifo_path, self._err_fifo_path])
-
-    def _remove_all_pipes(self):
-        self._device.RunShellCommand(
-            ['rm', '-f', self._in_fifo_path, self._out_fifo_path, self._err_fifo_path],
-            check_return=True)
-        return (not self._device.PathExists(self._in_fifo_path) and
-                not self._device.PathExists(self._out_fifo_path) and
-                not self._device.PathExists(self._err_fifo_path))
-
     def start(self, pixel_tests, per_test_args, deadline):
         # We override the default start() so that we can call _android_driver_cmd_line()
         # instead of cmd_line().
         new_cmd_line = self._android_driver_cmd_line(pixel_tests, per_test_args)
 
         # Since _android_driver_cmd_line() is different than cmd_line() we need to provide
         # our own mechanism for detecting when the process should be stopped.
         if self._current_cmd_line is None:
             self._current_android_cmd_line = None
         if new_cmd_line != self._current_android_cmd_line:
             self.stop()
         self._current_android_cmd_line = new_cmd_line
 
         super(ChromiumAndroidDriver, self).start(pixel_tests, per_test_args, deadline)
 
     def _start(self, pixel_tests, per_test_args):
         if not self._android_devices.is_device_prepared(self._device.serial):
             raise driver.DeviceFailure("%s is not prepared in _start()" % self._device.serial)
 
         for retries in range(3):
             try:
                 if self._start_once(pixel_tests, per_test_args):
                     return
             except ScriptError as e:
                 self._abort('ScriptError("%s") in _start()' % str(e))
 
-            self._log_error('Failed to start the content_shell application. Retries=%d. Log:%s' % (retries, self._get_logcat()))
+            self._log_error('Failed to start the content_shell application. Retries=%d. Log:\n%s' % (retries, self._get_logcat()))
             self.stop()
             time.sleep(2)
         self._abort('Failed to start the content_shell application multiple times. Giving up.')
 
     def _start_once(self, pixel_tests, per_test_args):
         super(ChromiumAndroidDriver, self)._start(pixel_tests, per_test_args, wait_for_ready=False)
 
+        self._device.adb.Logcat(clear=True)
+
+        self._device.RunShellCommand(
+            ['rm', '-rf', self._driver_details.device_crash_dumps_directory()],
+            check_return=True)
+        self._device.RunShellCommand(
+            ['mkdir', self._driver_details.device_crash_dumps_directory()],
+            check_return=True)
+        self._device.RunShellCommand(
+            ['chmod', '-R', '777', self._driver_details.device_crash_dumps_directory()],
+            check_return=True)

[qyearsley, 2016/12/15 19:21:01]

Maybe some parts of this large function would be easier to read if parts were
extracted to helper functions and given a name. Maybe this block above is a good
candidate for extraction?

[jbudorick, 2016/12/21 00:55:16]

Done.

+
+        # Read back the shell prompt to ensure adb shell is ready.
+        deadline = time.time() + DRIVER_START_STOP_TIMEOUT_SECS
+        self._server_process.start()

[qyearsley, 2016/12/15 19:21:02]

What process does self._server_process refer to?

[jbudorick, 2016/12/21 00:55:16]

For Android, it's basically a host proxy process for the stdio streams to/from
ContentShell.apk. (I think it's an adb shell process for which we replace
std{in,out,err} after it starts up.)

+        self._read_prompt(deadline)
+        self._log_debug('Interactive shell started')
+
+        # Start a netcat process to which the test driver will connect to write stdout.
+        self._read_stdout_process, stdout_port = self._start_netcat(
+            'ReadStdout', read_from_stdin=False)
+        self._log_debug('Redirecting stdout to port %d' % stdout_port)
+
+        # Start a netcat process to which the test driver will connect to write stderr.
+        self._read_stderr_process, stderr_port = self._start_netcat(
+            'ReadStderr', first_port=stdout_port + 1, read_from_stdin=False)
+        self._log_debug('Redirecting stderr to port %d' % stderr_port)
+
+        # Start a netcat process to which the test driver will connect to read stdin.
+        self._write_stdin_process, stdin_port = self._start_netcat(
+            'WriteStdin', first_port=stderr_port + 1)
+        self._log_debug('Redirecting stdin to port %d' % stdin_port)
+
+        # Combine the stdin, stdout, and stderr pipes into self._server_process.
+        self._replace_server_process_streams()
+
         # We delay importing forwarder as long as possible because it uses fcntl,
         # which isn't available on windows.
         from devil.android import forwarder
 
         self._log_debug('Starting forwarder')
         forwarder.Forwarder.Map(
             [(p, p) for p in FORWARD_PORTS.split()],
             self._device)

[qyearsley, 2016/12/15 19:21:02]

What happens when you "start the forwarder"? Does it start a sub-process? Are
there two or more host_forwarder processes started here, when Forwarder.Map is
called twice for different purposes?

[jbudorick, 2016/12/21 00:55:16]

It starts a host_forwarder client process which then forks a daemon process if
one does not already exist. The call to Map waits for the client process to
terminate. One client process will be started (and finished) for each port being
mapped. Only one daemon process will be created, even if more than one port is
mapped.

-
-        self._device.adb.Logcat(clear=True)
+        forwarder.Forwarder.Map(
+            [(0, stdout_port), (0, stderr_port), (0, stdin_port)],
+            self._device)

[qyearsley, 2016/12/15 19:21:02]

It's not obvious from here that 0 means that a port of the device should be
allocated dynamically and can be obtained after allocation via
forwarder.Forwarder.DevicePortForHostPort; maybe a little comment would be
helpful.

[jbudorick, 2016/12/21 00:55:16]

That or a constant in forwarder.py: https://codereview.chromium.org/2587153002/

[qyearsley, 2017/01/04 19:00:32]

I agree, that is good :-)

 
         cmd_line_file_path = self._driver_details.command_line_file()
         original_cmd_line_file_path = cmd_line_file_path + '.orig'
         if (self._device.PathExists(cmd_line_file_path)
                 and not self._device.PathExists(original_cmd_line_file_path)):
             # We check for both the normal path and the backup because we do not want to step
             # on the backup. Otherwise, we'd clobber the backup whenever we changed the
             # command line during the run.
             self._device.RunShellCommand(
                 ['mv', cmd_line_file_path, original_cmd_line_file_path],
                 check_return=True)
 
+        stream_port_args = [
+            '--android-stderr-port=%s' % forwarder.Forwarder.DevicePortForHostPort(stderr_port),
+            '--android-stdin-port=%s' % forwarder.Forwarder.DevicePortForHostPort(stdin_port),
+            '--android-stdout-port=%s' % forwarder.Forwarder.DevicePortForHostPort(stdout_port),
+        ]
+        cmd_line_contents = self._android_driver_cmd_line(pixel_tests, per_test_args + stream_port_args)
         self._device.WriteFile(
             self._driver_details.command_line_file(),
-            ' '.join(self._android_driver_cmd_line(pixel_tests, per_test_args)))
+            ' '.join(cmd_line_contents))
+        self._log_debug('Command-line file contents: %s' % ' '.join(cmd_line_contents))
         self._created_cmd_line = True
 
-        self._device.RunShellCommand(
-            ['rm', '-rf', self._driver_details.device_crash_dumps_directory()],
-            check_return=True)
-        self._device.RunShellCommand(
-            ['mkdir', self._driver_details.device_crash_dumps_directory()],
-            check_return=True)
-        self._device.RunShellCommand(
-            ['chmod', '-R', '777', self._driver_details.device_crash_dumps_directory()],
-            check_return=True)
-
         try:
             self._device.StartActivity(
                 intent.Intent(
                     component=self._driver_details.activity_name(),
                     extras={'RunInSubThread': None}))
         except device_errors.CommandFailedError as exc:
             self._log_error('Failed to start the content_shell application. Exception:\n' + str(exc))
             return False
 
-        if not ChromiumAndroidDriver._loop_with_timeout(self._all_pipes_created, DRIVER_START_STOP_TIMEOUT_SECS):
-            return False
-
-        # Read back the shell prompt to ensure adb shell ready.
-        deadline = time.time() + DRIVER_START_STOP_TIMEOUT_SECS
-        self._server_process.start()
-        self._read_prompt(deadline)
-        self._log_debug('Interactive shell started')
-
-        # Start a process to read from the stdout fifo of the test driver and print to stdout.
-        self._log_debug('Redirecting stdout to ' + self._out_fifo_path)
-        self._read_stdout_process = self._port._server_process_constructor(
-            self._port, 'ReadStdout',
-            [self._device.adb.GetAdbPath(), '-s', self._device.serial, 'shell', 'cat', self._out_fifo_path])
-        self._read_stdout_process.start()
-
-        # Start a process to read from the stderr fifo of the test driver and print to stdout.
-        self._log_debug('Redirecting stderr to ' + self._err_fifo_path)
-        self._read_stderr_process = self._port._server_process_constructor(
-            self._port, 'ReadStderr',
-            [self._device.adb.GetAdbPath(), '-s', self._device.serial, 'shell', 'cat', self._err_fifo_path])
-        self._read_stderr_process.start()
-
-        self._log_debug('Redirecting stdin to ' + self._in_fifo_path)
-        self._server_process.write('cat >%s\n' % self._in_fifo_path)
-
-        # Combine the stdout and stderr pipes into self._server_process.
-        self._server_process.replace_outputs(self._read_stdout_process._proc.stdout, self._read_stderr_process._proc.stdout)
-
-        def deadlock_detector(processes, normal_startup_event):
-            if not ChromiumAndroidDriver._loop_with_timeout(lambda: normal_startup_event.is_set(), DRIVER_START_STOP_TIMEOUT_SECS):
-                # If normal_startup_event is not set in time, the main thread must be blocked at
-                # reading/writing the fifo. Kill the fifo reading/writing processes to let the
-                # main thread escape from the deadlocked state. After that, the main thread will
-                # treat this as a crash.
-                self._log_error('Deadlock detected. Processes killed.')
-                for i in processes:
-                    i.kill()
-
-        # Start a thread to kill the pipe reading/writing processes on deadlock of the fifos during startup.
-        normal_startup_event = threading.Event()
-        threading.Thread(
-            name='DeadlockDetector',
-            target=deadlock_detector,
-            args=([self._server_process, self._read_stdout_process, self._read_stderr_process], normal_startup_event)).start()
-
-        # The test driver might crash during startup or when the deadlock detector hits
-        # a deadlock and kills the fifo reading/writing processes.
+        # The test driver might crash during startup.
         if not self._wait_for_server_process_output(self._server_process, deadline, '#READY'):
             return False
 
-        # Inform the deadlock detector that the startup is successful without deadlock.
-        normal_startup_event.set()
         self._log_debug("content_shell is ready")
         return True
 
+    def _start_netcat(self, server_name, first_port=10201, read_from_stdin=True):

[qyearsley, 2016/12/15 19:21:02]

Server name must always be one of {"ReadStderr", "ReadStdout", "WriteStdin"},
right?

Are these names arbitrary? (Could they just be changed in this file to something
else?) Or are these strings determined elsewhere?

Why is 10201 the first port to try?

What role does netcat play here?

[jbudorick, 2016/12/21 00:55:16]

The three call sites provide those, but there's nothing that would break if we
renamed them.
Arbitrary.
No particular reason.
It opens a socket for ContentShell to connect to for the stdio streams and reads
from stdin + writes to stdout as appropriate.

[qyearsley, 2017/01/04 19:00:32]

Optionally, if you think it makes it clearer, we could add constants for the
server name strings and the first port to try, with a short comment saying that
they're arbitrary.

[jbudorick, 2017/01/04 19:50:15]

I think it helps for the port and have added a constant for that. I don't think
it does for the server names, though.

+        for i in itertools.count(first_port, 65536):
+            nc_cmd = ['nc', '-l', str(i)]
+            if not read_from_stdin:
+                nc_cmd.append('-d')
+            proc = self._port.server_process_constructor(self._port, server_name, nc_cmd)
+            proc.start()
+            self._port.host.executive.wait_limited(proc.pid(), limit_in_seconds=1)
+            if self._port.host.executive.check_running_pid(proc.pid()):
+                return (proc, i)

[qyearsley, 2016/12/15 19:21:02]

What would happen hypothetically if this loop exits after counting to 65536 for
some reason?

[jbudorick, 2016/12/21 00:55:16]

hmm, this would return None and we'd get a confusing-looking exception on trying
to unpack it.

Made this throw an Exception.

[qyearsley, 2017/01/04 19:00:32]

SGTM

+
+    def _replace_server_process_streams(self):
+        # pylint: disable=protected-access
+        self._server_process.replace_input(
+            self._write_stdin_process._proc.stdin)
+        self._server_process.replace_outputs(
+            self._read_stdout_process._proc.stdout,
+            self._read_stderr_process._proc.stdout)
+
     def _pid_on_target(self):
         pids = self._device.GetPids(self._driver_details.package_name())
         return pids.get(self._driver_details.package_name())
 
     def stop(self):
         if not self._device_failed:
             # Do not try to stop the application if there's something wrong with the device; adb may hang.
             # FIXME: crbug.com/305040. Figure out if it's really hanging (and why).
             self._device.ForceStop(self._driver_details.package_name())
 
+        if self._write_stdin_process:
+            self._write_stdin_process.kill()
+            self._write_stdin_process = None
+
         if self._read_stdout_process:
             self._read_stdout_process.kill()
             self._read_stdout_process = None
 
         if self._read_stderr_process:
             self._read_stderr_process.kill()
             self._read_stderr_process = None
 
         # We delay importing forwarder as long as possible because it uses fcntl,
         # which isn't available on windows.
         from devil.android import forwarder
 
         forwarder.Forwarder.KillDevice(self._device)
         forwarder.Forwarder.KillHost()
 
         super(ChromiumAndroidDriver, self).stop()
 
-        if self._android_devices.is_device_prepared(self._device.serial):
-            if not ChromiumAndroidDriver._loop_with_timeout(self._remove_all_pipes, DRIVER_START_STOP_TIMEOUT_SECS):
-                self._abort('Failed to remove fifo files. May be locked.')
-
         self._clean_up_cmd_line()
 
     def _pull_crash_dumps_from_device(self):
         result = []
         if not self._device.PathExists(self._driver_details.device_crash_dumps_directory()):
             return result
         dumps = self._device.ListDirectory(
             self._driver_details.device_crash_dumps_directory())
         for dump in dumps:
             device_dump = '%s/%s' % (self._driver_details.device_crash_dumps_directory(), dump)
@@ skipping 40 matching lines @@
         return command
 
     def _read_prompt(self, deadline):
         last_char = ''
         while True:
             current_char = self._server_process.read_stdout(deadline, 1)
             if current_char == ' ':
                 if last_char in ('#', '$'):
                     return
             last_char = current_char