[Spice-devel] [PATCH spice-gtk] spice-widget: Make sure we can use X11 from different thread

[Daniel P. Berrangé]

On Mon, Nov 26, 2018 at 09:37:31AM -0500, Frediano Ziglio wrote:
> > On Mon, Nov 26, 2018 at 06:27:48AM -0500, Frediano Ziglio wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > On 11/26/18 10:16 AM, Frediano Ziglio wrote:
> > > > > In order to support GStreamer overlay this is necessary as some
> > > > > plugins can use X11 from a different thread.
> > > > >
> > > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > > ---
> > > > >   src/spice-widget.c | 13 +++++++++++++
> > > > >   1 file changed, 13 insertions(+)
> > > > >
> > > > > CI result:
> > > > > https://gitlab.freedesktop.org/fziglio/spice-gtk/pipelines/9559
> > > > >
> > > > > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > > > > index 312c640a..cca0867e 100644
> > > > > --- a/src/spice-widget.c
> > > > > +++ b/src/spice-widget.c
> > > > > @@ -2547,6 +2547,19 @@ static void queue_draw_area(SpiceDisplay
> > > > > *display,
> > > > > gint x, gint y,
> > > > >                                  x, y, width, height);
> > > > >   }
> > > > >   
> > > > > +#ifdef GDK_WINDOWING_X11
> > > > > +/* See
> > > > > https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/GstGLDisplay.html
> > > > > + * (look for XInitThreads). We call it into a constructor to make sure
> > > > > + * we call before any X11 function.
> > > > > + * In case of GStreamer some plugins will use X11 from different
> > > > > + * threads.
> > > > > + */
> > > > > +SPICE_CONSTRUCTOR_FUNC(x11_threads_init)
> > > > > +{
> > > > > +    XInitThreads();
> > > > > +}
> > > > > +#endif
> > 
> > Can this be done at all from a shared library, or should this be the
> > responsibility of the library user? For example, if spice-gtk is
> > dlopened for some reason, XInitThreads will be called too late.
> > #ifdef GDK_WINDOWING_X11 is not enough, you also need some runtime check
> > as you don't want to call this when running "GDK_BACKEND=wayland spicy",
> > but you want to call this with "GDK_BACKEND=x11 spicy".
> > 
> 
> In this case also the application should do it, but does not harm
> to do also in the shared object which likely is going to be loaded
> at program start as this is usually the way is done.

If the application already does call XInitThreads, then there is no
benefit to calling it in spice-gtk constructors, as threading is already
initialized.

If the application does *not* call XInitThreads, then calling it in
spice-gtk will cause possible crashes or deadlocks or memory corruption
when dlopen'd.

Per the man page the XInitThreads call *must* be the very X11 call
made by an application, before *ANY* other X11 call.

Consider if you have a thread A which is in the middle of an X11
call. Now thread B triggers dlopen of spice-gtk which calls XInitThreads.
When thread A finishes its X11 call it will trigger an XUnlockThreads()
call, despite there being no original XLockThreads call. This is very
bad as at the very least you will corrupt the mutex state by having too
many unlock calls.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|