[next] telepathy-glib: name owner watching: more debug, and use a free-function to free watches

[Simon McVittie]

Module: telepathy-glib
Branch: next
Commit: d7d4acae55a66164d18a45cfedd7425890a41363
URL:    http://cgit.freedesktop.org/telepathy/telepathy-glib/commit/?id=d7d4acae55a66164d18a45cfedd7425890a41363

Author: Simon McVittie <simon.mcvittie at collabora.co.uk>
Date:   Fri Mar 21 19:01:08 2014 +0000

name owner watching: more debug, and use a free-function to free watches

Several Mission Control tests crash when trying to add more callbacks
to a _NameOwnerWatch that has not been removed from the hash table
after calling _tp_dbus_daemon_stop_watching(). It's not clear to me
how that can happen, but turning _tp_dbus_daemon_stop_watching()
into an ordinary free-function ensures that "remove it from the hash
table" and "free it" happen in the right order.

---

 telepathy-glib/dbus-daemon.c |   33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/telepathy-glib/dbus-daemon.c b/telepathy-glib/dbus-daemon.c
index 184d00a..0b64018 100644
--- a/telepathy-glib/dbus-daemon.c
+++ b/telepathy-glib/dbus-daemon.c
@@ -160,8 +160,7 @@ typedef struct
   GDestroyNotify destroy;
 } _NameOwnerSubWatch;
 
-static void _tp_dbus_daemon_stop_watching (TpDBusDaemon *self,
-    const gchar *name, _NameOwnerWatch *watch);
+static void name_owner_watch_free (gpointer p);
 
 static void
 tp_dbus_daemon_maybe_free_name_owner_watch (TpDBusDaemon *self,
@@ -179,6 +178,8 @@ tp_dbus_daemon_maybe_free_name_owner_watch (TpDBusDaemon *self,
   if (watch->invoking > 0)
     return;
 
+  DEBUG ("%s: %p (%u callbacks in %p)", name, watch, array->len, array);
+
   for (i = array->len; i > 0; i--)
     {
       _NameOwnerSubWatch *entry = &g_array_index (array,
@@ -187,6 +188,8 @@ tp_dbus_daemon_maybe_free_name_owner_watch (TpDBusDaemon *self,
       if (entry->callback != NULL)
         continue;
 
+      DEBUG ("compacting expired entry #%u", i - 1);
+
       if (entry->destroy != NULL)
         entry->destroy (entry->user_data);
 
@@ -195,7 +198,7 @@ tp_dbus_daemon_maybe_free_name_owner_watch (TpDBusDaemon *self,
 
   if (array->len == 0)
     {
-      _tp_dbus_daemon_stop_watching (self, name, watch);
+      DEBUG ("no more callbacks, removing %p", watch);
       g_hash_table_remove (self->priv->name_owner_watches, name);
     }
 }
@@ -329,12 +332,12 @@ tp_dbus_daemon_watch_name_owner (TpDBusDaemon *self,
 
   if (watch == NULL)
     {
-      DEBUG ("- new watch");
       /* Allocate a new watch */
       watch = g_slice_new0 (_NameOwnerWatch);
       watch->last_owner = NULL;
       watch->callbacks = g_array_new (FALSE, FALSE,
           sizeof (_NameOwnerSubWatch));
+      DEBUG ("- new watch %p (callbacks at %p)", watch, watch->callbacks);
 
       g_hash_table_insert (self->priv->name_owner_watches, g_strdup (name),
           watch);
@@ -349,7 +352,8 @@ tp_dbus_daemon_watch_name_owner (TpDBusDaemon *self,
     }
   else
     {
-      DEBUG ("- appending to existing watch");
+      DEBUG ("- appending to existing watch %p (callbacks at %p)", watch,
+          watch->callbacks);
     }
 
   g_array_append_val (watch->callbacks, tmp);
@@ -363,10 +367,13 @@ tp_dbus_daemon_watch_name_owner (TpDBusDaemon *self,
 }
 
 static void
-_tp_dbus_daemon_stop_watching (TpDBusDaemon *self,
-                               const gchar *name,
-                               _NameOwnerWatch *watch)
+name_owner_watch_free (gpointer p)
 {
+  _NameOwnerWatch *watch = p;
+
+  DEBUG ("%p (%u callbacks in %p)", watch, watch->callbacks->len,
+      watch->callbacks);
+
   /* Clean up any leftöver callbacks. */
   if (watch->callbacks->len > 0)
     {
@@ -418,7 +425,7 @@ tp_dbus_daemon_cancel_name_owner_watch (TpDBusDaemon *self,
   g_return_val_if_fail (name != NULL, FALSE);
   g_return_val_if_fail (callback != NULL, FALSE);
 
-  DEBUG ("%s", name);
+  DEBUG ("%s (%p)", name, watch);
 
   if (watch != NULL)
     {
@@ -429,7 +436,7 @@ tp_dbus_daemon_cancel_name_owner_watch (TpDBusDaemon *self,
        * we iterate in reverse to have "last in = first out" as documented. */
       guint i;
 
-      DEBUG ("- %u watch(es) found", array->len);
+      DEBUG ("- %u watch(es) in %p", array->len, array);
 
       for (i = array->len; i > 0; i--)
         {
@@ -438,7 +445,7 @@ tp_dbus_daemon_cancel_name_owner_watch (TpDBusDaemon *self,
 
           if (entry->callback == callback && entry->user_data == user_data)
             {
-              DEBUG ("- found matching callback and user data");
+              DEBUG ("- found matching callback and user data (#%u)", i - 1);
               entry->callback = NULL;
               tp_dbus_daemon_maybe_free_name_owner_watch (self, name, watch);
               return TRUE;
@@ -1042,7 +1049,7 @@ tp_dbus_daemon_init (TpDBusDaemon *self)
       TpDBusDaemonPrivate);
 
   self->priv->name_owner_watches = g_hash_table_new_full (g_str_hash,
-      g_str_equal, g_free, NULL);
+      g_str_equal, g_free, name_owner_watch_free);
 }
 
 static void
@@ -1065,7 +1072,7 @@ tp_dbus_daemon_dispose (GObject *object)
 
           /* it refs us while invoking stuff */
           g_assert (watch->invoking == 0);
-          _tp_dbus_daemon_stop_watching (self, k, watch);
+          DEBUG ("removing watch for %s: %p", (gchar *) k, watch);
           g_hash_table_iter_remove (&iter);
         }