Merge lp:~ted/libindicator/lp719457 into lp:libindicator/0.5

[Ted Gould]

Saving the string before doing the remove so the string being free'd doesn't effect us.

[Charles Kerr (charlesk)]

libindicator doesn't appear to be violating the glib API -- this smells like a bug in GHashTable that we'd be better off reporting upstream instead of using a local band-aid.

Also, nobody's reported this since March 16 last year, and glib has revved several times since then. Maybe it would make sense to CC desrt to see if that valgrind log looks familiar?

[Ted Gould (ted)]

On Sun, 2012-03-04 at 21:34 +0000, charles wrote:
> libindicator doesn't appear to be violating the glib API -- this
> smells like a bug in GHashTable that we'd be better off reporting
> upstream instead of using a local band-aid.
>
> Also, nobody's reported this since March 16 last year, and glib has
> revved several times since then. Maybe it would make sense to CC desrt
> to see if that valgrind log looks familiar?

I actually don't think it's a bug in the hashtable, it's that the remove
in the hashtable us causing an unwatch to be called, which is freeing
the datastructure with the string. It is something that is likely to be
difficult to fix in GDBus as it would have to then queue the destruction
of the data structure until the current call stack returns. Since
that's a non-trivial change, I figured it would be worth working around.

[Charles Kerr (charlesk)]

Oh, okay. Thanks for the walkthrough Ted :)

review: Approve

=== modified file 'libindicator/indicator-service.c'
--- libindicator/indicator-service.c	2011-07-14 18:08:31 +0000
+++ libindicator/indicator-service.c	2012-03-02 16:58:21 +0000
@@ -588,7 +588,9 @@
 	/* Remove us from the watcher list here */
 	gpointer watcher_item = g_hash_table_lookup(priv->watchers, name);
 	if (watcher_item != NULL) {
-		g_hash_table_remove(priv->watchers, name);
+		gchar * safe_name = g_strdup(name);
+		g_hash_table_remove(priv->watchers, safe_name);
+		g_free(safe_name);
 	} else {
 		/* Odd that we couldn't find the person, but, eh */
 		g_warning("Unable to find watcher who is unwatching: %s", name);