[Telepathy-commits] [telepathy-idle/master] Free a bunch more memory leaks reported by valgrind

[Jonathon Jongsma]

Many telepathy signals take GPtrArrays of specialized dbus-glib boxed types.
The basic pattern for all of these leaks is the same:
- create a GValue for the specialized dbus type
- call g_value_take_boxed(foo, specialized-dbus-type);
- dbus_g_type_struct_set(...) /* set struct props */
- g_ptr_array_add(array, g_value_get_boxed())
- emit tp signal
- free ptr array

The problem is that even though the pointer array of the boxed types are freed,
the boxed types themselves are not properly freed.  So you need to go through
the list and call g_boxed_free on all of the array elements before freeing the
array (or call g_value_unset() after you're done with the ptr array, which will
result in the boxed type being freed by the GValue since it was initialized with
take_boxed()).

In addition, there was a somewhat related/intertwined bug in the topic handlers:
- in idle_muc_channel_topic_touch(), where we were calling g_value_get_boxed()
  and adding it to the GPtrArray, then modifying the GValue, calling
  g_value_get_boxed(), and adding that to the array again.  The intent was to
  add two different objects to the array, but since we were not copying the
  boxed type (e.g. g_value_dup_boxed()), the result was that we were adding a
  pointer to a boxed type in the array, modifying the object and then adding it
  again, so we ended up with two identical boxed type objects in the array
  rather than two different objects.  I'm not entirely sure what effect this bug
  had on things, but it should be fixed now regardless.
- in idle_muc_channel_topic_full(), we had the same problem as above, but in
  addition, the 'subject' property was never actually added to the ptr array, so
  I added this call.
---
 src/idle-connection.c  |    6 +++++
 src/idle-muc-channel.c |   52 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/src/idle-connection.c b/src/idle-connection.c
index 6c2d645..d884d0a 100644
--- a/src/idle-connection.c
+++ b/src/idle-connection.c
@@ -66,6 +66,11 @@
 #define IDLE_TP_ALIAS_PAIR_TYPE (dbus_g_type_get_struct ("GValueArray", \
 			G_TYPE_UINT, G_TYPE_STRING, G_TYPE_INVALID))
 
+static void _free_alias_pair(gpointer data, gpointer user_data)
+{
+	g_boxed_free(IDLE_TP_ALIAS_PAIR_TYPE, data);
+}
+
 static void _aliasing_iface_init(gpointer, gpointer);
 static void _renaming_iface_init(gpointer, gpointer);
 
@@ -876,6 +881,7 @@ void idle_connection_emit_queued_aliases_changed(IdleConnection *conn) {
 
 	tp_svc_connection_interface_aliasing_emit_aliases_changed(conn, priv->queued_aliases);
 
+	g_ptr_array_foreach(priv->queued_aliases, _free_alias_pair, NULL);
 	g_ptr_array_free(priv->queued_aliases, TRUE);
 	priv->queued_aliases = NULL;
 
diff --git a/src/idle-muc-channel.c b/src/idle-muc-channel.c
index 85e1bcd..b89b6cc 100644
--- a/src/idle-muc-channel.c
+++ b/src/idle-muc-channel.c
@@ -184,6 +184,22 @@ static const gchar *ascii_muc_states[] = {
 	"MUC_STATE_PARTED"
 };
 
+static void _free_prop_value_struct(gpointer data, gpointer user_data)
+{
+	g_boxed_free(TP_TYPE_PROPERTY_VALUE_STRUCT, data);
+}
+
+static void _free_flags_struct(gpointer data, gpointer user_data)
+{
+	g_boxed_free(TP_TYPE_PROPERTY_FLAGS_STRUCT, data);
+}
+
+static void _free_prop_info_struct(gpointer data, gpointer user_data)
+{
+	g_boxed_free(TP_TYPE_PROPERTY_INFO_STRUCT, data);
+}
+
+
 static gboolean add_member(GObject *gobj, TpHandle handle, const gchar *message, GError **error);
 static gboolean remove_member(GObject *gobj, TpHandle handle, const gchar *message, GError **error);
 
@@ -626,6 +642,7 @@ static void set_tp_property_flags(IdleMUCChannel *chan, const GArray *props, TpP
 		tp_svc_properties_interface_emit_property_flags_changed((TpSvcPropertiesInterface *)(chan), changed_props);
 	}
 
+	g_ptr_array_foreach(changed_props, _free_flags_struct, NULL);
 	g_ptr_array_free(changed_props, TRUE);
 }
 
@@ -845,6 +862,9 @@ static void change_mode_state(IdleMUCChannel *obj, guint add, guint remove) {
 
 	priv->mode_state.flags = flags;
 
+	g_ptr_array_foreach(tp_props_to_change, _free_prop_value_struct, NULL);
+	g_ptr_array_free(tp_props_to_change, TRUE);
+
 	IDLE_DEBUG("changed to %x", flags);
 }
 
@@ -1212,6 +1232,8 @@ void idle_muc_channel_topic(IdleMUCChannel *chan, const char *topic) {
 	arr = g_ptr_array_new();
 	g_ptr_array_add(arr, g_value_get_boxed(&prop));
 	change_tp_properties(chan, arr);
+	g_value_unset(&val);
+	g_value_unset(&prop);
 	g_ptr_array_free(arr, TRUE);
 }
 
@@ -1232,23 +1254,26 @@ void idle_muc_channel_topic_touch(IdleMUCChannel *chan, const TpHandle toucher,
 	g_value_set_uint(&val, toucher);
 
 	dbus_g_type_struct_set(&prop,
-							0, TP_PROPERTY_SUBJECT_CONTACT,
-							1, &val,
-							G_MAXUINT);
+			       0, TP_PROPERTY_SUBJECT_CONTACT,
+			       1, &val,
+			       G_MAXUINT);
 
-	g_ptr_array_add(arr, g_value_get_boxed(&prop));
+	/* need to copy it out with g_value_dup_boxed() so we don't over-write
+	 * it when we re-use this GValue for the next property */
+	g_ptr_array_add(arr, g_value_dup_boxed(&prop));
 
 	g_value_set_uint(&val, timestamp);
 
 	dbus_g_type_struct_set(&prop,
-							0, TP_PROPERTY_SUBJECT_TIMESTAMP,
-							1, &val,
-							G_MAXUINT);
+			       0, TP_PROPERTY_SUBJECT_TIMESTAMP,
+			       1, &val,
+			       G_MAXUINT);
 
 	g_ptr_array_add(arr, g_value_get_boxed(&prop));
 
 	change_tp_properties(chan, arr);
 
+	g_ptr_array_foreach(arr, _free_prop_value_struct, NULL);
 	g_ptr_array_free(arr, TRUE);
 }
 
@@ -1271,14 +1296,16 @@ void idle_muc_channel_topic_full(IdleMUCChannel *chan, const TpHandle toucher, c
 			0, TP_PROPERTY_SUBJECT_CONTACT,
 			1, &val,
 			G_MAXUINT);
-	g_ptr_array_add(arr, g_value_get_boxed(&prop));
+	/* need to copy it out with g_value_dup_boxed() so we don't over-write
+	 * it when we re-use this GValue for the next property */
+	g_ptr_array_add(arr, g_value_dup_boxed(&prop));
 
 	g_value_set_uint(&val, timestamp);
 	dbus_g_type_struct_set(&prop,
 			0, TP_PROPERTY_SUBJECT_TIMESTAMP,
 			1, &val,
 			G_MAXUINT);
-	g_ptr_array_add(arr, g_value_get_boxed(&prop));
+	g_ptr_array_add(arr, g_value_dup_boxed(&prop));
 
 	g_value_unset(&val);
 	g_value_init(&val, G_TYPE_STRING);
@@ -1287,8 +1314,11 @@ void idle_muc_channel_topic_full(IdleMUCChannel *chan, const TpHandle toucher, c
 							0, TP_PROPERTY_SUBJECT,
 							1, &val,
 							G_MAXUINT);
+	g_ptr_array_add(arr, g_value_get_boxed(&prop));
+
 	change_tp_properties(chan, arr);
 
+	g_ptr_array_foreach(arr, _free_prop_value_struct, NULL);
 	g_ptr_array_free(arr, TRUE);
 }
 
@@ -1695,7 +1725,7 @@ static void idle_muc_channel_get_properties (TpSvcPropertiesInterface *iface, co
 			dbus_g_method_return_error(context, error);
 			g_error_free(error);
 
-			return;;
+			return;
 		}
 	}
 
@@ -1719,6 +1749,7 @@ static void idle_muc_channel_get_properties (TpSvcPropertiesInterface *iface, co
 
 	tp_svc_properties_interface_return_from_get_properties(context, ret);
 
+	g_ptr_array_foreach(ret, _free_prop_value_struct, NULL);
 	g_ptr_array_free(ret, TRUE);
 }
 
@@ -1795,6 +1826,7 @@ static void idle_muc_channel_list_properties (TpSvcPropertiesInterface *iface, D
 
 	tp_svc_properties_interface_return_from_list_properties(context, ret);
 
+	g_ptr_array_foreach(ret, _free_prop_info_struct, NULL);
 	g_ptr_array_free(ret, TRUE);
 }
 
-- 
1.5.6.5