Xpra does not hold the gtk global mutex while calling into gtk

[totaam]

Issue migrated from trac ticket # 420

component: platforms | priority: major | resolution: fixed | keywords: freebsd threads

2013-08-22 03:44:20: thefloweringash created the issue

---

Xpra 0.8.8 on FreeBSD crashes on startup with an error from pthread_mutex_unlock (ports/177800).

The gtk documentation on threads says:

GTK+ is "thread aware" but not thread safe — it provides a global lock controlled by gdk_threads_enter()/gdk_threads_leave() which protects all use of GTK+. That is, only one thread can use GTK+ at any given time.

However I can't find any use of gdk_threads_enter() or gdk_threads_leave() within Xpra's source.

This crash on FreeBSD is caused by gtk_main() calling GDK_THREADS_LEAVE() to unlock the global mutex, with the assumption that it is currently locked by the caller. When attempting to unlock an unlocked mutex (or a mutex held by another thread), FreeBSD returns EPERM, which gtk quickly promotes to an abort() (in g_mutex_unlock(), g_thread_abort()).

Xpra works on Linux since Linux (apparently?) returns 0 in the case of unlocking an unlocked mutex, and as far as I'm aware, POSIX leaves this as undefined behavior (http://stackoverflow.com/a/1778821).

Applying the obvious patch (https://gist.github.com/thefloweringash/6302597) allows Xpra to start and accept an xterm, but I haven't yet been able to attach due to a socket.error (Socket is not connected), which I'm assuming is unrelated.

[totaam]

2013-08-22 03:50:39: totaam commented

---

You do not need gdk_threads_enter() / gdk_threads_leave() if your application only accesses GTK from the UI thread.
This is what xpra does unless proven otherwise.
You never need to add those calls around gtk.main, for sure.

[[BR]]

Looks to me like something is broken on your build (GTK? Python?).
Please try the latest releases, 0.8.x is not supported, and even 0.9.x is unlikely to get another update, so you should be testing with 0.10.1 or later (trunk).

[totaam]

2013-08-22 04:02:04: thefloweringash commented

---

The gtk examples in the threads page do wrap gtk_main() with gtk_threads_enter()/gtk_threads_leave(), and though I can't quickly find anything definitive on the subject, this post turned up by a quick google seems to agree with my analysis.

http://blogs.operationaldynamics.com/andrew/software/gnome-desktop/gtk-thread-awareness

I tested with 0.8.8 since I defaulted to the version in ports. I'll test 0.10.1.

[totaam]

2013-08-22 04:37:07: totaam commented

---

For more information on how xpra uses the UI thread, see:

• What are the general tips for using threads with PyGTK?: A way to avoid using threads_enter/threads_leave or similar context manager is to call the function from the main thread. A simple way of sending a request to the server is to use gobject.idle_add
  Note the absence of enter/leave anywhere.
• I am using a separate thread to run my code, but the application (or the UI) hangs:

• "1. Allow only the main thread to touch the GUI (gtk) part, while letting other threads do background work" - is what we do
• "2. Allow any thread to do GUI stuff. Warning: people doing win32 pygtk programming have said that having non-main threads doing GUI stuff in win32 doesn't work. So this programming style is really not recommended." - we cannot use this approach because it would break win32 (and probably others too)

Now it is possible that there is some code somewhere that improperly calls some gtk ui code from a non-main thread, if that is the case we really want to hunt it down and fix it, you should be able to get a backtrace for us using gdb.

What can be misleading on threading with (py)gtk is posts like this one: Multi-threaded GTK applications – Part 1: Misconceptions which state: It turns out that you’re supposed to put gdk_threads_enter()/leave() around the call you make to gtk_main(). - that is only true if you intend to use threading (multiple threads modifying the UI) - which is not our case (for the reasons explained above).

Please also note: no matter if you are doing PyGTK calls from a separate thread or not, you must compile PyGTK with --enable-threads - I assume this is your case (I would have expected the enter/leave calls to error out if it was not the case)

[totaam]

2013-08-22 05:28:25: thefloweringash commented

---

gdb session of xpra 0.10.1 https://gist.github.com/thefloweringash/6303230

EDIT: a gdb session for the following simplified version https://gist.github.com/thefloweringash/6303264

#!/usr/local/bin/python2.7
import gtk.gdk
gtk.gdk.threads_init()
gtk.main()

[totaam]

2013-08-22 05:49:00: totaam changed status from new to closed

[totaam]

2013-08-22 05:49:00: totaam changed resolution from ** to invalid

[totaam]

2013-08-22 05:49:00: totaam commented

---

Then the problem is with FreeBSD, this code is valid and can be found in many applications, including the textbook examples from the pygtk FAQ linked above.

[totaam]

2013-08-23 06:29:08: thefloweringash commented

---

Short version

FreeBSD compatibility for 0.10.1 is a very small patch: https://gist.github.com/thefloweringash/6315767. Since 0.8.8, most of the gtk.gdk.threads_init() calls have been changed to the more precise glib.threads_init() (formerly gobject.threads_init()). After applying this change to server.py I can successfully interact with an xterm over xpra.

Long version

After investigating further I have to disagree. The PyGTK samples run without aborting on FreeBSD, and I find none of the posts linked to be in error. I do not believe the code sample in #comment:4 is valid. At the risk of being overly verbose, I hope a somewhat thorough exploration might still be generally useful, since there seems to be some disagreement on this topic.

I propose the following condition:

• You must wrap gdk_threads_enter() and gdk_threads_leave() around gtk_main() if you have previously called gdk_threads_init().

or for the sake of avoiding ambiguity, in pygtk terms

• You must wrap gtk.gdk.threads_enter() and gtk.gdk.threads_leave() around gtk.main() if you have previously called gtk.gdk.threads_init().

It is important to note that glib.threads_init() (source) (docs) corresponds to g_thread_init(), and does not call gdk_threads_init(). That is, it is enabling glib thread safety, and not engaging the gtk global mutex.

In the PyGTK FAQ 20.6. The approach labelled 1 satisfies the condition as it does not call gtk.gdk.threads_init(). The approach labelled 2, as demoed on the mailing list also satisfies the condition the as it both calls gtk.gdk.threads_init() and wraps gtk.main() with gtk.gdk.threads_enter() and gtk.gdk.threads_leave().

The "Misconceptions" post explains that after gdk_threads_init(), the call to gtk_main() will unlock the global mutex, which will already be unlocked without gdk_threads_enter(). In my opinion unlocking an unlocked mutex is a bug, and on glib on FreeBSD it's an abort(). The comments for g_mutex_unlock() make it quite explicitly undefined behavior.

[totaam]

2013-08-23 10:25:24: antoine commented

---

Thanks for the details and the patches.

The straightforward build fixes are merged in r4216 and I will apply them to the 0.10.x branch before the next point release.

The threading patch was merged in r4217, I am hoping this one will get more testing on a variety of platforms (especially win32 which has had problems in the past) before I apply to 0.10.x - this sort of change makes me nervous.

(I have briefly read the links and it looks like you are right about enter/leave being needed once we call gtk.gdk.threads_init())

[totaam]

2013-11-24 12:07:48: totaam commented

---

r4790 is required to prevent a server crash on OpenBSD (and probably FreeBSD and others too) - will backport to v0.10.x.

[totaam]

2013-11-25 11:12:37: totaam commented

---

• r4794 does the same thing for the gtk2 client (now also works on FreeBSD - although there are unrelated libav problems there...)
• r4796 adds the same code to the launcher and does some refactoring

[[BR]]

All backported to v0.10.x in 4799

[totaam]

2013-11-26 12:22:09: totaam changed status from closed to reopened

[totaam]

2013-11-26 12:22:09: totaam changed resolution from invalid to **

[totaam]

2013-11-26 12:22:09: totaam commented

---

Re-opening so I can change the resolution: this bug was not invalid.

[totaam]

2013-11-26 12:22:18: totaam changed status from reopened to closed

[totaam]

2013-11-26 12:22:18: totaam changed resolution from ** to fixed

[totaam]

2014-01-04 12:12:00: totaam commented

---

Damn: this caused a regression, see #485

[totaam]

2014-01-15 15:50:34: totaam commented

---

And yet another one: #497

So. Much. Pain.