Bug 29943 - Make TpDebugSender's g_log handler thread safe
Summary: Make TpDebugSender's g_log handler thread safe
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium major
Assignee: Simon McVittie
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/sm...
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2010-09-01 10:42 UTC by Olivier Crête
Modified: 2010-09-10 05:03 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Give the correct type to the debug_sender global variable. (1.29 KB, patch)
2010-09-01 10:42 UTC, Olivier Crête
Details | Splinter Review
Make TpDebugSender's g_log handler thread safe (2.50 KB, patch)
2010-09-01 10:42 UTC, Olivier Crête
Details | Splinter Review
updated patch (2.52 KB, patch)
2010-09-01 10:59 UTC, Olivier Crête
Details | Splinter Review
updated again (2.52 KB, patch)
2010-09-01 11:05 UTC, Olivier Crête
Details | Splinter Review

Description Olivier Crête 2010-09-01 10:42:00 UTC
It currently isn't thread safe. So if g_debug or g_warning or g_something_else is called from another thread, it is very likely to crash!
Comment 1 Olivier Crête 2010-09-01 10:42:30 UTC
Created attachment 38362 [details] [review]
Give the correct type to the debug_sender global variable.
Comment 2 Olivier Crête 2010-09-01 10:42:44 UTC
Created attachment 38363 [details] [review]
Make TpDebugSender's g_log handler thread safe
Comment 3 Simon McVittie 2010-09-01 10:51:46 UTC
Review of attachment 38362 [details] [review]:

Sadly, yes there is a reason.

::: telepathy-glib/debug-sender.c
@@ +207,3 @@
           type, n_construct_params, construct_params);
+      debug_sender = TP_DEBUG_SENDER (retval);
+      g_object_add_weak_pointer (retval, (gpointer *) &debug_sender);

This violates ISO C99 aliasing rules, which is why it was a gpointer in the first place.

(void ** and TpDebugSender ** are not "similar enough" to be considered aliasable - void * is special but void ** is not.)
Comment 4 Simon McVittie 2010-09-01 10:55:14 UTC
Review of attachment 38363 [details] [review]:

::: telepathy-glib/debug-sender.c
@@ +401,3 @@
+    goto ignore_message;
+
+  if (g_queue_get_length (debug_sender->priv->messages) >= DEBUG_MESSAGE_LIMIT)

This stuff needs factoring out of tp_debug_sender_add_message() into a common helper, instead of being duplicated.

@@ +494,3 @@
+  /* This could be called from any thread, but since Telepathy-GLib is not
+   * in any way thread safe, we must send it to the main loop to be
+   * processedx

"processed"

(FWIW, it's mostly dbus-glib that's not thread-safe)
Comment 5 Olivier Crête 2010-09-01 10:59:17 UTC
Created attachment 38364 [details] [review]
updated patch

Arg, I hate strict aliasing rules, what a waste of time.
Comment 6 Olivier Crête 2010-09-01 11:05:57 UTC
Created attachment 38365 [details] [review]
updated again

Split the function into two parts..

Also, in this case, its not only dbus-glib that was crashing, but playing around with the GQueue in TpDebugSender ;)
Comment 7 Simon McVittie 2010-09-02 03:31:31 UTC
Review of attachment 38365 [details] [review]:

Either the patch review stuff is lying to me, or this is just your original patch back again?
Comment 8 Simon McVittie 2010-09-10 04:32:50 UTC
Stealing this bug because we want this before telepathy-glib 0.12.0. How's this?

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/thread-safety
Comment 9 Simon McVittie 2010-09-10 05:03:26 UTC
Fixed in git for 0.11.15


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.