Title: [195032] releases/WebKitGTK/webkit-2.10/Tools
Revision
195032
Author
carlo...@webkit.org
Date
2016-01-14 04:01:17 -0800 (Thu, 14 Jan 2016)

Log Message

Merge r192568 - [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: releases/WebKitGTK/webkit-2.10/Tools/ChangeLog (195031 => 195032)


--- releases/WebKitGTK/webkit-2.10/Tools/ChangeLog	2016-01-14 11:58:39 UTC (rev 195031)
+++ releases/WebKitGTK/webkit-2.10/Tools/ChangeLog	2016-01-14 12:01:17 UTC (rev 195032)
@@ -1,3 +1,54 @@
+2015-11-18  Carlos Garcia Campos  <cgar...@igalia.com>
+
+        Unreviewed. Fix webkitpy tests after r192568.
+
+        Instead of ignoring exception when closing mock fds, use a helper
+        function that the unit test can override to do nothing.
+
+        * Scripts/webkitpy/port/xvfbdriver.py:
+        (XvfbDriver._xvfb_close_pipe):
+        (XvfbDriver._xvfb_run):
+        * Scripts/webkitpy/port/xvfbdriver_unittest.py:
+        (XvfbDriverTest.make_driver):
+
+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-10-23  Michael Saboff  <msab...@apple.com>
 
         REGRESSION (r179357-r179359): WebContent Crash using AOL Mail @ com.apple._javascript_Core JSC::linkPolymorphicCall(JSC::ExecState*, JSC::CallLinkInfo&, JSC::CallVariant, JSC::RegisterPreservationMode) + 1584

Modified: releases/WebKitGTK/webkit-2.10/Tools/Scripts/webkitpy/port/xvfbdriver.py (195031 => 195032)


--- releases/WebKitGTK/webkit-2.10/Tools/Scripts/webkitpy/port/xvfbdriver.py	2016-01-14 11:58:39 UTC (rev 195031)
+++ releases/WebKitGTK/webkit-2.10/Tools/Scripts/webkitpy/port/xvfbdriver.py	2016-01-14 12:01:17 UTC (rev 195032)
@@ -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
 
-    def _xvfb_screen_depth(self):
-        return os.environ.get('XVFB_SCREEN_DEPTH', '24')
+        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
 
-    def _start(self, pixel_tests, per_test_args):
-        self.stop()
+            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 = []
 
-        # 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
+        return int(display_id)
 
-        server_name = self._port.driver_name()
-        environment = self._port.setup_environ_for_server(server_name)
+    def _xvfb_close_pipe(self, pipe_fds):
+        os.close(pipe_fds[0])
+        os.close(pipe_fds[1])
 
-        run_xvfb = ["Xvfb", ":%d" % display_id, "-screen",  "0", "1024x768x%s" % self._xvfb_screen_depth(), "-nolisten", "tcp"]
-
+    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)
 
-        # 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)
+        self._xvfb_close_pipe((read_fd, write_fd))
 
+        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
         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: releases/WebKitGTK/webkit-2.10/Tools/Scripts/webkitpy/port/xvfbdriver_unittest.py (195031 => 195032)


--- releases/WebKitGTK/webkit-2.10/Tools/Scripts/webkitpy/port/xvfbdriver_unittest.py	2016-01-14 11:58:39 UTC (rev 195031)
+++ releases/WebKitGTK/webkit-2.10/Tools/Scripts/webkitpy/port/xvfbdriver_unittest.py	2016-01-14 12:01:17 UTC (rev 195032)
@@ -52,6 +52,9 @@
         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._xvfb_close_pipe = lambda p: None
         driver._environment = port.setup_environ_for_server(port.driver_name())
         return driver
 
@@ -66,71 +69,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 +90,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