Created attachment 31345 [details] [review]
fix

If this is as serious as it sounds it defitely sounds like it should go in. Mark?

(Adding Mark to the cc list)

I've never seen any problems with this, I'd say we could delay it to 2.8.1.

Created attachment 31395 [details]
demonstrates the bug

I suggest you examine this demonstration, which illustrates a simple scheme
where a program updates its splash screen from a thread.

Created attachment 31396 [details]
backtrace

When running against an unpatched libgnomeui it crashes about 25% of the time,
always with a trace similar to this one.

Fortunately no mainstream GNOME program seem to be triggering this bug at the
moment, but it is nevertheless serious. Please commit the trivial fix in GNOME 2.8.

Created attachment 31397 [details] [review]
other fixes

While I was on this I thought to audit libgnomeui for other bugs of the same
class, and I found three of these, fixed by this patch.

I suggest you perform this audit for all the GNOME libraries being at or above
the GDK layer.

Punting to 2.8.x

Comment on attachment 31345 [details] [review]
fix

It would seem more correct to me to only take the GDK lock when we need to.

So, the GDK_THREADS_ENTER()/LEAVE() pair would seem to make more sense to me in
client_save_yourself_callback()

Hi,

I can reproduce the problem using the submitted demonstration program. I also
get similar backtraces when using mail-notification (that's why I tested it).

I found one prerequisite for this bug: It only occurs when having Acessibility
enabled. ("Applications/Desktop Preferences/Accessibility/Assistive Technology
Support/Enable assistive technologies").

I hope that helps to determine the problem. I suggest to set this bug to NEW.

Best regards,
Christian

Ping

Marking down since this doesn't seem to manifest itself much with regular use. 

What about just applying the f*****g patches, now?

Please, read mark comments and try to post a new patch. Thanks

The g_signal_emit (context, disconnect_id, 0) call should also be protected.
Moreover, I am not sure that client_save_yourself_callback() can only be called
 from process_ice_messages().

Created attachment 38976 [details] [review]
updated fixes

Updated patch for libgnomeui 2.10.0, now also protects GConf callbacks.

the patch seems to be pretty dubious to me. if you're going to try and make
gnome thread-safe, then you should start at the bottom, not add
gdk_threads_enter/leave around every call. that's just rediculous and totally
the wrong approach to solving your problem.

You don't understand the patch.

Well, could you please enlighten people as to what you're trying to achieve
instead of just adding locking pairs and pointing to a crash in a test program
that is almost impossible to trigger? I can't trigger the crash at all here.
Also, telling other developers "You don't understand the patch" without offering
to explain what they supposedly don't understand isn't the best way of getting
attention to your problem.

And btw, from the impression I've gotten after talking to people about this it
seems that this patch for README in libgnomeui is just as pertinent:

diff -u -p -r1.2 README
--- README      18 May 2003 22:10:59 -0000      1.2
+++ README      1 Jul 2005 18:33:09 -0000
@@ -1 +1,4 @@
 This is the gui parts of what was previously gnome-libs
+
+This library is not thread safe.
+

GDK is not thread-safe. In multi-threaded applications, the GDK global lock must
be held while using GDK/GTK+ interfaces. Such applications achieve this by
wrapping the GTK+ main loop (gtk_main()) into a GDK_THREADS_ENTER/LEAVE pair.
However, idles, timeouts and input functions are executed with the GDK lock
_not_  held. This is the reason why, when calling GDK/GTK+ from
idle/timeout/input callbacks, you _must_ acquire the GDK global lock (see
http://developer.gnome.org/doc/API/2.0/gdk/gdk-Threads.html). If you look at the
libgnomeui code, you will see that this is already done in some places. My patch
simply holds the GDK global lock where doing so has been forgotten by the
libgnomeui developers.

Most of the added enter-leave pairs make sense to me, though I would probably 
convert multiple returns into a goto done, rather than adding multiple leaves.

I have to agree with markmc though, that you can not simply wrap the whole
body of process_ice_messages in an enter-leave pair. That changes the api for
all uses of the disconnect signal. Previously it was emitted without holding the
gdk lock, now it is emitted while holding the lock.

Thank you so much for your input Matthias. Maybe we could massage the patch into
submittable form then so we can get this one resolved finally.

So these are the only issues left to solve:

- changing the multiple returns to use goto's
- move the locking pair from process_ice_messages() into
client_save_yourself_callback()

right?

Created attachment 48527 [details] [review]
use gotos, and do not lock disconnect emissions

"Previously it was emitted without holding the gdk lock" -> you forget
gnome_client_disconnect(). Anyway, I have updated the patch.

[Removing the meaninglessly old milestone. If people think this should be a
showstopper for 2.12, feel free to add that.]

<sigh> Can someone review and/or commit this thing? please? :) [Probably on a
2.13 branch? :/

*** Bug 301754 has been marked as a duplicate of this bug. ***

(In reply to comment #26)
> <sigh> Can someone review and/or commit this thing? please? :) [Probably on a
> 2.13 branch? :/

I have the patches "fix" and "other fixes" applied since gnome-2.8 and I've never seen the problems anymore. Sorry, that I can't say anything about the current patch, but the old ones fixed the problem anyway.

I'm going to commit this to HEAD now. The gtkfilesystemgnomevfs.c patch is rejected now, has this been fixed in a different manner there?

Commited except for the bits in gtkfilesystemgnomevfs.c. Please file a new bug for that if it's still relevant.