Merge lp:~charlesk/appmenu-gtk/lp-787736 into lp:appmenu-gtk/12.10

[Charles Kerr]

Fix a memory leak -- RebuildStructs were being leaked. (LP #787736)

Putting this fix in a standalone branch, without a half-dozen surrounding interwoven cleanup commits, to make it easier to cherry pick

[jenkins (martin-mrazik+qa)]

PASSED: Continuous integration, rev:154
http://s-jenkins:8080/job/appmenu-gtk-ci/1/

review: Approve (continuous-integration)

[jenkins (martin-mrazik+qa)]

PASSED: Continuous integration, rev:156
http://jenkins.qa.ubuntu.com/job/appmenu-gtk-ci/6/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/appmenu-gtk-ci/./label=precise/6/console

review: Approve (continuous-integration)

[Lars Karlitski (larsu)]

That's much cleaner, I like it.

* rebuild_list_clear is called in both dispose and in finalize

* rebuild_timer_func is the callback for the timer to rebuild widgets. It calls rebuild_list_clear, which removes the timer source with g_source_remove. Is removing sources in their callbacks allowed?

* you could be making life easier for this particular reviewer if you return G_SOURCE_REMOVE instead of FALSE in rebuild_timer_func (he has trouble remembering if TRUE or FALSE remove a source)

review: Needs Information

[Charles Kerr (charlesk)]

* wrt rebuild_list_clear being called twice, yes. I was continuing the double-clear behavior that's been in place since http://bazaar.launchpad.net/~canonical-dx-team/appmenu-gtk/trunk.12.10/revision/119.3.3 but it's not clear to me if the original behavior was necessary or accidental.

* removing sources in their callbacks works fine.

* Ah, G_SOURCE_REMOVE is a nice mnemonic device. I didn't know about that!

[jenkins (martin-mrazik+qa)]

PASSED: Continuous integration, rev:158
http://jenkins.qa.ubuntu.com/job/appmenu-gtk-ci/7/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/appmenu-gtk-ci/./label=precise/7/console

review: Approve (continuous-integration)

[Lars Karlitski (larsu)]

Thanks for the info!

review: Approve

[jenkins (martin-mrazik+qa)]

FAILED: Autolanding.
No commit message was specified.
http://jenkins.qa.ubuntu.com/job/appmenu-gtk-autolanding/3/

review: Needs Fixing (continuous-integration)

[Charles Kerr (charlesk)]

(That failure message from jenkins looks like a false negative. Plus, the same code passed twelve hours ago.)

[jenkins (martin-mrazik+qa)]

FAILED: Autolanding.
No commit message was specified.
http://jenkins.qa.ubuntu.com/job/appmenu-gtk-autolanding/4/

review: Needs Fixing (continuous-integration)

[Charles Kerr (charlesk)]

:/

[Allan LeSage (allanlesage)]

Charles here's the link to that dput-failure: go ahead and post a commit message and I'll re-start the job: http://s-jenkins/job/generic-land/346/console

[Charles Kerr (charlesk)]

Landing this manually after talking over the CI problem with Allan.

=== modified file 'src/bridge.c'
--- src/bridge.c	2012-08-22 06:56:22 +0000
+++ src/bridge.c	2012-08-30 13:08:20 +0000
@@ -85,21 +85,13 @@
 
   GDBusProxy       *appmenuproxy;
   gboolean          online;
+
+  GSList           *rebuild_list;
+  guint             rebuild_timer_id;
 };
 
-typedef struct _RecurseContext
-{
-  AppMenuBridge    *bridge;
-  AppWindowContext *context;
-
-  gint count;
-  gboolean previous;
-  DbusmenuMenuitem *stack[30];
-} RecurseContext;
-
 G_DEFINE_DYNAMIC_TYPE(AppMenuBridge, app_menu_bridge, UBUNTU_TYPE_MENU_PROXY)
 
-static GHashTable *rebuild_ids = NULL;
 static GDBusNodeInfo *      registrar_node_info = NULL;
 static GDBusInterfaceInfo * registrar_interface_info = NULL;
 
@@ -213,28 +205,59 @@
   g_free (context);
 }
 
+
+/* a widget that was queued for rebuild has been destroyed,
+   so remove it from our queue */
+static void
+rebuild_list_widget_destroyed (gpointer gbridge, GObject * dead_object)
+{
+  AppMenuBridge * bridge = APP_MENU_BRIDGE (gbridge);
+  AppMenuBridgePrivate * p = bridge->priv;
+
+  p->rebuild_list = g_slist_remove (p->rebuild_list, dead_object);
+}
+
+static void
+rebuild_list_clear (AppMenuBridge * bridge)
+{
+  GSList * l;
+  AppMenuBridgePrivate * p = bridge->priv;
+  g_debug (G_STRLOC" clearing bridge %p's rebuild list", bridge);
+
+  /* unref the widgets and blow away the list */
+  for (l=p->rebuild_list; l!=NULL; l=l->next)
+    g_object_weak_unref (l->data, rebuild_list_widget_destroyed, bridge);
+  g_slist_free (p->rebuild_list);
+  p->rebuild_list = NULL;
+
+  /* remove the rebuild timer */
+  if (p->rebuild_timer_id > 0)
+    {
+      g_source_remove (p->rebuild_timer_id);
+      p->rebuild_timer_id = 0;
+    }
+}
+
 static void
 app_menu_bridge_dispose (GObject *object)
 {
   AppMenuBridge *bridge = APP_MENU_BRIDGE (object);
-
-  g_list_foreach (bridge->priv->windows, (GFunc)context_dispose, NULL);
-
-  if (bridge->priv->appmenuproxy)
-    {
-      g_object_unref (bridge->priv->appmenuproxy);
-      bridge->priv->appmenuproxy = NULL;
-    }
+  AppMenuBridgePrivate * p = bridge->priv;
+
+  rebuild_list_clear (bridge);
+  g_list_foreach (p->windows, (GFunc)context_dispose, NULL);
+  g_clear_object (&p->appmenuproxy);
 }
 
 static void
 app_menu_bridge_finalize (GObject *object)
 {
   AppMenuBridge *bridge = APP_MENU_BRIDGE (object);
+  AppMenuBridgePrivate * p = bridge->priv;
 
-  g_list_foreach (bridge->priv->windows, (GFunc)context_free, NULL);
-  g_list_free (bridge->priv->windows);
-  bridge->priv->windows = NULL;
+  g_list_foreach (p->windows, (GFunc)context_free, NULL);
+  g_list_free (p->windows);
+  p->windows = NULL;
 
   G_OBJECT_CLASS (app_menu_bridge_parent_class)->finalize (object);
 }
@@ -505,63 +528,56 @@
 	return;
 }
 
-typedef struct _RebuildData {
-  AppMenuBridge *bridge;
-  GtkWidget     *widget;
-} RebuildData;
-
 static gboolean
-do_rebuild (RebuildData *data)
-{
-  if (data->widget != NULL && gtk_widget_is_toplevel (data->widget))
-    {
-      rebuild_window_items (data->bridge,
-                            data->widget);
-    }
-
-  if (data->widget != NULL)
-    {
-      g_object_remove_weak_pointer (G_OBJECT (data->widget), (gpointer*)&data->widget);
-      g_hash_table_remove (rebuild_ids, data->widget);
-    }
-
-  g_free (data);
-
-  return FALSE;
-}
-
-static void
-rebuild (AppMenuBridge *bridge,
-         GtkWidget     *toplevel)
-{
-  guint id = 0;
-
-  if (rebuild_ids != NULL)
-    {
-      id = GPOINTER_TO_UINT (g_hash_table_lookup (rebuild_ids, toplevel));
-
-      if (id > 0)
+rebuild_timer_func (gpointer gbridge)
+{
+  GSList * l;
+  AppMenuBridge *bridge = APP_MENU_BRIDGE (gbridge);
+  AppMenuBridgePrivate * p = bridge->priv;
+
+  /* call rebuild_window_items() for each toplevel in our list */
+  for (l=p->rebuild_list; l!=NULL; l=l->next)
+    {
+      GtkWidget * w = GTK_WIDGET(l->data);
+
+      if (gtk_widget_is_toplevel(w))
         {
-          g_source_remove (id);
-          g_hash_table_remove (rebuild_ids, toplevel);
-          id = 0;
+          g_debug (G_STRLOC" rebuilding bridge %p's toplevel %p", bridge, w);
+          rebuild_window_items (bridge, w);
         }
     }
 
-  RebuildData *data = g_new0 (RebuildData, 1);
-  data->bridge = bridge;
-  data->widget = toplevel;
-
-  id = gdk_threads_add_timeout (100, (GSourceFunc)do_rebuild, data);
-
-  g_object_add_weak_pointer (G_OBJECT (data->widget), (gpointer*)&data->widget);
-
-  if (rebuild_ids == NULL)
-    {
-      rebuild_ids = g_hash_table_new (g_direct_hash, g_direct_equal);
-    }
-
-  g_hash_table_insert (rebuild_ids, toplevel, GUINT_TO_POINTER (id));
+  /* cleanup */
+  rebuild_list_clear (bridge);
+  return G_SOURCE_REMOVE;
+}
+
+/* widgets tend to get a lot of changes all at once...
+ + inserting a 100 msec delay here lets us batch up those changes */
+static void
+rebuild (AppMenuBridge * bridge, GtkWidget * toplevel)
+{
+  AppMenuBridgePrivate * p = bridge->priv;
+
+  g_return_if_fail (toplevel != NULL);
+
+  /* ensure there's a rebuild timer ticking */
+  if (p->rebuild_timer_id == 0)
+    {
+      g_debug (G_STRLOC" creating rebuild timer");
+      p->rebuild_timer_id = gdk_threads_add_timeout (100,
+                                                     rebuild_timer_func,
+                                                     bridge);
+    }
+
+  /* ensure this widget is in our rebuild list */
+  if (g_slist_find (p->rebuild_list, toplevel) == NULL)
+    {
+      GObject * o = G_OBJECT(toplevel);
+      g_debug (G_STRLOC" adding widget %p to bridge %p's queue", o, bridge);
+      p->rebuild_list = g_slist_prepend (p->rebuild_list, o);
+      g_object_weak_ref (o, rebuild_list_widget_destroyed, bridge);
+    }
 }
 
 static DbusmenuMenuitem * find_menu_bar (GtkWidget * widget);
@@ -989,9 +1005,4 @@
 G_MODULE_EXPORT void
 menu_proxy_module_unload (UbuntuMenuProxyModule *module)
 {
-  if (rebuild_ids)
-    {
-      g_hash_table_destroy (rebuild_ids);
-      rebuild_ids = NULL;
-    }
 }