1272485 - Link list races with GDK X11 error handler and off main thread Xlib

Status: NEW

(Core :: Graphics, defect, P3)

Description

[Karl Tomlinson (:karlt)]

_gdk_x11_display_error_event walks a linked list on the thread that the
error handler is called.
https://git.gnome.org/browse/gtk+/tree/gdk/x11/gdkdisplay-x11.c?h=3.20.4#n2531

I don't think anything prevents that list being manipulated on the main thread
at the same time.

Using GDKs Display on another thread for a method waiting in _XReply()
is a problem, but it goes beyond that.
When _XGetRequest() finds the send queue full, it calls _XFlush() which calls
_XEventsQueued(dpy, QueuedAfterReading).  That means that any request, not
just those that wait, may trigger processing of error events.

We have the ability to hook the XSetErrorHandler dynamic lookup, but I don't
know that that helps.  GDK calls delete_outdated_error_traps() outside of the
calls to XSetErrorHandler() during push and pop.

Using async_handlers on the Display to simply intercept and ignore the error
would mean that GDK wouldn't find out whether the request succeeded.
I haven't looked into what problems that may cause.

I wonder how much synchronization would be required to use a separate display
on the compositor thread.

Comment 1

[Andrew Comminos [:acomminos]]

Attachment: 0001-atomicxerrorhandler.patch

Here's a draft implementation of an idea I had to solve this by blocking the X11 display during error handling. Ideally, we would use a separate display on the compositor thread and multiplex error events to either GDK (on the main thread's X display) or to our own scoped error handlers through this class.

Comment 2

[Karl Tomlinson (:karlt)]

If separate display's are used on each thread, then these races with GDK's linked lists are avoided.  Using async_handlers as in [1] will avoid the problem with races on XSetErrorHandler callers.

When GDK's display is used on another thread, IIUC the approach in attachment 8765078 [details] [diff] [review] would only avoid races on GDK's linked lists if GDK could be persuaded to use AtomicXErrorHandler::PushTrap() when sending a request that might generate an error.

GDK calls XSetErrorHandler() on pop too early to know that any error has already been received.  However, it will have called XSync() if it needs the error code, and so perhaps that XSetErrorHandler() could still be intercepted to add an async_handler to ignore errors up to Display::request.

[1] http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/media/webrtc/trunk/webrtc/modules/desktop_capture/x11/x_error_trap.cc#39

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #2)
> http://searchfox.org/mozilla-central/rev/
> 970569ad57ac4436ff31aa2ac63f38ed3ee2932d/media/webrtc/trunk/webrtc/modules/
> desktop_capture/x11/x_error_trap.cc#39

I suspect this approach could be extended to support the same display on multiple threads by using XLockDisplay on install and XUnlockDisplay on uninstall.

That's a solution to XSetErrorHandler races only, not to the GDK linked list races tracked here.