(gdb) t a a bt

+ Trace 231914

(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