Merge lp:~bregma/unity/fix-indicator-leaks into lp:unity

[Stephen M. Webb]

dispose of ATK objects when indicators are removed (lp: #1231222)

= Problem description =

A circular ref between indicator objects and their ATK counterparts cause indicators to never get disposed, which shows up as nenory leaks on unity-panel-service shutdown. This makes it very difficult to spot other memory leaks.

= The fix =

Added a signal to dispose of the ATK objects before the indicator objects are removed.

= Test coverage =

Requires a manual test for verification: run unity-panel-service with the environment variable G_MESSAGES_DEBUG=u-p-s and verify indicator refcounts are all 1 when shut down.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3533
http://jenkins.qa.ubuntu.com/job/unity-ci/396/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-saucy-amd64-ci/285/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-saucy-armhf-ci/283/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-saucy-i386-ci/284/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-ci/396/rebuild

review: Needs Fixing (continuous-integration)

[Christopher Townsend (townsend)]

Getting a segfault in a Panel Service test:

[ RUN ] TestPanelService.ManyEmptyIndicatorObjectsAddition
Segmentation fault

review: Needs Fixing

[Marco Trevisan (Treviño) (3v1n0)]

 - g_object_ref (indicator);

Why removing this? That code is used only indicators that have been instantiated somewhere else (not created by the ups, and what we call custom_indicators used only for testing), so in this case we should take the ownership of the indicator as there's no warranty that the previous owner is still alive (and that's why the test crashes).

Also I think that the code
132 if (g_object_is_floating (G_OBJECT (indicator)))
133 {
134 g_object_ref_sink (G_OBJECT (indicator));
135 }

Is not needed anymore now (indicators are no more GInitiallyUnowned, and even in that case it was placed in the wrong spot :/).

Unmerged revisions

3533. By Stephen M. Webb on 2013-09-26

broke some circular refs during shutdown to reduce memory leaks reported by valgrind (lp: #1231222)

3532. By Stephen M. Webb on 2013-09-25

unity-panel-service: add debug domain

=== modified file 'services/CMakeLists.txt'
--- services/CMakeLists.txt	2013-08-07 16:37:05 +0000
+++ services/CMakeLists.txt	2013-09-26 02:15:48 +0000
@@ -38,7 +38,7 @@
 set(CFLAGS
     ${SERVICE_DEPS_CFLAGS}
     ${SERVICE_DEPS_CFLAGS_OTHER}
-    "-Werror -Wall"
+    "-Werror -Wall -DG_LOG_DOMAIN=\\\"u-p-s\\\""
     )
 
 string (REPLACE ";" " " CFLAGS "${CFLAGS}")
 
=== modified file 'services/panel-indicator-accessible.c'
--- services/panel-indicator-accessible.c	2011-10-05 16:54:21 +0000
+++ services/panel-indicator-accessible.c	2013-09-26 02:15:48 +0000
@@ -384,3 +384,9 @@
 
   return state_set;
 }
+
+IndicatorObject *
+panel_indicator_accessible_get_indicator (PanelIndicatorAccessible *pia)
+{
+  return pia->priv->indicator;
+}
 
=== modified file 'services/panel-indicator-accessible.h'
--- services/panel-indicator-accessible.h	2011-02-08 11:50:18 +0000
+++ services/panel-indicator-accessible.h	2013-09-26 02:15:48 +0000
@@ -49,6 +49,7 @@
 
 GType      panel_indicator_accessible_get_type (void);
 AtkObject *panel_indicator_accessible_new      (IndicatorObject *indicator);
+IndicatorObject *panel_indicator_accessible_get_indicator (PanelIndicatorAccessible *pia);
 
 G_END_DECLS
 
 
=== modified file 'services/panel-root-accessible.c'
--- services/panel-root-accessible.c	2012-02-25 01:41:29 +0000
+++ services/panel-root-accessible.c	2013-09-26 02:15:48 +0000
@@ -89,23 +89,26 @@
 }
 
 static void
-on_indicator_removed (gpointer *data, GObject *removed_indicator)
+on_indicator_removed_cb (PanelService *panel_service, IndicatorObject *indicator, AtkObject *child)
 {
-  PanelRootAccessible *root = data[0];
-  AtkObject *child = data[1];
-
+  AtkObject *parent;
+  PanelRootAccessible *root;
+
+  if (indicator != panel_indicator_accessible_get_indicator (PANEL_INDICATOR_ACCESSIBLE (child)))
+    return;
+
+  parent = atk_object_get_parent (child);
+  root = PANEL_ROOT_ACCESSIBLE (parent);
   g_return_if_fail (PANEL_IS_ROOT_ACCESSIBLE (root));
-  g_return_if_fail (ATK_IS_OBJECT (child));
 
   gint index = g_slist_index (root->priv->a11y_children, child);
   if (index >= 0)
     {
       root->priv->a11y_children = g_slist_remove (root->priv->a11y_children, child);
       g_signal_emit_by_name (root, "children-changed::remove", index, child);
+      g_signal_handlers_disconnect_by_func (panel_service, on_indicator_removed_cb, child);
       g_object_unref (child);
     }
-
-  g_free (data);
 }
 
 /* Implementation of AtkObject methods */
@@ -133,17 +136,9 @@
       indicator = panel_service_get_indicator_nth (root->priv->service, i);
       if (INDICATOR_IS_OBJECT (indicator))
         {
-          AtkObject *child;
-          gpointer *weak_data;
+          AtkObject *child = panel_indicator_accessible_new (indicator);
 
-          child = panel_indicator_accessible_new (indicator);
-          /* FIXME: use proper signals once we support dynamic adding/removing
-           * of indicators */
-          weak_data = g_new0 (gpointer, 2);
-          weak_data[0] = root;
-          weak_data[1] = child;
-          g_object_weak_ref (G_OBJECT (indicator), 
-              (GWeakNotify) on_indicator_removed, weak_data);
+          g_signal_connect (root->priv->service, "indicator-removed", G_CALLBACK (on_indicator_removed_cb), child);
 
           atk_object_set_parent (child, accessible);
           root->priv->a11y_children = g_slist_append (root->priv->a11y_children, child);
 
=== modified file 'services/panel-service.c'
--- services/panel-service.c	2013-09-24 00:27:02 +0000
+++ services/panel-service.c	2013-09-26 02:15:48 +0000
@@ -92,6 +92,7 @@
   ENTRY_ACTIVATE_REQUEST,
   ENTRY_SHOW_NOW_CHANGED,
   GEOMETRIES_CHANGED,
+  INDICATOR_REMOVED,
   INDICATORS_CLEARED,
 
   LAST_SIGNAL
@@ -270,6 +271,14 @@
                   NULL, NULL, NULL,
                   G_TYPE_NONE, 2, G_TYPE_STRING, G_TYPE_BOOLEAN);
 
+  _service_signals[INDICATOR_REMOVED] =
+    g_signal_new ("indicator-removed",
+                  G_OBJECT_CLASS_TYPE (obj_class),
+                  G_SIGNAL_RUN_LAST,
+                  0,
+                  NULL, NULL, NULL,
+                  G_TYPE_NONE, 1, G_TYPE_OBJECT);
+
   _service_signals[INDICATORS_CLEARED] =
     g_signal_new ("indicators-cleared",
                   G_OBJECT_CLASS_TYPE (obj_class),
@@ -688,11 +697,16 @@
 
   self->priv->indicators = g_slist_remove (self->priv->indicators, indicator);
 
+  indicator_object_set_visible (indicator, FALSE);
+
+  g_signal_emit (self, _service_signals[INDICATOR_REMOVED], 0, indicator);
+
   if (g_object_is_floating (G_OBJECT (indicator)))
     {
       g_object_ref_sink (G_OBJECT (indicator));
     }
 
+  g_debug ("%s unreffing indicator \"%s\" (refcount is %u)", G_STRFUNC, (gchar *)g_object_get_data (G_OBJECT (indicator), "id"), G_OBJECT(indicator)->ref_count);
   g_object_unref (G_OBJECT (indicator));
 }
 
@@ -817,7 +831,6 @@
   g_return_if_fail (PANEL_IS_SERVICE (self));
   g_return_if_fail (INDICATOR_IS_OBJECT (indicator));
 
-  g_object_ref (indicator);
   load_indicator (self, indicator, NULL);
 }
 
@@ -1116,6 +1129,7 @@
     }
   g_list_free (entries);
 
+  g_debug ("%s loaded indicator \"%s\"", G_STRFUNC, name);
   g_free (name);
 }
 
@@ -1142,7 +1156,6 @@
         continue;
 
       path = g_build_filename (INDICATORDIR, name, NULL);
-      g_debug ("Loading: %s", path);
 
       object = indicator_object_new_from_file (path);
       if (object == NULL)