[Test Automation] Use xvfb directly instead of xvfb-run

Thereby fix signal handling for SIGTERM and SIGINT.

The steps needed to start Xvfb are drawn from:
https://github.com/cgoldberg/xvfbwrapper/blob/master/xvfbwrapper.py
https://github.com/revnode/xvfb-run/blob/master/xvfb-run

Added unit tests to presubmit to test xvfb.py


Bug: 932240
Change-Id: I3b9439991697ae94e98b93e4f1fcfd411a451536
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1548424
Commit-Queue: Ilia Samsonov <isamsonov@google.com>
Reviewed-by: Caleb Rouleau <crouleau@chromium.org>
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: John Chen <johnchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652562}

testing/PRESUBMIT.py

@@ -12,6 +12,8 @@ for more details on the presubmit API built into depot_tools.
 def CommonChecks(input_api, output_api):
   output = []
   blacklist = [r'gmock.*', r'gtest.*']
+  output.extend(input_api.canned_checks.RunUnitTestsInDirectory(
+      input_api, output_api, '.', [r'^.+_unittest\.py$']))
   output.extend(input_api.canned_checks.RunPylint(
       input_api, output_api, black_list=blacklist))
   return output

testing/test_env.py

@@ -253,6 +253,8 @@ def forward_signals(procs):
   assert all(isinstance(p, subprocess.Popen) for p in procs)
   def _sig_handler(sig, _):
     for p in procs:
+      if p.poll() is not None:
+        continue
       # SIGBREAK is defined only for win32.
       if sys.platform == 'win32' and sig == signal.SIGBREAK:
         p.send_signal(signal.CTRL_BREAK_EVENT)
@@ -262,7 +264,7 @@ def forward_signals(procs):
     signal.signal(signal.SIGBREAK, _sig_handler)
   else:
     signal.signal(signal.SIGTERM, _sig_handler)
-
+    signal.signal(signal.SIGINT, _sig_handler)
 
 def run_executable(cmd, env, stdoutfile=None):
   """Runs an executable with:

testing/xvfb.py

@@ -7,15 +7,20 @@
 
 import os
 import os.path
-import platform
+import random
 import signal
 import subprocess
 import sys
 import threading
-
+import time
 import test_env
 
 
+class _XvfbProcessError(Exception):
+  """Exception raised when Xvfb cannot start."""
+  pass
+
+
 def _kill(proc, send_signal):
   """Kills |proc| and ignores exceptions thrown for non-existent processes."""
   try:
@@ -35,21 +40,37 @@ def kill(proc, timeout_in_seconds=10):
 
   thread.join(timeout_in_seconds)
   if thread.is_alive():
-    print >> sys.stderr, 'Xvfb running after SIGTERM, trying SIGKILL.'
+    print >> sys.stderr, '%s running after SIGTERM, trying SIGKILL.' % proc.name
     _kill(proc, signal.SIGKILL)
 
   thread.join(timeout_in_seconds)
   if thread.is_alive():
-    print >> sys.stderr, 'Xvfb running after SIGTERM and SIGKILL; good luck!'
+    print >> sys.stderr, \
+      '%s running after SIGTERM and SIGKILL; good luck!' % proc.name
 
 
-def run_executable(cmd, env, stdoutfile=None):
+# TODO(crbug.com/949194): Encourage setting flags to False.
+def run_executable(
+    cmd, env, stdoutfile=None, use_openbox=True, use_xcompmgr=True):
   """Runs an executable within Xvfb on Linux or normally on other platforms.
 
-  If |stdoutfile| is provided, symbolization via script is disabled and stdout
-  is written to this file as well as to stdout.
-
-  Returns the exit code of the specified commandline, or 1 on failure.
+  The method sets SIGUSR1 handler for Xvfb to return SIGUSR1
+  when it is ready for connections.
+  https://www.x.org/archive/X11R7.5/doc/man/man1/Xserver.1.html under Signals.
+
+  Args:
+    cmd: Command to be executed.
+    env: A copy of environment variables, "DISPLAY" and
+      "_CHROMIUM_INSIDE_XVFB" will be set if Xvfb is used.
+    stdoutfile: If provided, symbolization via script is disabled and stdout
+      is written to this file as well as to stdout.
+    use_openbox: A flag to use openbox process.
+      Some ChromeOS tests need a window manager.
+    use_xcompmgr: A flag to use xcompmgr process.
+      Some tests need a compositing wm to make use of transparent visuals.
+
+  Returns:
+    the exit code of the specified commandline, or 1 on failure.
   """
 
   # It might seem counterintuitive to support a --no-xvfb flag in a script
@@ -63,45 +84,113 @@ def run_executable(cmd, env, stdoutfile=None):
     cmd.remove('--no-xvfb')
 
   if sys.platform == 'linux2' and use_xvfb:
-    if env.get('_CHROMIUM_INSIDE_XVFB') == '1':
-      openbox_proc = None
-      xcompmgr_proc = None
-      try:
-        # Some ChromeOS tests need a window manager.
-        openbox_proc = subprocess.Popen('openbox', stdout=subprocess.PIPE,
-                                        stderr=subprocess.STDOUT, env=env)
-
-        # Some tests need a compositing wm to make use of transparent visuals.
-        xcompmgr_proc = subprocess.Popen('xcompmgr', stdout=subprocess.PIPE,
-                                         stderr=subprocess.STDOUT, env=env)
-
-        return test_env.run_executable(cmd, env, stdoutfile)
-      except OSError as e:
-        print >> sys.stderr, 'Failed to start Xvfb or Openbox: %s' % str(e)
-        return 1
-      finally:
-        kill(openbox_proc)
-        kill(xcompmgr_proc)
-    else:
-      env['_CHROMIUM_INSIDE_XVFB'] = '1'
-      if stdoutfile:
-        env['_XVFB_EXECUTABLE_STDOUTFILE'] = stdoutfile
-      xvfb_script = __file__
-      if xvfb_script.endswith('.pyc'):
-        xvfb_script = xvfb_script[:-1]
-      # TODO(crbug.com/932240): Propagate signals properly.
-      return subprocess.call([
-          'xvfb-run', '-a', "--server-args=-screen 0 "
-          "1280x800x24 -ac -nolisten tcp -dpi 96 "
-          "+extension RANDR", xvfb_script] + cmd, env=env)
+    env['_CHROMIUM_INSIDE_XVFB'] = '1'
+    openbox_proc = None
+    xcompmgr_proc = None
+    xvfb_proc = None
+    xvfb_ready = MutableBoolean()
+    def set_xvfb_ready(*_):
+      xvfb_ready.setvalue(True)
+
+    try:
+      signal.signal(signal.SIGTERM, raise_xvfb_error)
+      signal.signal(signal.SIGINT, raise_xvfb_error)
+
+      # Due to race condition for display number, Xvfb might fail to run.
+      # If it does fail, try again up to 10 times, similarly to xvfb-run.
+      for _ in range(10):
+        xvfb_ready.setvalue(False)
+        display = find_display()
+
+        # Sets SIGUSR1 to ignore for Xvfb to signal current process
+        # when it is ready. Due to race condition, USR1 signal could be sent
+        # before the process resets the signal handler, we cannot rely on
+        # signal handler to change on time.
+        signal.signal(signal.SIGUSR1, signal.SIG_IGN)
+        xvfb_proc = subprocess.Popen(
+            ['Xvfb', display, '-screen', '0', '1280x800x24', '-ac',
+             '-nolisten', 'tcp', '-dpi', '96', '+extension', 'RANDR'],
+            stderr=subprocess.STDOUT, env=env)
+        signal.signal(signal.SIGUSR1, set_xvfb_ready)
+        for _ in range(10):
+          time.sleep(.1)  # gives Xvfb time to start or fail.
+          if xvfb_ready.getvalue() or xvfb_proc.poll() is not None:
+            break  # xvfb sent ready signal, or already failed and stopped.
+
+        if xvfb_proc.poll() is None:
+          break  # xvfb is running, can proceed.
+      if xvfb_proc.poll() is not None:
+        raise _XvfbProcessError('Failed to start after 10 tries')
+
+      env['DISPLAY'] = display
+
+      if use_openbox:
+        openbox_proc = subprocess.Popen(
+            'openbox', stderr=subprocess.STDOUT, env=env)
+
+      if use_xcompmgr:
+        xcompmgr_proc = subprocess.Popen(
+            'xcompmgr', stderr=subprocess.STDOUT, env=env)
+
+      return test_env.run_executable(cmd, env, stdoutfile)
+    except OSError as e:
+      print >> sys.stderr, 'Failed to start Xvfb or Openbox: %s' % str(e)
+      return 1
+    except _XvfbProcessError as e:
+      print >> sys.stderr, 'Xvfb fail: %s' % str(e)
+      return 1
+    finally:
+      kill(openbox_proc)
+      kill(xcompmgr_proc)
+      kill(xvfb_proc)
   else:
     return test_env.run_executable(cmd, env, stdoutfile)
 
 
+class MutableBoolean(object):
+  """Simple mutable boolean class. Used to be mutated inside an handler."""
+
+  def __init__(self):
+    self._val = False
+
+  def setvalue(self, val):
+    assert isinstance(val, bool)
+    self._val = val
+
+  def getvalue(self):
+    return self._val
+
+
+def raise_xvfb_error(*_):
+  raise _XvfbProcessError('Terminated')
+
+
+def find_display():
+  """Iterates through X-lock files to find an available display number.
+
+  The lower bound follows xvfb-run standard at 99, and the upper bound
+  is set to 119.
+
+  Returns:
+    A string of a random available display number for Xvfb ':{99-119}'.
+
+  Raises:
+    _XvfbProcessError: Raised when displays 99 through 119 are unavailable.
+  """
+
+  available_displays = [
+      d for d in range(99, 120)
+      if not os.path.isfile('/tmp/.X{}-lock'.format(d))
+  ]
+  if available_displays:
+    return ':{}'.format(random.choice(available_displays))
+  raise _XvfbProcessError('Failed to find display number')
+
+
 def main():
-  USAGE = 'Usage: xvfb.py [command [--no-xvfb] args...]'
+  usage = 'Usage: xvfb.py [command [--no-xvfb] args...]'
   if len(sys.argv) < 2:
-    print >> sys.stderr, USAGE
+    print >> sys.stderr, usage
     return 2
 
   # If the user still thinks the first argument is the execution directory then
@@ -109,14 +198,11 @@ def main():
   if os.path.isdir(sys.argv[1]):
     print >> sys.stderr, (
         'Invalid command: \"%s\" is a directory' % sys.argv[1])
-    print >> sys.stderr, USAGE
+    print >> sys.stderr, usage
     return 3
 
-  stdoutfile = os.environ.get('_XVFB_EXECUTABLE_STDOUTFILE')
-  if stdoutfile:
-    del os.environ['_XVFB_EXECUTABLE_STDOUTFILE']
-  return run_executable(sys.argv[1:], os.environ.copy(), stdoutfile)
+  return run_executable(sys.argv[1:], os.environ.copy())
 
 
-if __name__ == "__main__":
+if __name__ == '__main__':
   sys.exit(main())

testing/xvfb_test_script.py

+#!/usr/bin/env python
+# Copyright (c) 2019 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Simple script for xvfb_unittest to launch.
+
+This script outputs formatted data to stdout for the xvfb unit tests
+to read and compare with expected output.
+"""
+
+import os
+import signal
+import sys
+import time
+
+
+def print_signal(sig, *_):
+  print 'Signal :{}'.format(sig)
+
+
+if __name__ == '__main__':
+  signal.signal(signal.SIGTERM, print_signal)
+  signal.signal(signal.SIGINT, print_signal)
+
+  # test if inside xvfb flag is set.
+  print 'Inside_xvfb :{}'.format(
+      os.environ.get('_CHROMIUM_INSIDE_XVFB', 'None'))
+  # test the subprocess display number.
+  print 'Display :{}'.format(os.environ.get('DISPLAY', 'None'))
+
+  if len(sys.argv) > 1 and sys.argv[1] == '--sleep':
+    time.sleep(2)  # gives process time to receive signal.

testing/xvfb_unittest.py

+#!/usr/bin/env python
+# Copyright (c) 2019 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Unit tests for xvfb.py functionality.
+
+Each unit test is launching xvfb_test_script.py
+through xvfb.py as a subprocess, then tests its expected output.
+"""
+
+import os
+import signal
+import subprocess
+import sys
+import time
+import unittest
+
+
+TEST_FILE = __file__.replace('.pyc', '.py')
+XVFB = TEST_FILE.replace('_unittest', '')
+XVFB_TEST_SCRIPT = TEST_FILE.replace('_unittest', '_test_script')
+
+
+def launch_process(args):
+  """Launches a sub process to run through xvfb.py."""
+  return subprocess.Popen(
+      [XVFB, XVFB_TEST_SCRIPT] + args, stdout=subprocess.PIPE,
+      stderr=subprocess.STDOUT, env=os.environ.copy())
+
+
+def read_subprocess_message(proc, starts_with):
+  """Finds the value after first line prefix condition."""
+  for line in proc.stdout:
+    if line.startswith(starts_with):
+      return line.rstrip().replace(starts_with, '')
+
+
+def send_signal(proc, sig, sleep_time=0.3):
+  """Sends a signal to subprocess."""
+  time.sleep(sleep_time)  # gives process time to launch.
+  os.kill(proc.pid, sig)
+  proc.wait()
+
+
+class XvfbLinuxTest(unittest.TestCase):
+
+  def setUp(self):
+    super(XvfbLinuxTest, self).setUp()
+    if sys.platform != 'linux2':
+      self.skipTest('linux only test')
+
+  def test_no_xvfb_display(self):
+    proc = launch_process(['--no-xvfb'])
+    proc.wait()
+    display = read_subprocess_message(proc, 'Display :')
+    self.assertEqual(display, os.environ.get('DISPLAY', 'None'))
+
+  def test_xvfb_display(self):
+    proc = launch_process([])
+    proc.wait()
+    display = read_subprocess_message(proc, 'Display :')
+    self.assertIsNotNone(display)
+    self.assertNotEqual(display, os.environ.get('DISPLAY', 'None'))
+
+  def test_no_xvfb_flag(self):
+    proc = launch_process(['--no-xvfb'])
+    proc.wait()
+    environ_flag = read_subprocess_message(proc, 'Inside_xvfb :')
+    self.assertEqual(environ_flag, 'None')
+
+  def test_xvfb_flag(self):
+    proc = launch_process([])
+    proc.wait()
+    environ_flag = read_subprocess_message(proc, 'Inside_xvfb :')
+    self.assertEqual(environ_flag, '1')
+
+  def test_xvfb_race_condition(self):
+    proc_list = [launch_process([]) for _ in range(15)]
+    for proc in proc_list:
+      proc.wait()
+    display_list = [read_subprocess_message(p, 'Display :') for p in proc_list]
+    for display in display_list:
+      self.assertIsNotNone(display)
+      self.assertNotEqual(display, os.environ.get('DISPLAY', 'None'))
+
+
+class XvfbTest(unittest.TestCase):
+
+  def setUp(self):
+    super(XvfbTest, self).setUp()
+    if sys.platform == 'win32':
+      self.skipTest('non-win32 test')
+
+  def test_send_sigint(self):
+    proc = launch_process(['--sleep'])
+    send_signal(proc, signal.SIGINT)
+    sig = read_subprocess_message(proc, 'Signal :')
+    self.assertEqual(sig, str(signal.SIGINT))
+
+  def test_send_sigterm(self):
+    proc = launch_process(['--sleep'])
+    send_signal(proc, signal.SIGTERM)
+    sig = read_subprocess_message(proc, 'Signal :')
+    self.assertEqual(sig, str(signal.SIGTERM))
+
+if __name__ == '__main__':
+  unittest.main()