Bug 77335 – [Qt][NRWT] Run each DRT in it's own xvfb

• Component: WebKit Qt
• Status: RESOLVED INVALID

[János Badics]

running xvfb-run ...../DumpRenderTree [testname] gives the right output;
but putting xvfb-run in cmd_line(self) causes tests to timeout and give no results.

[Kristóf Kosztyó]

Hi,
I looked after it and I have found this problem is caused by the xvfb-run because it redirects the stderr to stdout.
You can try to modify the xvfb-run and put into the webkit dir and call this modified script.

[Kristóf Kosztyó]

Created attachment 125312 [details]
proposed fix

Hi
I modified the xvfb-run script which now doesn't redirect the stderr to the stdout, so the nrwt get the stderr from the driver.
I also add a new --xvfb option to the nrwt.

[Csaba Osztrogonác]

We need it to be able run layout tests parallel with NRWT (https://bugs.webkit.org/show_bug.cgi?id=77730).

There are strange flakey crashes now in the editing tests,
because they try to use same clipboard at the same time.

There are several ways to solve this problem:
- run all DRT in separated XVFB (with separated clipboards) - best solution
- serialize editing tests - it works, but it makes testing slow

[Eric Seidel (no email)]

Comment on attachment 125312 [details]
proposed fix

The cr-linux EWS uses xvfb.  I wonder how adam set that up.

[Csaba Osztrogonác]

Our buildbots run in xvfb long time ago. The problem is that now the buildslave
run in xvfb, and its DRT instances run in one xvfb with its one clipboard, etc.

[Tony Chang]

GTK+ has the same problem and has code for this already: http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/gtk.py#L46

Maybe Qt and GTK can somehow refactor and share this code?

Chromium Linux doesn't have this problem because we use a mock clipboard in DRT.

[Adam Barth]

> The cr-linux EWS uses xvfb.  I wonder how adam set that up.

I created a chromium-xvfb port:

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/ports.py#L261

Why doesn't the buildbot just run-webkit-tests inside an xvfb?  This doesn't seem like a concern for run-webkit-tests itself.

[Dirk Pranke]

(In reply to comment #7)
> > The cr-linux EWS uses xvfb.  I wonder how adam set that up.
> 
> I created a chromium-xvfb port:
> 
> http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/ports.py#L261
> 
> Why doesn't the buildbot just run-webkit-tests inside an xvfb?  This doesn't seem like a concern for run-webkit-tests itself.

See comment #3 - we need one xvfb per worker, not one for all of NRWT.

[Adam Barth]

Is this a problem for all ports?  Do we need to solve this problem for everyone (not just buildbots)?

[Csaba Osztrogonác]

(In reply to comment #9)
> Is this a problem for all ports?  Do we need to solve this problem for everyone (not just buildbots)?

I don't know how other ports works or not with parallel testing. But it is a blocker to run parallel tests on Qt now. 

It isn't problem only for the buildbots. Developers can't run parallel tests because of this bug.

[Csaba Osztrogonác]

Kristóf, could you pick up this bug again?

[Kristóf Kosztyó]

Created attachment 138810 [details]
proposed fix

This patch uses the same way as the gtk to start each DRT in separate xvfb hence I moved the GtkDriver to the new XvfbDriver class and use this in the qt as well.

[Eric Seidel (no email)]

Odd that you'r eusing xvfb for isolation.  I guess it works.

Chromium has code for this, for running tests in cr-linux-ews.  But it's in a different place.  I believe we run the whole session in a single xvfb.

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/ports.py#L196

[WebKit Review Bot]

Comment on attachment 138810 [details]
proposed fix

Clearing flags on attachment: 138810

Committed r115240: <http://trac.webkit.org/changeset/115240>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

(In reply to comment #13)
> Odd that you'r eusing xvfb for isolation.  I guess it works.
> 
> Chromium has code for this, for running tests in cr-linux-ews.  But it's in a different place.  I believe we run the whole session in a single xvfb.

If that's true, we should change it. Running tests in parallel is blocked on the xvfb process if there's only one. I thought we changed to used multiple xvfbs ages ago, but I don't see the code now.

[Dirk Pranke]

(In reply to comment #16)
> (In reply to comment #13)
> > Odd that you'r eusing xvfb for isolation.  I guess it works.
> > 
> > Chromium has code for this, for running tests in cr-linux-ews.  But it's in a different place.  I believe we run the whole session in a single xvfb.
> 
> If that's true, we should change it. Running tests in parallel is blocked on the xvfb process if there's only one. I thought we changed to used multiple xvfbs ages ago, but I don't see the code now.

See the earlier comments on this bug ... the problems arise on the Qt and Gtk ports from using the shared clipboard; chromium's DRT uses a Mock clipboard and so doesn't have to worry about this.

[Eric Seidel (no email)]

Historically the approach has been to mock out things like clipboard at a system level in DRT.  But it's possible xvfb is a better solution for these linux ports.  It won't help them on non-linux platforms of course :)

[Ojan Vafai]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #13)
> > > Odd that you'r eusing xvfb for isolation.  I guess it works.
> > > 
> > > Chromium has code for this, for running tests in cr-linux-ews.  But it's in a different place.  I believe we run the whole session in a single xvfb.
> > 
> > If that's true, we should change it. Running tests in parallel is blocked on the xvfb process if there's only one. I thought we changed to used multiple xvfbs ages ago, but I don't see the code now.
> 
> See the earlier comments on this bug ... the problems arise on the Qt and Gtk ports from using the shared clipboard; chromium's DRT uses a Mock clipboard and so doesn't have to worry about this.

We don't need it for correctness, but chromium should use multiple xvfbs because the tests will run faster because they won't be gated on the single xvfb process.

[Dirk Pranke]

(In reply to comment #19)
> 
> We don't need it for correctness, but chromium should use multiple xvfbs because the tests will run faster because they won't be gated on the single xvfb process.

Maybe. I've not seen xvfb be a bottleneck, but then again I haven't really tested it lately. It could be that having multiple X servers will just increase contention on the CPUs ...

[Csaba Osztrogonác]

Reopen, because it was rolled out - http://trac.webkit.org/changeset/115286

Please check the bot history. :(

[Dirk Pranke]

Interesting, I didn't realize wait() would raise an OSError if the process was already dead.

You can either catch that, or use _xvfb_process.terminate() instead of executive.kill_process and/or use _xvfb_process.join() instead of wait(), since join() definitely won't throw an error.

[Tony Chang]

(In reply to comment #20)
> (In reply to comment #19)
> > 
> > We don't need it for correctness, but chromium should use multiple xvfbs because the tests will run faster because they won't be gated on the single xvfb process.
> 
> Maybe. I've not seen xvfb be a bottleneck, but then again I haven't really tested it lately. It could be that having multiple X servers will just increase contention on the CPUs ...

xvfb isn't a bottleneck since we don't paint to the screen.  I forget when we made that change, but it's been a while.

Chromium only wants a single Xvfb.

[Kristóf Kosztyó]

(In reply to comment #22)
> Interesting, I didn't realize wait() would raise an OSError if the process was already dead.
> 
> You can either catch that, or use _xvfb_process.terminate() instead of executive.kill_process and/or use _xvfb_process.join() instead of wait(), since join() definitely won't throw an error.

I've tried to reproduce that when the wait throw an OSError but it wasn't successful. Anyway I've modified the patch as you said.

[Kristóf Kosztyó]

Created attachment 140265 [details]
proposed fix

[Dirk Pranke]

Comment on attachment 140265 [details]
proposed fix

looks fine.

[Csaba Osztrogonác]

Comment on attachment 140265 [details]
proposed fix

Clearing flags on attachment: 140265

Committed r116134: <http://trac.webkit.org/changeset/116134>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because XVFBDriver was disabled on Qt temporarily: https://trac.webkit.org/changeset/120688

See https://bugs.webkit.org/show_bug.cgi?id=88414 for details.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.