Title: [192568] trunk/Tools
Revision
192568
Author
carlo...@webkit.org
Date
2015-11-18 01:05:28 -0800 (Wed, 18 Nov 2015)

Log Message

[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.

Modified Paths

Diff

Modified: trunk/Tools/ChangeLog (192567 => 192568)


--- trunk/Tools/ChangeLog	2015-11-18 07:52:47 UTC (rev 192567)
+++ trunk/Tools/ChangeLog	2015-11-18 09:05:28 UTC (rev 192568)
@@ -1,3 +1,41 @@
+2015-11-18  Carlos Garcia Campos  <cgar...@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  <a...@apple.com>
 
         run-webkit-tests should not truncate persistent lines

Modified: trunk/Tools/Scripts/webkitpy/port/xvfbdriver.py (192567 => 192568)


--- trunk/Tools/Scripts/webkitpy/port/xvfbdriver.py	2015-11-18 07:52:47 UTC (rev 192567)
+++ trunk/Tools/Scripts/webkitpy/port/xvfbdriver.py	2015-11-18 09:05:28 UTC (rev 192568)
@@ -34,7 +34,6 @@
 
 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,52 +49,56 @@
             _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 _xvfb_pipe(self):
+        return os.pipe()
 
-    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_read_display_id(self, read_fd):
+        import errno
+        import select
 
+        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
+
+            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 = []
+
+        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
+        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
+
+        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()
 
-        # 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
-
         server_name = self._port.driver_name()
         environment = self._port.setup_environ_for_server(server_name)
+        display_id = self._xvfb_run(environment)
 
-        run_xvfb = ["Xvfb", ":%d" % display_id, "-screen",  "0", "1024x768x%s" % self._xvfb_screen_depth(), "-nolisten", "tcp"]
-
-        if self._port._should_use_jhbuild():
-                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)
-
-        # 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)
-
         # We must do this here because the DISPLAY number depends on _worker_number
         environment['DISPLAY'] = ":%d" % display_id
         self._driver_tempdir = self._port.host.filesystem.mkdtemp(prefix='%s-' % self._port.driver_name())
@@ -115,11 +118,6 @@
 
     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)

Modified: trunk/Tools/Scripts/webkitpy/port/xvfbdriver_unittest.py (192567 => 192568)


--- trunk/Tools/Scripts/webkitpy/port/xvfbdriver_unittest.py	2015-11-18 07:52:47 UTC (rev 192567)
+++ trunk/Tools/Scripts/webkitpy/port/xvfbdriver_unittest.py	2015-11-18 09:05:28 UTC (rev 192568)
@@ -52,6 +52,8 @@
         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 @@
         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 @@
             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))
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to