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

RESOLVED FIXED in Firefox 51

Status

()

Core
Widget: Gtk
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: acomminos, Assigned: acomminos)

Tracking

50 Branch
mozilla50
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 wontfix, firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8772017 [details]
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/
Attachment #8772017 - Flags: review?(lsalzman)
(Assignee)

Comment 2

a year ago
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 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"
Attachment #8772017 - Flags: review?(lsalzman) → review+
(Assignee)

Comment 4

a year ago
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

a year ago
Pushed by acomminos@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a69345f4f4e
Use a separate X display for non-XRender basic composition paths. r=lsalzman

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2a69345f4f4e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(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.
(Assignee)

Comment 8

a year ago
(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.)
Depends on: 1289233

Updated

a year ago
Depends on: 1304637
Depends on: 1307362
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.
status-firefox50: fixed → wontfix
status-firefox51: --- → fixed
You need to log in before you can comment on or make changes to this bug.