GNOME Bugzilla – Bug 699909
Do not use toggle_refs in CamelObjectBag
Last modified: 2013-07-01 09:00:16 UTC
GLib-GObject:ERROR:gobject.c:2732:toggle_refs_notify: assertion failed: (tstack.n_toggle_refs == 1) Program received signal SIGABRT, Aborted. 0x0000003909235819 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 56 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig); (gdb) imap(dwmw2): Info: Disconnected for inactivity in=1874 out=3740 (gdb) bt
+ Trace 231913
(gdb) t a a bt
+ Trace 231914
Thread 9219 (Thread 0x7fff1d1e4700 (LWP 17423))
Thread 1 (Thread 0x7ffff7f86a40 (LWP 28597))
(gdb) p *(GObject *)0x1baa150 $4 = {g_type_instance = {g_class = 0x7fffa8002c80}, ref_count = 1, qdata = 0x7fff28003990} (gdb) p *(GObjectClass *)0x7fffa8002c80 $5 = {g_type_class = {g_type = 140736011971584}, construct_properties = 0x7fffb00ac060 = {0x127bc00, 0x1296030, 0xaf9350, 0xaf92e0, 0x7fffe0003ab0}, constructor = 0x390c614580 <g_object_constructor>, set_property = 0x0, get_property = 0x0, dispose = 0x7fffe90d4dc0 <eas_folder_dispose>, finalize = 0x7fffe90d4d60 <eas_folder_finalize>, dispatch_properties_changed = 0x390c613d60 <g_object_dispatch_properties_changed>, notify = 0x0, constructed = 0x7fffe90d4320 <eas_folder_constructed>, flags = 1, pdummy = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0}} Hm, mea culpa? Thread 9219 is in the middle of creating the folder in question, I presume. Is there a race condition here?
It looks like the folder creation (in particular the database operation for the summary creation) is taking longer than normal due — this box does have performance issues. And somehow, camel_folder_changed() is getting called before we're finished. And the folder_emit_changed_cb() is happening when we don't expect it? I don't quite understand the refcounting here, but I don't think the ActiveSync back end is doing anything special. It's fairly much the same as the EWS back end that I copied it from. (gdb) p *(CamelEasFolder *)0x1baa150 $6 = {parent = {parent = {parent = {parent = {g_type_instance = {g_class = 0x7fffa8002c80}, ref_count = 1, qdata = 0x7fff28003990}, priv = 0x1baa1d0}, priv = 0x1baa1e0, summary = 0x47d3330 [CamelEasSummary], folder_flags = CAMEL_FOLDER_HAS_SUMMARY_CAPABILITY, permanent_flags = 4127, later = {0x0, 0x0, 0x0, 0x0}}, priv = 0x1baa240}, priv = 0x1baa250, search = 0x7fff140185c0 [CamelFolderSearch], cache = 0x7fff9000e0e0 [CamelDataCache]} (gdb) p *(CamelEasSummary *)0x47d3330 $7 = {parent = {parent = {parent = {g_type_instance = {g_class = 0x7fffc4004a60}, ref_count = 1, qdata = 0x0}, priv = 0x47d33b0}, priv = 0x47d33c0, version = 14, time = 0, flags = (unknown: 0), collate = 0x0, sort_by = 0x0, later = {0x0, 0x0, 0x0, 0x0}}, sync_state = 0x7fff29d84040 "1986612094", version = 1}
Looking at the code in toggle_refs_notify() I don't really understand how it can be correct. It looks like this: G_UNLOCK (toggle_refs_mutex); /* Reentrancy here is not as tricky as it seems, because a toggle reference * will only be notified when there is exactly one of them. */ g_assert (tstack.n_toggle_refs == 1); tstack.toggle_refs[0].notify (tstack.toggle_refs[0].data, tstack.object, is_last_ref); } Note: It *drops* toggle_refs_mutex. Then it can sleep for an unspecified period of time, while g_object_add_toggle_ref() or g_object_remove_toggle_ref() can be called. And then it hits the assert(), with absolutely no justification for beliving that n_toggle_refs will *still* be precisely one, even if that was the case when the function was called.
It looks like toggle_ref is not intended to be used for this purpose. It isn't entirely thread-safe. This usage of toggle_ref was added in commit 3ee0ee8a in order to fix bug 638810, and a glib RFE (bug 667424) was filed to give us a better way of coping with it. That glib RFE has been responded to, with "see GWeakRef". That is probably the answer.
Created attachment 243627 [details] [review] Attempted patch Completely untested. Does compile.
Review of attachment 243627 [details] [review]: ::: camel/camel-object-bag.c @@ +348,3 @@ + /* We have an object; no need to reserve the key. */ + object_bag_unreserve (bag, key); + object = g_weak_ref_get (&ref->ref); You already did g_weak_ref_get() above @@ +415,3 @@ gpointer copied_key; + ref = g_new(ObjRef, 1); Perhaps g_slice_new() should be used instead
Created attachment 243638 [details] [review] New attempt New patch incorporating the above feedback (thanks) and also results from actual testing. Incremental patch below for illustration: --- a/camel/camel-object-bag.c.gob2 2013-05-08 23:22:06.903789921 +0100 +++ a/camel/camel-object-bag.c 2013-05-08 23:29:37.030229134 +0100 @@ -120,7 +120,7 @@ wref_free_func (gpointer p) ObjRef *ref = p; g_weak_ref_clear (&ref->ref); - g_free (ref); + g_slice_free (ObjRef, ref); } /** @@ -347,7 +347,6 @@ camel_object_bag_reserve (CamelObjectBag if (object != NULL) { /* We have an object; no need to reserve the key. */ object_bag_unreserve (bag, key); - object = g_weak_ref_get (&ref->ref); } /* Remove stale reference to dead object. */ @@ -414,7 +413,7 @@ camel_object_bag_add (CamelObjectBag *ba ObjRef *ref; gpointer copied_key; - ref = g_new(ObjRef, 1); + ref = g_slice_new(ObjRef); /* We need to stash a 'raw' pointer since that's the key we use in the key_table */ ref->obj = object; @@ -523,7 +522,9 @@ camel_object_bag_list (CamelObjectBag *b values = g_hash_table_get_values (bag->object_table); while (values != NULL) { - g_ptr_array_add (array, g_object_ref (values->data)); + GObject *obj = g_weak_ref_get (values->data); + if (obj) + g_ptr_array_add (array, obj); values = g_list_delete_link (values, values); }
Created attachment 243677 [details] [review] Use g_object_weak_ref too, to avoid potential memory leak I was concerned that an approach based purely on GWeakRef might leave the "stale" GWeakRef corresponding to dead objects around for ever, which is effectively a memory leak. It's all very well removing them "on demand" when we try to look them up or add something in their place, but that might not happen. So this version adds the g_object_weak_ref() callback again, while preserving the safety that's provided by using GWeakRef. Incremental patch from before: --- a/camel/camel-object-bag.c 2013-05-08 23:29:37.030229134 +0100 +++ a/camel/camel-object-bag.c 2013-05-09 09:42:59.959862625 +0100 @@ -111,14 +111,41 @@ typedef struct { GWeakRef ref; gconstpointer *obj; + CamelObjectBag *bag; } ObjRef; -/* Properly destroy a GWeakRef as it's freed from the hash table */ +static void +object_bag_notify (CamelObjectBag *bag, + GObject *where_the_object_was) +{ + gconstpointer key; + + g_mutex_lock (&bag->mutex); + + key = g_hash_table_lookup (bag->key_table, where_the_object_was); + if (key != NULL) { + g_hash_table_remove (bag->key_table, where_the_object_was); + g_hash_table_remove (bag->object_table, key); + } + + g_mutex_unlock (&bag->mutex); +} + +/* Properly destroy an ObjRef as it's freed from the hash table */ static void wref_free_func (gpointer p) { ObjRef *ref = p; + GObject *obj = g_weak_ref_get (&ref->ref); + if (obj) { + /* The object is being removed from the bag while it's + still alive, e.g. by camel_object_bag_remove() + or camel_object_bag_destroy(). Drop the weak_ref. */ + g_object_weak_unref (obj, (GWeakNotify)object_bag_notify, + ref->bag); + g_object_unref (obj); + } g_weak_ref_clear (&ref->ref); g_slice_free (ObjRef, ref); } @@ -414,6 +441,7 @@ gpointer copied_key; ref = g_slice_new(ObjRef); + ref->bag = bag; /* We need to stash a 'raw' pointer since that's the key we use in the key_table */ ref->obj = object; @@ -422,6 +450,9 @@ g_hash_table_insert (bag->key_table, object, copied_key); g_hash_table_insert (bag->object_table, copied_key, ref); object_bag_unreserve (bag, key); + + g_object_weak_ref (G_OBJECT (object), + (GWeakNotify) object_bag_notify, bag); } g_mutex_unlock (&bag->mutex); @@ -522,7 +553,8 @@ values = g_hash_table_get_values (bag->object_table); while (values != NULL) { - GObject *obj = g_weak_ref_get (values->data); + ObjRef *ref = values->data; + GObject *obj = g_weak_ref_get (&ref->ref); if (obj) g_ptr_array_add (array, obj); values = g_list_delete_link (values, values);
Review of attachment 243677 [details] [review]: I'd like to do thorough testing with all the below addressed (including coding style issues not mentioned here) :) ::: a/camel/camel-object-bag.c.gob @@ +111,3 @@ +typedef struct { + GWeakRef ref; + gconstpointer *obj; it should be 'gpointer obj;' instead @@ +119,3 @@ + GObject *where_the_object_was) +{ + gconstpointer key; here are used spaces, instead of tab @@ +143,3 @@ + still alive, e.g. by camel_object_bag_remove() + or camel_object_bag_destroy(). Drop the weak_ref. */ + g_object_weak_unref (obj, (GWeakNotify)object_bag_notify, missing space after the cast; There are plenty of other similar coding style issues @@ +259,3 @@ + if (object != NULL) { + g_mutex_unlock (&bag->mutex); + return object; by this you keep a reservation in the bag on the object, thus the return should not be here @@ +377,3 @@ + } + + /* Remove stale reference to dead object. */ it's not necessarily dead here (should be in 'else' body)
Created attachment 243802 [details] [review] Patch #4 Thanks for the feedback.
Review of attachment 243802 [details] [review]: Thanks for the fix. Just fix the below and commit, to master only. I would keep this in master only, for better testing. You said you got it only once, thus it's a big race condition anyway. ::: camel/camel-object-bag.c @@ +111,3 @@ +typedef struct { + GWeakRef ref; + gpointer *obj; I didn't mean only gconstpointer replace, I meant that a gpointer is a 'void *' already, thus you get void **obj, which is not what you want.
Committed; thanks. https://git.gnome.org/browse/evolution-data-server/commit/?id=87b365d9 (Ick, this is why I hate gratuitous typedefs like gpointer. If it's a pointer, it should have a * on it.)
David, can be a downstream bug report [1] related to your changes? It crashed on place where it didn't crash for years, and the 3.8.3 contains the above fix. [1] https://bugzilla.redhat.com/show_bug.cgi?id=979689