Bug 151135 - [XvfbDriver] Fail to run all layout tests when X server started with -displayfd option
Summary: [XvfbDriver] Fail to run all layout tests when X server started with -display...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2015-11-11 06:56 PST by Carlos Garcia Campos
Modified: 2015-11-18 03:03 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.66 KB, patch)
2015-11-11 07:03 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Fix coding style (6.64 KB, patch)
2015-11-12 03:23 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (13.41 KB, patch)
2015-11-16 02:20 PST, Carlos Garcia Campos
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2015-11-11 06:56:29 PST
Since my last debian update my xserver no longer uses the :<display-id> option, but -displayfd. The XvfbDriver uses the x server command line to check the displayas that are currently in use. This doesn't work when x server was satarted with -displayfd. This option is used to let the server find the display id available and it's written to the given file descriptor. With this option xord 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 when available. That would simplify the code, fixing also race conditions between we check for available displays and XVfb opens 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.
Comment 1 Carlos Garcia Campos 2015-11-11 06:57:34 PST
cgarcia   1245  1.6  2.5 1118548 416480 tty2   Sl+  08:08   7:44 /usr/lib/xorg/Xorg vt2 -displayfd 3 -auth /run/user/1001/gdm/Xauthority -nolisten tcp -background none -noreset -keeptty -verbose 3
carlos   29750  0.0  0.2 479492 48224 tty3     Sl+  11:27   0:02 /usr/lib/xorg/Xorg vt3 -displayfd 3 -auth /run/user/1000/gdm/Xauthority -nolisten tcp -background none -noreset -keeptty -verbose 3

this is how Xorg is run in my laptop
Comment 2 Carlos Garcia Campos 2015-11-11 07:03:01 PST
Created attachment 265284 [details]
Patch
Comment 3 WebKit Commit Bot 2015-11-11 07:04:54 PST
Attachment 265284 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/port/xvfbdriver.py:82:  multiple imports on one line  [pep8/E401] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Carlos Garcia Campos 2015-11-12 03:23:45 PST
Created attachment 265377 [details]
Fix coding style
Comment 5 Carlos Garcia Campos 2015-11-13 05:45:46 PST
ping reviewers, it's a bit annoying having to keep this patch around locally to be able to run the tests.
Comment 6 Zan Dobersek 2015-11-14 11:38:44 PST
Is running Xvfb via displayfd required or is that now supported because it can be run that way?
Comment 7 Carlos Garcia Campos 2015-11-15 23:39:10 PST
(In reply to comment #6)
> Is running Xvfb via displayfd required or is that now supported because it
> can be run that way?

It's not required, but there's no easy way to get a list of display ids currently used when xorg itself is run with displayfd. However, using displayfd we don't need that list at all and it fixes other issues of the current model.
Comment 8 Zan Dobersek 2015-11-15 23:53:39 PST
Would it be possible to just switch to only using displayfd? How recent is this flag?

Having two ways to run this isn't optimal. BUt if you add some test cases in xvfbdriver_unittest.py I guess it's not that big of a deal either.
Comment 9 Carlos Garcia Campos 2015-11-15 23:57:07 PST
(In reply to comment #8)
> Would it be possible to just switch to only using displayfd? How recent is
> this flag?
> 
> Having two ways to run this isn't optimal. BUt if you add some test cases in
> xvfbdriver_unittest.py I guess it's not that big of a deal either.

I agree, and I thought about it, but since I was not sure (and didn't have much time) I took the safest approach. I'll check when displayfd was added.
Comment 10 Zan Dobersek 2015-11-15 23:58:51 PST
Would it be possible to just switch to only using displayfd? How recent is this flag?

Having two ways to run this isn't optimal, but please at least add some test cases in xvfbdriver_unittest.py.
Comment 11 Carlos Garcia Campos 2015-11-16 02:20:55 PST
Created attachment 265579 [details]
Updated patch

The displayfd option is available in Xvfb since 3 years ago, so I've removed the old code and updated the unit tests.
Comment 12 Darin Adler 2015-11-17 08:18:06 PST
Comment on attachment 265579 [details]
Updated patch

rs=me
Comment 13 Carlos Garcia Campos 2015-11-18 01:05:26 PST
(In reply to comment #12)
> Comment on attachment 265579 [details]
> Updated patch
> 
> rs=me

Thanks Darin.
Comment 14 Carlos Garcia Campos 2015-11-18 01:05:40 PST
Committed r192568: <http://trac.webkit.org/changeset/192568>
Comment 15 Carlos Garcia Campos 2015-11-18 01:28:48 PST
(In reply to comment #11)
> Created attachment 265579 [details]
> Updated patch
> 
> The displayfd option is available in Xvfb since 3 years ago, so I've removed
> the old code and updated the unit tests.

I had forgotten that we build our own xserver, so we are not using Xvfb from the distro but from the jhbuild env. And the version we build 1.12.0 (released in March 2012) doesn't have the displayfd option. This shouldn't be a problem for EFL since they don't build xserver. So, I think it's better to just update xserver to a newer version or stop building our own.
Comment 16 Csaba Osztrogonác 2015-11-18 02:34:10 PST
(In reply to comment #14)
> Committed r192568: <http://trac.webkit.org/changeset/192568>

I have no idea how and why, but it broke the 
webkitpy test script on the Apple Mac bots.
Comment 17 Carlos Garcia Campos 2015-11-18 02:37:00 PST
(In reply to comment #16)
> (In reply to comment #14)
> > Committed r192568: <http://trac.webkit.org/changeset/192568>
> 
> I have no idea how and why, but it broke the 
> webkitpy test script on the Apple Mac bots.

Oh, I'll check that too.
Comment 18 Carlos Garcia Campos 2015-11-18 02:42:40 PST
I can reproduce it locally, I'm investigating.
Comment 19 Carlos Garcia Campos 2015-11-18 03:03:46 PST
Python tests should be fixed in r192569.