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

Proposed by Stephen M. Webb on 2013-09-26
Status: Needs review
Proposed branch: lp:~bregma/unity/fix-indicator-leaks
Merge into: lp:unity
Diff against target: 164 lines (+35/-20)
5 files modified
services/CMakeLists.txt (+1/-1)
services/panel-indicator-accessible.c (+6/-0)
services/panel-indicator-accessible.h (+1/-0)
services/panel-root-accessible.c (+12/-17)
services/panel-service.c (+15/-2)
To merge this branch: bzr merge lp:~bregma/unity/fix-indicator-leaks
Reviewer Review Type Date Requested Status
Christopher Townsend 2013-09-26 Needs Fixing on 2013-09-26
PS Jenkins bot (community) continuous-integration Needs Fixing on 2013-09-26
Review via email: mp+187636@code.launchpad.net

Commit message

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

Description of the change

= 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.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Christopher Townsend (townsend) wrote :

Getting a segfault in a Panel Service test:

[ RUN ] TestPanelService.ManyEmptyIndicatorObjectsAddition
Segmentation fault

review: Needs Fixing
Marco Trevisan (Treviño) (3v1n0) wrote :

 - 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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'services/CMakeLists.txt'
2--- services/CMakeLists.txt 2013-08-07 16:37:05 +0000
3+++ services/CMakeLists.txt 2013-09-26 02:15:48 +0000
4@@ -38,7 +38,7 @@
5 set(CFLAGS
6 ${SERVICE_DEPS_CFLAGS}
7 ${SERVICE_DEPS_CFLAGS_OTHER}
8- "-Werror -Wall"
9+ "-Werror -Wall -DG_LOG_DOMAIN=\\\"u-p-s\\\""
10 )
11
12 string (REPLACE ";" " " CFLAGS "${CFLAGS}")
13
14=== modified file 'services/panel-indicator-accessible.c'
15--- services/panel-indicator-accessible.c 2011-10-05 16:54:21 +0000
16+++ services/panel-indicator-accessible.c 2013-09-26 02:15:48 +0000
17@@ -384,3 +384,9 @@
18
19 return state_set;
20 }
21+
22+IndicatorObject *
23+panel_indicator_accessible_get_indicator (PanelIndicatorAccessible *pia)
24+{
25+ return pia->priv->indicator;
26+}
27
28=== modified file 'services/panel-indicator-accessible.h'
29--- services/panel-indicator-accessible.h 2011-02-08 11:50:18 +0000
30+++ services/panel-indicator-accessible.h 2013-09-26 02:15:48 +0000
31@@ -49,6 +49,7 @@
32
33 GType panel_indicator_accessible_get_type (void);
34 AtkObject *panel_indicator_accessible_new (IndicatorObject *indicator);
35+IndicatorObject *panel_indicator_accessible_get_indicator (PanelIndicatorAccessible *pia);
36
37 G_END_DECLS
38
39
40=== modified file 'services/panel-root-accessible.c'
41--- services/panel-root-accessible.c 2012-02-25 01:41:29 +0000
42+++ services/panel-root-accessible.c 2013-09-26 02:15:48 +0000
43@@ -89,23 +89,26 @@
44 }
45
46 static void
47-on_indicator_removed (gpointer *data, GObject *removed_indicator)
48+on_indicator_removed_cb (PanelService *panel_service, IndicatorObject *indicator, AtkObject *child)
49 {
50- PanelRootAccessible *root = data[0];
51- AtkObject *child = data[1];
52-
53+ AtkObject *parent;
54+ PanelRootAccessible *root;
55+
56+ if (indicator != panel_indicator_accessible_get_indicator (PANEL_INDICATOR_ACCESSIBLE (child)))
57+ return;
58+
59+ parent = atk_object_get_parent (child);
60+ root = PANEL_ROOT_ACCESSIBLE (parent);
61 g_return_if_fail (PANEL_IS_ROOT_ACCESSIBLE (root));
62- g_return_if_fail (ATK_IS_OBJECT (child));
63
64 gint index = g_slist_index (root->priv->a11y_children, child);
65 if (index >= 0)
66 {
67 root->priv->a11y_children = g_slist_remove (root->priv->a11y_children, child);
68 g_signal_emit_by_name (root, "children-changed::remove", index, child);
69+ g_signal_handlers_disconnect_by_func (panel_service, on_indicator_removed_cb, child);
70 g_object_unref (child);
71 }
72-
73- g_free (data);
74 }
75
76 /* Implementation of AtkObject methods */
77@@ -133,17 +136,9 @@
78 indicator = panel_service_get_indicator_nth (root->priv->service, i);
79 if (INDICATOR_IS_OBJECT (indicator))
80 {
81- AtkObject *child;
82- gpointer *weak_data;
83+ AtkObject *child = panel_indicator_accessible_new (indicator);
84
85- child = panel_indicator_accessible_new (indicator);
86- /* FIXME: use proper signals once we support dynamic adding/removing
87- * of indicators */
88- weak_data = g_new0 (gpointer, 2);
89- weak_data[0] = root;
90- weak_data[1] = child;
91- g_object_weak_ref (G_OBJECT (indicator),
92- (GWeakNotify) on_indicator_removed, weak_data);
93+ g_signal_connect (root->priv->service, "indicator-removed", G_CALLBACK (on_indicator_removed_cb), child);
94
95 atk_object_set_parent (child, accessible);
96 root->priv->a11y_children = g_slist_append (root->priv->a11y_children, child);
97
98=== modified file 'services/panel-service.c'
99--- services/panel-service.c 2013-09-24 00:27:02 +0000
100+++ services/panel-service.c 2013-09-26 02:15:48 +0000
101@@ -92,6 +92,7 @@
102 ENTRY_ACTIVATE_REQUEST,
103 ENTRY_SHOW_NOW_CHANGED,
104 GEOMETRIES_CHANGED,
105+ INDICATOR_REMOVED,
106 INDICATORS_CLEARED,
107
108 LAST_SIGNAL
109@@ -270,6 +271,14 @@
110 NULL, NULL, NULL,
111 G_TYPE_NONE, 2, G_TYPE_STRING, G_TYPE_BOOLEAN);
112
113+ _service_signals[INDICATOR_REMOVED] =
114+ g_signal_new ("indicator-removed",
115+ G_OBJECT_CLASS_TYPE (obj_class),
116+ G_SIGNAL_RUN_LAST,
117+ 0,
118+ NULL, NULL, NULL,
119+ G_TYPE_NONE, 1, G_TYPE_OBJECT);
120+
121 _service_signals[INDICATORS_CLEARED] =
122 g_signal_new ("indicators-cleared",
123 G_OBJECT_CLASS_TYPE (obj_class),
124@@ -688,11 +697,16 @@
125
126 self->priv->indicators = g_slist_remove (self->priv->indicators, indicator);
127
128+ indicator_object_set_visible (indicator, FALSE);
129+
130+ g_signal_emit (self, _service_signals[INDICATOR_REMOVED], 0, indicator);
131+
132 if (g_object_is_floating (G_OBJECT (indicator)))
133 {
134 g_object_ref_sink (G_OBJECT (indicator));
135 }
136
137+ 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);
138 g_object_unref (G_OBJECT (indicator));
139 }
140
141@@ -817,7 +831,6 @@
142 g_return_if_fail (PANEL_IS_SERVICE (self));
143 g_return_if_fail (INDICATOR_IS_OBJECT (indicator));
144
145- g_object_ref (indicator);
146 load_indicator (self, indicator, NULL);
147 }
148
149@@ -1116,6 +1129,7 @@
150 }
151 g_list_free (entries);
152
153+ g_debug ("%s loaded indicator \"%s\"", G_STRFUNC, name);
154 g_free (name);
155 }
156
157@@ -1142,7 +1156,6 @@
158 continue;
159
160 path = g_build_filename (INDICATORDIR, name, NULL);
161- g_debug ("Loading: %s", path);
162
163 object = indicator_object_new_from_file (path);
164 if (object == NULL)