1287463 - Use a separate X display on the compositor thread for basic composition buffering paths

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Andrew Comminos [:acomminos]]

We need this for XCB in nsShmImage to be useful; if a round trip is made to the X server using the same display on the main thread, we won't gracefully check the error.

Comment 1

[Andrew Comminos [:acomminos]]

Attachment: Bug 1287463 - Use a separate X display for non-XRender basic composition paths.

Review commit: https://reviewboard.mozilla.org/r/64956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64956/

Comment 2

[Andrew Comminos [:acomminos]]

This switches to using our compositor display for all non-XRender draw paths (using a separate display with XRender would likely require additional X synchronization).

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=732b79f1e57f

Comment 3

[Lee Salzman [:lsalzman]]

Comment on attachment 8772017 [details]
Bug 1287463 - Use a separate X display for non-XRender basic composition paths.

https://reviewboard.mozilla.org/r/64956/#review61974

::: widget/nsShmImage.cpp:43
(Diff revision 1)
>  {
>    mConnection = XGetXCBConnection(aDisplay);
>    mozilla::PodZero(&mLastRequest);
> +  if (aDisplay == mozilla::DefaultXDisplay()) {
> +    // If another thread spins the X event loop during a checked call,
> +    // an error that should've been checked by XCB may by handled by the Xlib

Typo: "may by handled" -> "may be handled"

Comment 4

[Andrew Comminos [:acomminos]]

Comment on attachment 8772017 [details]
Bug 1287463 - Use a separate X display for non-XRender basic composition paths.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64956/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a69345f4f4e
Use a separate X display for non-XRender basic composition paths. r=lsalzman

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a69345f4f4e

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Andrew Comminos [:acomminos] from comment #0)
> We need this for XCB in nsShmImage to be useful; if a round trip is made to
> the X server using the same display on the main thread, we won't gracefully
> check the error.

Are you sure?  I would expect xcb to filter out the error into the cookie while handling the connection before Xlib on the main thread gets a chance to see it.

Comment 8

[Andrew Comminos [:acomminos]]

(In reply to Karl Tomlinson (:karlt) from comment #7)
> (In reply to Andrew Comminos [:acomminos] from comment #0)
> > We need this for XCB in nsShmImage to be useful; if a round trip is made to
> > the X server using the same display on the main thread, we won't gracefully
> > check the error.
> 
> Are you sure?  I would expect xcb to filter out the error into the cookie
> while handling the connection before Xlib on the main thread gets a chance
> to see it.

You're absolutely correct; I was mislead by some X_ShmAttach errors being caught in XCB-based builds on crash-stats.mozilla.org. The following test confirms it (at least on my machine);

> #include <X11/Xlib-xcb.h>
> #include <assert.h>
> 
> static int xlib_error = 0;
> int x_error_trap(Display* display, XErrorEvent* e) {
>   xlib_error = 1;
> }
> 
> void main() {
>   XSetErrorHandler(x_error_trap);
> 
>   Display* display = XOpenDisplay(NULL);
>   xcb_connection_t* conn = XGetXCBConnection(display);
>   assert(xlib_error == 0);
> 
>   // Make illegal request via XCB
>   xcb_get_geometry_cookie_t cookie = xcb_get_geometry(conn, -1);
> 
>   // Test to ensure that XCB caught our error despite Xlib processing events
>   XSync(display, False);
>   xcb_generic_error_t* error;
>   xcb_get_geometry_reply(conn, cookie, &error);
>   assert(error != NULL);
>   assert(xlib_error == 0);
> 
>   // Ensure that our request should have generated an error in the first place
>   XGetGeometry(display, -1, 0, 0, 0, 0, 0, 0, 0);
>   XSync(display, False);
>   assert(xlib_error == 1);
> }

We might want to switch back to a single X display then for bug 1287666. We do get interesting performance improvements from the use of a separate X display, at the expense of having SHM images swap slower;

https://treeherder.mozilla.org/perf.html#/alerts?id=1922

(fwiw, these tests were unaffected by the XCB switch- their e10s variants were, however.)

Comment 9

[Kartikaya Gupta (email:[e-mail])]

I backed this out on beta (Firefox 50) for causing bug 1304637. It may get backed out on 51 as well if we don't get around to fixing the regression in time.