[XvfbDriver] Fail to run all layout tests when X server started with -displayfd option
authorcarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Nov 2015 09:05:28 +0000 (09:05 +0000)
committercarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Nov 2015 09:05:28 +0000 (09:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=151135

Reviewed by Darin Adler.

The XvfbDriver uses the x server command line to check the
displays that are currently in use. This doesn't work when X
server was started with -displayfd option. This option is used to let
the server find the display id available that is written to the
given file descriptor. With this option xorg doesn't need to
create the lock files in tmp either. The -displayfd option is also
available in Xvfb, so we could use it too. That would simplify the
code, fixing also race conditions between the check for available
displays and Xvfb opening the connection, we wouldn't need to wait
for 4 seconds after launching Xvfb, and all lock files we are
using won't be needed either.

* Scripts/webkitpy/port/xvfbdriver.py:
(XvfbDriver._xvfb_pipe): Helper function to create the pipe, only
needed to be overriden by unit tests.
(XvfbDriver._xvfb_read_display_id): Helper function to read from
the pipe, only needed to be overriden by unit tests.
(XvfbDriver._xvfb_run): Run Xvfb with -displayfd option, using a
pipe to read the display id.
(XvfbDriver._start): Call _xvfb_run() and remove the code to run
Xvfb for a given display.
(XvfbDriver.stop): Remove code to release and delete file locks.
* Scripts/webkitpy/port/xvfbdriver_unittest.py:
(XvfbDriverTest.make_driver):
(XvfbDriverTest.test_start):
(XvfbDriverTest.test_start_arbitrary_worker_number):
(XvfbDriverTest.test_stop):
(XvfbDriverTest.assertDriverStartSuccessful): Deleted.
(XvfbDriverTest): Deleted.
(XvfbDriverTest.test_stop.FakeXvfbProcess): Deleted.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@192568 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/Scripts/webkitpy/port/xvfbdriver.py
Tools/Scripts/webkitpy/port/xvfbdriver_unittest.py

index c8b142b..446ee29 100644 (file)
@@ -1,3 +1,41 @@
+2015-11-18  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [XvfbDriver] Fail to run all layout tests when X server started with -displayfd option
+        https://bugs.webkit.org/show_bug.cgi?id=151135
+
+        Reviewed by Darin Adler.
+
+        The XvfbDriver uses the x server command line to check the
+        displays that are currently in use. This doesn't work when X
+        server was started with -displayfd option. This option is used to let
+        the server find the display id available that is written to the
+        given file descriptor. With this option xorg doesn't need to
+        create the lock files in tmp either. The -displayfd option is also
+        available in Xvfb, so we could use it too. That would simplify the
+        code, fixing also race conditions between the check for available
+        displays and Xvfb opening the connection, we wouldn't need to wait
+        for 4 seconds after launching Xvfb, and all lock files we are
+        using won't be needed either.
+
+        * Scripts/webkitpy/port/xvfbdriver.py:
+        (XvfbDriver._xvfb_pipe): Helper function to create the pipe, only
+        needed to be overriden by unit tests.
+        (XvfbDriver._xvfb_read_display_id): Helper function to read from
+        the pipe, only needed to be overriden by unit tests.
+        (XvfbDriver._xvfb_run): Run Xvfb with -displayfd option, using a
+        pipe to read the display id.
+        (XvfbDriver._start): Call _xvfb_run() and remove the code to run
+        Xvfb for a given display.
+        (XvfbDriver.stop): Remove code to release and delete file locks.
+        * Scripts/webkitpy/port/xvfbdriver_unittest.py:
+        (XvfbDriverTest.make_driver):
+        (XvfbDriverTest.test_start):
+        (XvfbDriverTest.test_start_arbitrary_worker_number):
+        (XvfbDriverTest.test_stop):
+        (XvfbDriverTest.assertDriverStartSuccessful): Deleted.
+        (XvfbDriverTest): Deleted.
+        (XvfbDriverTest.test_stop.FakeXvfbProcess): Deleted.
+
 2015-11-17  Alexey Proskuryakov  <ap@apple.com>
 
         run-webkit-tests should not truncate persistent lines
index b66948a..01b8ea1 100644 (file)
@@ -34,7 +34,6 @@ import time
 
 from webkitpy.port.server_process import ServerProcess
 from webkitpy.port.driver import Driver
-from webkitpy.common.system.file_lock import FileLock
 
 _log = logging.getLogger(__name__)
 
@@ -50,51 +49,55 @@ class XvfbDriver(Driver):
             _log.error("No Xvfb found. Cannot run layout tests.")
         return xvfb_found
 
-    def __init__(self, *args, **kwargs):
-        Driver.__init__(self, *args, **kwargs)
-        self._guard_lock = None
-        self._startup_delay_secs = 1.0
-
-    def _next_free_display(self):
-        running_pids = self._port.host.executive.run_command(['ps', '-eo', 'comm,command'])
-        reserved_screens = set()
-        for pid in running_pids.split('\n'):
-            match = re.match('(X|Xvfb|Xorg|Xorg\.bin)\s+.*\s:(?P<screen_number>\d+)', pid)
-            if match:
-                reserved_screens.add(int(match.group('screen_number')))
-        for i in range(1, 99):
-            if i not in reserved_screens:
-                _guard_lock_file = self._port.host.filesystem.join('/tmp', 'WebKitXvfb.lock.%i' % i)
-                self._guard_lock = self._port.host.make_file_lock(_guard_lock_file)
-                if self._guard_lock.acquire_lock():
-                    return i
+    def _xvfb_pipe(self):
+        return os.pipe()
 
-    def _xvfb_screen_depth(self):
-        return os.environ.get('XVFB_SCREEN_DEPTH', '24')
+    def _xvfb_read_display_id(self, read_fd):
+        import errno
+        import select
 
-    def _start(self, pixel_tests, per_test_args):
-        self.stop()
+        fd_set = [read_fd]
+        while fd_set:
+            try:
+                fd_list = select.select(fd_set, [], [])[0]
+            except select.error, e:
+                if e.args[0] == errno.EINTR:
+                    continue
+                raise
 
-        # Use even displays for pixel tests and odd ones otherwise. When pixel tests are disabled,
-        # DriverProxy creates two drivers, one for normal and the other for ref tests. Both have
-        # the same worker number, so this prevents them from using the same Xvfb instance.
-        display_id = self._next_free_display()
-        self._lock_file = "/tmp/.X%d-lock" % display_id
+            if read_fd in fd_list:
+                # We only expect a number, so first read should be enough.
+                display_id = os.read(read_fd, 256).strip('\n')
+                fd_set = []
 
-        server_name = self._port.driver_name()
-        environment = self._port.setup_environ_for_server(server_name)
-
-        run_xvfb = ["Xvfb", ":%d" % display_id, "-screen",  "0", "1024x768x%s" % self._xvfb_screen_depth(), "-nolisten", "tcp"]
+        return int(display_id)
 
+    def _xvfb_run(self, environment):
+        read_fd, write_fd = self._xvfb_pipe()
+        run_xvfb = ["Xvfb", "-displayfd", str(write_fd), "-screen",  "0", "1024x768x%s" % self._xvfb_screen_depth(), "-nolisten", "tcp"]
         if self._port._should_use_jhbuild():
-                run_xvfb = self._port._jhbuild_wrapper + run_xvfb
-
+            run_xvfb = self._port._jhbuild_wrapper + run_xvfb
         with open(os.devnull, 'w') as devnull:
             self._xvfb_process = self._port.host.executive.popen(run_xvfb, stderr=devnull, env=environment)
+            display_id = self._xvfb_read_display_id(read_fd)
+
+        try:
+            os.close(read_fd)
+            os.close(write_fd)
+        except OSError:
+            pass
 
-        # Crashes intend to occur occasionally in the first few tests that are run through each
-        # worker because the Xvfb display isn't ready yet. Halting execution a bit should avoid that.
-        time.sleep(self._startup_delay_secs)
+        return display_id
+
+    def _xvfb_screen_depth(self):
+        return os.environ.get('XVFB_SCREEN_DEPTH', '24')
+
+    def _start(self, pixel_tests, per_test_args):
+        self.stop()
+
+        server_name = self._port.driver_name()
+        environment = self._port.setup_environ_for_server(server_name)
+        display_id = self._xvfb_run(environment)
 
         # We must do this here because the DISPLAY number depends on _worker_number
         environment['DISPLAY'] = ":%d" % display_id
@@ -115,11 +118,6 @@ class XvfbDriver(Driver):
 
     def stop(self):
         super(XvfbDriver, self).stop()
-        if self._guard_lock:
-            self._guard_lock.release_lock()
-            self._guard_lock = None
         if getattr(self, '_xvfb_process', None):
             self._port.host.executive.kill_process(self._xvfb_process.pid)
             self._xvfb_process = None
-            if self._port.host.filesystem.exists(self._lock_file):
-                self._port.host.filesystem.remove(self._lock_file)
index e0af1bb..3761f4b 100644 (file)
@@ -52,6 +52,8 @@ class XvfbDriverTest(unittest.TestCase):
         driver = XvfbDriver(port, worker_number=worker_number, pixel_tests=True)
         driver._startup_delay_secs = 0
         driver._xvfb_screen_depth = lambda: '24'
+        driver._xvfb_pipe = lambda: (3, 4)
+        driver._xvfb_read_display_id = lambda(x): 1
         driver._environment = port.setup_environ_for_server(port.driver_name())
         return driver
 
@@ -66,71 +68,20 @@ class XvfbDriverTest(unittest.TestCase):
         self.assertTrue(driver._server_process.started)
         self.assertEqual(driver._server_process.env["DISPLAY"], expected_display)
 
-    def test_start_no_pixel_tests(self):
+    def test_start(self):
         driver = self.make_driver()
-        expected_logs = ("MOCK run_command: ['ps', '-eo', 'comm,command'], cwd=None\nMOCK popen: ['Xvfb', ':1', '-screen', '0', '1024x768x24', '-nolisten', 'tcp'], env=%s\n" % driver._environment)
+        expected_logs = ("MOCK popen: ['Xvfb', '-displayfd', '4', '-screen', '0', '1024x768x24', '-nolisten', 'tcp'], env=%s\n" % driver._environment)
         self.assertDriverStartSuccessful(driver, expected_logs=expected_logs, expected_display=":1")
         self.cleanup_driver(driver)
 
-    def test_start_pixel_tests(self):
-        driver = self.make_driver()
-        expected_logs = ("MOCK run_command: ['ps', '-eo', 'comm,command'], cwd=None\nMOCK popen: ['Xvfb', ':1', '-screen', '0', '1024x768x24', '-nolisten', 'tcp'], env=%s\n" % driver._environment)
-        self.assertDriverStartSuccessful(driver, expected_logs=expected_logs, expected_display=":1", pixel_tests=True)
-        self.cleanup_driver(driver)
-
     def test_start_arbitrary_worker_number(self):
         driver = self.make_driver(worker_number=17)
-        expected_logs = ("MOCK run_command: ['ps', '-eo', 'comm,command'], cwd=None\nMOCK popen: ['Xvfb', ':1', '-screen', '0', '1024x768x24', '-nolisten', 'tcp'], env=%s\n" % driver._environment)
+        expected_logs = ("MOCK popen: ['Xvfb', '-displayfd', '4', '-screen', '0', '1024x768x24', '-nolisten', 'tcp'], env=%s\n" % driver._environment)
         self.assertDriverStartSuccessful(driver, expected_logs=expected_logs, expected_display=":1", pixel_tests=True)
         self.cleanup_driver(driver)
 
-    def test_next_free_display(self):
-        output = "Xorg            /usr/bin/X :1 -auth /var/run/lightdm/root/:1 -nolisten tcp vt7 -novtswitch -background none\nXvfb            Xvfb :2 -screen 0 800x600x24 -nolisten tcp"
-        executive = MockExecutive2(output)
-        driver = self.make_driver(executive=executive)
-        self.assertEqual(driver._next_free_display(), 3)
-        self.cleanup_driver(driver)
-        output = "X               /usr/bin/X :1 vt7 -nolisten tcp -auth /var/run/xauth/A:0-8p7Ybb"
-        executive = MockExecutive2(output)
-        driver = self.make_driver(executive=executive)
-        self.assertEqual(driver._next_free_display(), 2)
-        self.cleanup_driver(driver)
-        output = "Xvfb            Xvfb :1 -screen 0 800x600x24 -nolisten tcp"
-        executive = MockExecutive2(output)
-        driver = self.make_driver(executive=executive)
-        self.assertEqual(driver._next_free_display(), 2)
-        self.cleanup_driver(driver)
-        output = "Xvfb            Xvfb :2 -screen 0 800x600x24 -nolisten tcp\nXvfb            Xvfb :1 -screen 0 800x600x24 -nolisten tcp\nXvfb            Xvfb :3 -screen 0 800x600x24 -nolisten tcp"
-        executive = MockExecutive2(output)
-        driver = self.make_driver(executive=executive)
-        self.assertEqual(driver._next_free_display(), 4)
-        self.cleanup_driver(driver)
-        output = "Xorg.bin        /usr/libexec/Xorg.bin :1 -background none -noreset -verbose 3 -logfile /dev/null -auth /run/gdm/auth-for-gdm-Zt8Eq2/database -seat seat0 -nolisten tcp vt1"
-        executive = MockExecutive2(output)
-        driver = self.make_driver(executive=executive)
-        self.assertEqual(driver._next_free_display(), 2)
-        self.cleanup_driver(driver)
-        output = "/usr/libexec/Xorg            vt2 -displayfd 3 -auth /run/user/1000/gdm/Xauthority -nolisten tcp -background none -noreset -keeptty -verbose 3"
-        executive = MockExecutive2(output)
-        driver = self.make_driver(executive=executive)
-        self.assertEqual(driver._next_free_display(), 1)
-        self.cleanup_driver(driver)
-
-    def test_start_next_worker(self):
-        driver = self.make_driver()
-        driver._next_free_display = lambda: 1
-        expected_logs = ("MOCK popen: ['Xvfb', ':1', '-screen', '0', '1024x768x24', '-nolisten', 'tcp'], env=%s\n" % driver._environment)
-        self.assertDriverStartSuccessful(driver, expected_logs=expected_logs, expected_display=":1", pixel_tests=True)
-        self.cleanup_driver(driver)
-        driver = self.make_driver()
-        driver._next_free_display = lambda: 3
-        expected_logs = ("MOCK popen: ['Xvfb', ':3', '-screen', '0', '1024x768x24', '-nolisten', 'tcp'], env=%s\n" % driver._environment)
-        self.assertDriverStartSuccessful(driver, expected_logs=expected_logs, expected_display=":3", pixel_tests=True)
-        self.cleanup_driver(driver)
-
     def test_stop(self):
-        filesystem = MockFileSystem(files={'/tmp/.X42-lock': '1234\n'})
-        port = Port(MockSystemHost(log_executive=True, filesystem=filesystem), 'xvfbdrivertestport', options=MockOptions(configuration='Release'))
+        port = Port(MockSystemHost(log_executive=True), 'xvfbdrivertestport', options=MockOptions(configuration='Release'))
         port._executive.kill_process = lambda x: _log.info("MOCK kill_process pid: " + str(x))
         driver = XvfbDriver(port, worker_number=0, pixel_tests=True)
 
@@ -138,10 +89,8 @@ class XvfbDriverTest(unittest.TestCase):
             pid = 1234
 
         driver._xvfb_process = FakeXvfbProcess()
-        driver._lock_file = '/tmp/.X42-lock'
 
         expected_logs = "MOCK kill_process pid: 1234\n"
         OutputCapture().assert_outputs(self, driver.stop, [], expected_logs=expected_logs)
 
         self.assertIsNone(driver._xvfb_process)
-        self.assertFalse(port._filesystem.exists(driver._lock_file))