After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 752938 - Leak fixes for telepathy-accounts-widgets
Leak fixes for telepathy-accounts-widgets
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: tp-aw
unspecified
Other All
: Normal normal
: ---
Assigned To: empathy-maint
empathy-maint
Depends on:
Blocks:
 
 
Reported: 2015-07-27 21:45 UTC by Christophe Fergeau
Modified: 2015-07-31 19:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Free TpawProtocol::service_name in finalize() (837 bytes, patch)
2015-07-27 21:47 UTC, Christophe Fergeau
committed Details | Review
Free GList returned by tp_list_connection_managers_finish() (921 bytes, patch)
2015-07-27 21:47 UTC, Christophe Fergeau
committed Details | Review
Fix GetProtocolsData leak (2.00 KB, patch)
2015-07-27 21:47 UTC, Christophe Fergeau
committed Details | Review
Properly fix tp_list_connection_managers_finish() leak (3.76 KB, patch)
2015-07-30 09:44 UTC, Christophe Fergeau
committed Details | Review
account-widget: Fix 'default_display_name' leak (2.10 KB, patch)
2015-07-30 09:44 UTC, Christophe Fergeau
committed Details | Review
account-widget: Fix 'login_id' leak in _get_default_display_name() (3.73 KB, patch)
2015-07-30 09:44 UTC, Christophe Fergeau
committed Details | Review
account-settings: Always take ownership of variant in tpaw_account_settings_set (2.61 KB, patch)
2015-07-30 09:44 UTC, Christophe Fergeau
needs-work Details | Review
avatar-chooser: Don't call clear_image() from ::init() (4.21 KB, patch)
2015-07-30 09:45 UTC, Christophe Fergeau
needs-work Details | Review
account-settings: Fix 'account_req' leak (3.89 KB, patch)
2015-07-30 09:45 UTC, Christophe Fergeau
committed Details | Review
account-settings: Fix GSimpleAsyncResult leaks (1.58 KB, patch)
2015-07-30 09:45 UTC, Christophe Fergeau
committed Details | Review
avatar-chooser: Don't call clear_image() from ::init() (4.42 KB, patch)
2015-07-30 19:35 UTC, Christophe Fergeau
committed Details | Review
account-settings: Always take ownership of variant in ::set() (3.18 KB, patch)
2015-07-30 19:38 UTC, Christophe Fergeau
committed Details | Review

Description Christophe Fergeau 2015-07-27 21:45:56 UTC
Here are 3 patches fixing small leaks I found in telepathy-accounts-manager
when running gnome-online-accounts in valgrind.
Comment 1 Christophe Fergeau 2015-07-27 21:47:38 UTC
Created attachment 308250 [details] [review]
Free TpawProtocol::service_name in finalize()

It will be leaked otherwise.
Comment 2 Christophe Fergeau 2015-07-27 21:47:44 UTC
Created attachment 308251 [details] [review]
Free GList returned by tp_list_connection_managers_finish()

Make sure it's not leaked in tpaw_connection_managers_listed_cb()
Comment 3 Christophe Fergeau 2015-07-27 21:47:49 UTC
Created attachment 308252 [details] [review]
Fix GetProtocolsData leak

A GetProtocolsData instance gets associated with a GSimpleAsyncResult in
tpaw_protocol_get_all_async() using
g_simple_async_result_set_op_res_gpointer(). A GDestroyNotify is set so
that the GetProtocolsData instance is freed when the GSimpleAsyncResult
is freed.
However, this is not working as expected as the GetProtocolsData
instance is keeping a reference on the GSimpleAsyncResult it's
associated with, which creates a circular reference and causes the
GetProtocolsData instance to never be freed.

This commit drops the reference on the GSimpleAsyncResult instance owned
by the GetProtocolsData instance as soon as it's no longer needed so
that the GSimpleAsyncResult can be disposed, which triggers the
destruction of the GetProtocolsData too.
Comment 4 Debarshi Ray 2015-07-28 15:57:39 UTC
Review of attachment 308250 [details] [review]:

++
Comment 5 Debarshi Ray 2015-07-28 16:12:33 UTC
Review of attachment 308251 [details] [review]:

Thanks, Christophe. One small issue:

::: tp-account-widgets/tpaw-connection-managers.c
@@ +224,3 @@
             g_object_ref (cm));
     }
+  g_list_free (cms);

We need to unref the elements within the list too - either by stealing the references in the loop or by using g_list_free_full. I prefer the later because it is more explicit.
Comment 6 Debarshi Ray 2015-07-28 16:28:45 UTC
Review of attachment 308252 [details] [review]:

::: tp-account-widgets/tpaw-protocol.c
@@ +486,3 @@
       g_simple_async_result_take_error (data->result, error);
       g_simple_async_result_complete_in_idle (data->result);
+      g_object_unref (data->result);

I would have used a 'goto out', but whatever.
Comment 7 Debarshi Ray 2015-07-28 16:32:06 UTC
Could you please update the telepathy-account-widgets submodules in empathy and gnome-online-accounts once the patches have landed?
Comment 8 Christophe Fergeau 2015-07-28 20:43:26 UTC
Attachment 308250 [details] pushed as 40982ee - Free TpawProtocol::service_name in finalize()
Attachment 308251 [details] pushed as 103ed62 - Free GList returned by tp_list_connection_managers_finish()
Attachment 308252 [details] pushed as 6a4a30f - Fix GetProtocolsData leak
Comment 9 Christophe Fergeau 2015-07-29 07:35:27 UTC
Review of attachment 308251 [details] [review]:

Grmble, I only saw that review comment hours after pushing all 3 patches (including that one), sorry about that. I'll send a follow up patch to address this.
Comment 10 Debarshi Ray 2015-07-29 16:06:16 UTC
(In reply to Christophe Fergeau from comment #9)

> Grmble, I only saw that review comment hours after pushing all 3 patches
> (including that one), sorry about that. I'll send a follow up patch to
> address this.

That's ok.
Comment 11 Christophe Fergeau 2015-07-30 09:44:35 UTC
Created attachment 308440 [details] [review]
Properly fix tp_list_connection_managers_finish() leak

Commit 103ed6232 attempted to fix a leak in tp_list_connection_managers_finish() but it was
incomplete as it only freed the returned GList with g_list_free() while
the list elements must be freed too. This commit addresses that using
g_list_free_full()

==9013==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==9013==    by 0x7F150AC: g_malloc (gmem.c:97)
==9013==    by 0x7F2E0F7: g_slice_alloc (gslice.c:1007)
==9013==    by 0x7F091B7: g_list_copy_deep (glist.c:678)
==9013==    by 0x7F09184: g_list_copy (glist.c:633)
==9013==    by 0x370B157A64: _tp_g_list_copy_deep (util.c:2117)
==9013==    by 0x4FB06C5: tpaw_connection_managers_listed_cb (tpaw-connection-managers.c:209)
==9013==    by 0x7929107: g_simple_async_result_complete (gsimpleasyncresult.c:801)
==9013==    by 0x7929153: complete_in_idle_cb (gsimpleasyncresult.c:813)
==9013==    by 0x7F0F470: g_idle_dispatch (gmain.c:5397)
==9013==    by 0x7F0CAB8: g_main_dispatch (gmain.c:3122)
==9013==    by 0x7F0D8FC: g_main_context_dispatch (gmain.c:3737)
==9013==    by 0x7F0DAE0: g_main_context_iterate (gmain.c:3808)
==9013==    by 0x7F0DF06: g_main_loop_run (gmain.c:4002)
==9013==    by 0x4F803F3: wait_for_account_settings_ready (goatelepathyprovider.c:102)
==9013==    by 0x4F8127F: edit_connection_parameters (goatelepathyprovider.c:534)
==9013==    by 0x4F81F16: edit_parameters_clicked_cb (goatelepathyprovider.c:835)
==9013==    by 0x7C7727D: g_cclosure_marshal_VOID__VOIDv (gmarshal.c:905)
==9013==    by 0x7C744D1: _g_closure_invoke_va (gclosure.c:864)
==9013==    by 0x7C8F771: g_signal_emit_valist (gsignal.c:3246)
==9013==    by 0x7C908E9: g_signal_emit (gsignal.c:3393)
==9013==    by 0x6838A45: gtk_button_clicked (gtkbutton.c:1488)
==9013==    by 0x6839966: gtk_button_do_release (gtkbutton.c:1899)
==9013==    by 0x6839D45: gtk_real_button_released (gtkbutton.c:2017)
==9013==    by 0x7C7727D: g_cclosure_marshal_VOID__VOIDv (gmarshal.c:905)
==9013==    by 0x7C7490F: g_type_class_meta_marshalv (gclosure.c:1021)
==9013==    by 0x7C744D1: _g_closure_invoke_va (gclosure.c:864)
==9013==    by 0x7C8F771: g_signal_emit_valist (gsignal.c:3246)
==9013==    by 0x7C908E9: g_signal_emit (gsignal.c:3393)
==9013==    by 0x68364F5: multipress_released_cb (gtkbutton.c:613)
==9013==    by 0x36E5A05DAF: ffi_call_unix64 (unix64.S:76)
==9013==    by 0x36E5A05817: ffi_call (ffi64.c:525)
==9013==    by 0x7C75F7C: g_cclosure_marshal_generic_va (gclosure.c:1594)
==9013==    by 0x7C744D1: _g_closure_invoke_va (gclosure.c:864)
==9013==    by 0x7C8F771: g_signal_emit_valist (gsignal.c:3246)
==9013==    by 0x7C908E9: g_signal_emit (gsignal.c:3393)
==9013==    by 0x69303DB: gtk_gesture_multi_press_end (gtkgesturemultipress.c:273)
==9013==    by 0x7C78B90: g_cclosure_marshal_VOID__BOXEDv (gmarshal.c:1950)
==9013==    by 0x7C7490F: g_type_class_meta_marshalv (gclosure.c:1021)
==9013==    by 0x7C744D1: _g_closure_invoke_va (gclosure.c:864)
Comment 12 Christophe Fergeau 2015-07-30 09:44:43 UTC
Created attachment 308441 [details] [review]
account-widget: Fix 'default_display_name' leak

do_constructed() leaks the string returned by
tpaw_account_widget_get_default_display_name():

==15713== 21 bytes in 1 blocks are definitely lost in loss record 3,415 of 16,664
==15713==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==15713==    by 0x36E2E76B57: vasprintf (vasprintf.c:76)
==15713==    by 0x7F60DD2: g_vasprintf (gprintf.c:316)
==15713==    by 0x7F30D70: g_strdup_vprintf (gstrfuncs.c:507)
==15713==    by 0x7F30E15: g_strdup_printf (gstrfuncs.c:533)
==15713==    by 0x4FAAD98: tpaw_account_widget_get_default_display_name (tpaw-account-widget.c:2192)
==15713==    by 0x4FAA444: do_constructed (tpaw-account-widget.c:1960)
==15713==    by 0x7C7CC10: g_object_new_internal (gobject.c:1814)
==15713==    by 0x7C7D71A: g_object_new_valist (gobject.c:2033)
==15713==    by 0x7C7C6C5: g_object_new (gobject.c:1617)
==15713==    by 0x4FAABCC: tpaw_account_widget_new_for_protocol (tpaw-account-widget.c:2127)
==15713==    by 0x4F81352: edit_connection_parameters (goatelepathyprovider.c:544)
Comment 13 Christophe Fergeau 2015-07-30 09:44:49 UTC
Created attachment 308442 [details] [review]
account-widget: Fix 'login_id' leak in _get_default_display_name()

tpaw_account_get_default_display_name() returns early when login_id is non-NULL, and
only tries to g_free() it in the codepath after this early return, which causes a leak.
This commit moves the call to g_free (login_id) before the early return in the
(login_id != NULL) case. This fixes:

==26855== 8 bytes in 1 blocks are definitely lost in loss record 13,180 of 34,041
==26855==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==26855==    by 0x8A120AC: g_malloc (gmem.c:97)
==26855==    by 0x8A123C5: g_malloc_n (gmem.c:336)
==26855==    by 0x8A2DB9A: g_strdup (gstrfuncs.c:356)
==26855==    by 0x8A4C05B: g_variant_dup_string (gvariant.c:1500)
==26855==    by 0x4CB9B41: tpaw_account_settings_dup_string (tpaw-account-settings.c:843)
==26855==    by 0x4CC0F8B: tpaw_account_widget_get_default_display_name (tpaw-account-widget.c:2155)
==26855==    by 0x4CBDFE2: tpaw_account_widget_apply_and_log_in (tpaw-account-widget.c:999)
==26855==    by 0x4CBE05C: account_widget_apply_clicked_cb (tpaw-account-widget.c:1017)
==26855==    by 0x87741EA: g_cclosure_marshal_VOID__VOID (gmarshal.c:875)
==26855==    by 0x8771238: g_closure_invoke (gclosure.c:801)
==26855==    by 0x878E070: signal_emit_unlocked_R (gsignal.c:3581)
==26855==    by 0x878D3A7: g_signal_emit_valist (gsignal.c:3337)
==26855==    by 0x878D8E9: g_signal_emit (gsignal.c:3393)
==26855==    by 0x7326A22: gtk_button_clicked (gtkbutton.c:1488)
==26855==    by 0x7327943: gtk_button_do_release (gtkbutton.c:1899)
==26855==    by 0x7327D22: gtk_real_button_released (gtkbutton.c:2017)
==26855==    by 0x877427D: g_cclosure_marshal_VOID__VOIDv (gmarshal.c:905)
==26855==    by 0x877190F: g_type_class_meta_marshalv (gclosure.c:1021)
==26855==    by 0x87714D1: _g_closure_invoke_va (gclosure.c:864)
==26855==    by 0x878C771: g_signal_emit_valist (gsignal.c:3246)
==26855==    by 0x878D8E9: g_signal_emit (gsignal.c:3393)
==26855==    by 0x73244D2: multipress_released_cb (gtkbutton.c:613)
==26855==    by 0x36E5A05DAF: ffi_call_unix64 (unix64.S:76)
==26855==    by 0x36E5A05817: ffi_call (ffi64.c:525)
==26855==    by 0x8772F7C: g_cclosure_marshal_generic_va (gclosure.c:1594)
==26855==    by 0x87714D1: _g_closure_invoke_va (gclosure.c:864)
==26855==    by 0x878C771: g_signal_emit_valist (gsignal.c:3246)
==26855==    by 0x878D8E9: g_signal_emit (gsignal.c:3393)
==26855==    by 0x7421792: gtk_gesture_multi_press_end (gtkgesturemultipress.c:273)
==26855==    by 0x8775B90: g_cclosure_marshal_VOID__BOXEDv (gmarshal.c:1950)
==26855==    by 0x877190F: g_type_class_meta_marshalv (gclosure.c:1021)
==26855==    by 0x87714D1: _g_closure_invoke_va (gclosure.c:864)
==26855==    by 0x878C771: g_signal_emit_valist (gsignal.c:3246)
Comment 14 Christophe Fergeau 2015-07-30 09:44:56 UTC
Created attachment 308443 [details] [review]
account-settings: Always take ownership of variant in tpaw_account_settings_set

In one codepath, tpaw_account_settings_set() takes ownership of the
passed-in GVariant through g_variant_ref_sink, but in the other
codepath, ownership is left to the caller, which is unusual with
functions with GVariant parameters.
This commit makes sure we unref() the passed in GVariant when we don't
want to keep it around so that the caller gets the expected behaviour,
and so that we don't leak memory.
This fixes
==26855== 507 (240 direct, 267 indirect) bytes in 6 blocks are definitely lost in loss record 32,940 of 3
==26855==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==26855==    by 0x8A120AC: g_malloc (gmem.c:97)
==26855==    by 0x8A2B0F7: g_slice_alloc (gslice.c:1007)
==26855==    by 0x8A52C0B: g_variant_alloc (gvariant-core.c:476)
==26855==    by 0x8A52C9B: g_variant_new_from_bytes (gvariant-core.c:512)
==26855==    by 0x8A4A60F: g_variant_new_from_trusted (gvariant.c:295)
==26855==    by 0x8A4BAF6: g_variant_new_string (gvariant.c:1232)
==26855==    by 0x4CBBE2E: account_widget_entry_changed_common (tpaw-account-widget.c:281)
==26855==    by 0x4CBBED8: account_widget_entry_changed_cb (tpaw-account-widget.c:299)
==26855==    by 0x87741EA: g_cclosure_marshal_VOID__VOID (gmarshal.c:875)
==26855==    by 0x8771238: g_closure_invoke (gclosure.c:801)
==26855==    by 0x878E070: signal_emit_unlocked_R (gsignal.c:3581)
==26855==    by 0x878D3A7: g_signal_emit_valist (gsignal.c:3337)
==26855==    by 0x878DA56: g_signal_emit_by_name (gsignal.c:3433)
==26855==    by 0x73C0612: end_change (gtkentry.c:2928)
==26855==    by 0x73C5F0D: gtk_entry_real_insert_text (gtkentry.c:5360)
==26855==    by 0x7495B9A: _gtk_marshal_VOID__STRING_INT_POINTER (gtkmarshalers.c:7351)
==26855==    by 0x87719A3: g_type_iface_meta_marshal (gclosure.c:1045)
=
Comment 15 Christophe Fergeau 2015-07-30 09:45:03 UTC
Created attachment 308444 [details] [review]
avatar-chooser: Don't call clear_image() from ::init()

GtkButton currently has a bug where calling gtk_button_set_image() in
the ::init() vfunc of a subclass will cause the GtkImage to be leaked.
This bug does not happen if the image is set after the GtkButton has
been constructed, so this commit moves the call to
avatar_chooser_clear_image() (which calls gtk_button_set_image()) from
::init() to ::constructed().

See https://bugzilla.gnome.org/show_bug.cgi?id=753048

This fixes:

==388== 768 (304 direct, 464 indirect) bytes in 1 blocks are definitely lost in loss record 33,655 of 34,
==388==    at 0x8792D9C: g_type_create_instance (gtype.c:1849)
==388==    by 0x8779AC5: g_object_new_internal (gobject.c:1774)
==388==    by 0x8779FD4: g_object_newv (gobject.c:1921)
==388==    by 0x8779676: g_object_new (gobject.c:1614)
==388==    by 0x7453142: gtk_image_new_from_icon_name (gtkimage.c:792)
==388==    by 0x4CC2CEE: avatar_chooser_clear_image (tpaw-avatar-chooser.c:394)
==388==    by 0x4CC45AA: tpaw_avatar_chooser_init (tpaw-avatar-chooser.c:1195)
==388==    by 0x8792E7D: g_type_create_instance (gtype.c:1870)
==388==    by 0x8779AC5: g_object_new_internal (gobject.c:1774)
==388==    by 0x877A71A: g_object_new_valist (gobject.c:2033)
==388==    by 0x87796C5: g_object_new (gobject.c:1617)
==388==    by 0x4CC4673: tpaw_avatar_chooser_new (tpaw-avatar-chooser.c:1213)
==388==    by 0x4CD41D2: tpaw_user_info_constructed (tpaw-user-info.c:543)
==388==    by 0x8779C10: g_object_new_internal (gobject.c:1814)
==388==    by 0x877A71A: g_object_new_valist (gobject.c:2033)
==388==    by 0x87796C5: g_object_new (gobject.c:1617)
==388==    by 0x4CD4826: tpaw_user_info_new (tpaw-user-info.c:663)
==388==    by 0x4C97906: edit_personal_details (goatelepathyprovider.c:667)
==388==    by 0x4C980A7: edit_personal_details_clicked_cb (goatelepathyprovider.c:859)
==388==    by 0x877427D: g_cclosure_marshal_VOID__VOIDv (gmarshal.c:905)
==388==    by 0x87714D1: _g_closure_invoke_va (gclosure.c:864)
==388==    by 0x878C771: g_signal_emit_valist (gsignal.c:3246)
==388==    by 0x878D8E9: g_signal_emit (gsignal.c:3393)
==388==    by 0x7326A22: gtk_button_clicked (gtkbutton.c:1488)
==388==    by 0x7327943: gtk_button_do_release (gtkbutton.c:1899)
==388==    by 0x7327D22: gtk_real_button_released (gtkbutton.c:2017)
==388==    by 0x877427D: g_cclosure_marshal_VOID__VOIDv (gmarshal.c:905)
==388==    by 0x877190F: g_type_class_meta_marshalv (gclosure.c:1021)
==388==    by 0x87714D1: _g_closure_invoke_va (gclosure.c:864)
==388==    by 0x878C771: g_signal_emit_valist (gsignal.c:3246)
==388==    by 0x878D8E9: g_signal_emit (gsignal.c:3393)
==388==    by 0x73244D2: multipress_released_cb (gtkbutton.c:613)
==388==    by 0x36E5A05DAF: ffi_call_unix64 (unix64.S:76)
==388==    by 0x36E5A05817: ffi_call (ffi64.c:525)
==388==    by 0x8772F7C: g_cclosure_marshal_generic_va (gclosure.c:1594)
==388==    by 0x87714D1: _g_closure_invoke_va (gclosure.c:864)
==388==    by 0x878C771: g_signal_emit_valist (gsignal.c:3246)
==388==    by 0x878D8E9: g_signal_emit (gsignal.c:3393)
==388==    by 0x7421792: gtk_gesture_multi_press_end (gtkgesturemultipress.c:273)
==388==    by 0x8775B90: g_cclosure_marshal_VOID__BOXEDv (gmarshal.c:1950)
=
Comment 16 Christophe Fergeau 2015-07-30 09:45:10 UTC
Created attachment 308445 [details] [review]
account-settings: Fix 'account_req' leak

tpaw_account_settings_do_create_account() creates a TpAccountRequest
using tp_account_request_new() but never release its ref when it no
longer needs it. Adding the missing unref() fixes the following leak:

==21491== 848 (40 direct, 808 indirect) bytes in 1 blocks are definitely lost in loss record 18,879 of 19,643
==21491==    at 0x8792D3A: g_type_create_instance (gtype.c:1848)
==21491==    by 0x8779AC5: g_object_new_internal (gobject.c:1774)
==21491==    by 0x877A71A: g_object_new_valist (gobject.c:2033)
==21491==    by 0x87796C5: g_object_new (gobject.c:1617)
==21491==    by 0x567A173: tp_account_request_new (account-request.c:750)
==21491==    by 0x4CBAF7F: tpaw_account_settings_do_create_account (tpaw-account-settings.c:1382)
==21491==    by 0x4CBB270: tpaw_account_settings_apply_async (tpaw-account-settings.c:1464)
==21491==    by 0x4CBE023: tpaw_account_widget_apply_and_log_in (tpaw-account-widget.c:1009)
==21491==    by 0x4CBE042: account_widget_apply_clicked_cb (tpaw-account-widget.c:1017)
==21491==    by 0x87741EA: g_cclosure_marshal_VOID__VOID (gmarshal.c:875)
==21491==    by 0x8771238: g_closure_invoke (gclosure.c:801)
==21491==    by 0x878E070: signal_emit_unlocked_R (gsignal.c:3581)
==21491==    by 0x878D3A7: g_signal_emit_valist (gsignal.c:3337)
==21491==    by 0x878D8E9: g_signal_emit (gsignal.c:3393)
==21491==    by 0x7326A22: gtk_button_clicked (gtkbutton.c:1488)
==21491==    by 0x7327943: gtk_button_do_release (gtkbutton.c:1899)
==21491==    by 0x7327D22: gtk_real_button_released (gtkbutton.c:2017)
==21491==    by 0x877427D: g_cclosure_marshal_VOID__VOIDv (gmarshal.c:905)
==21491==    by 0x877190F: g_type_class_meta_marshalv (gclosure.c:1021)
==21491==    by 0x87714D1: _g_closure_invoke_va (gclosure.c:864)
==21491==    by 0x878C771: g_signal_emit_valist (gsignal.c:3246)
==21491==    by 0x878D8E9: g_signal_emit (gsignal.c:3393)
==21491==    by 0x73244D2: multipress_released_cb (gtkbutton.c:613)
==21491==    by 0x36E5A05DAF: ffi_call_unix64 (unix64.S:76)
==21491==    by 0x36E5A05817: ffi_call (ffi64.c:525)
==21491==    by 0x8772F7C: g_cclosure_marshal_generic_va (gclosure.c:1594)
==21491==    by 0x87714D1: _g_closure_invoke_va (gclosure.c:864)
==21491==    by 0x878C771: g_signal_emit_valist (gsignal.c:3246)
==21491==    by 0x878D8E9: g_signal_emit (gsignal.c:3393)
==21491==    by 0x7421792: gtk_gesture_multi_press_end (gtkgesturemultipress.c:273)
==21491==    by 0x8775B90: g_cclosure_marshal_VOID__BOXEDv (gmarshal.c:1950)
==21491==    by 0x877190F: g_type_class_meta_marshalv (gclosure.c:1021)
==21491==    by 0x87714D1: _g_closure_invoke_va (gclosure.c:864)
==21491==    by 0x878C771: g_signal_emit_valist (gsignal.c:3246)
==21491==    by 0x878D8E9: g_signal_emit (gsignal.c:3393)
==21491==    by 0x741D17F: _gtk_gesture_set_recognized (gtkgesture.c:275)
==21491==    by 0x741D28D: _gtk_gesture_check_recognized (gtkgesture.c:315)
==21491==    by 0x741DBD6: gtk_gesture_handle_event (gtkgesture.c:623)
==21491==    by 0x7423555: gtk_gesture_single_handle_event (gtkgesturesingle.c:220)
==21491==    by 0x73DBD1E: gtk_event_controller_handle_event (gtkeventcontroller.c:224)
=
Comment 17 Christophe Fergeau 2015-07-30 09:45:16 UTC
Created attachment 308446 [details] [review]
account-settings: Fix GSimpleAsyncResult leaks

tpaw_account_setting_set_display_name_async() and
tpaw_account_settings_set_icon_name_async() can return immediatly in an
idle when they don't have work to do. However in that case, these
methods don't drop the reference they own on the GSimpleAsyncResult used
for the async operation. This commit adds the missing g_object_unref()
calls.
Comment 18 Debarshi Ray 2015-07-30 11:25:27 UTC
Review of attachment 308440 [details] [review]:

++
Comment 19 Debarshi Ray 2015-07-30 11:30:54 UTC
Review of attachment 308441 [details] [review]:

++
Comment 20 Debarshi Ray 2015-07-30 11:34:36 UTC
Review of attachment 308442 [details] [review]:

++
Comment 21 Debarshi Ray 2015-07-30 11:59:46 UTC
Review of attachment 308443 [details] [review]:

Thanks for catching this, Christophe.

Nitpick: the subject of the commit message exceeds 72 characters. Maybe consider dropping the prefix.

::: tp-account-widgets/tpaw-account-settings.c
@@ +1033,3 @@
       g_free (settings->priv->password);
       settings->priv->password = g_variant_dup_string (v, NULL);
+      g_variant_unref (v);

I think we should only unref it if the GVariant is floating. Otherwise, we would be trespassing if the caller had explicitly kept a reference to the GVariant for its own use. So, we either need a g_variant_is_floating check, or we need to add a g_variant_ref_sink at the beginning of the branch.
Comment 22 Debarshi Ray 2015-07-30 12:34:52 UTC
Review of attachment 308444 [details] [review]:

Although it smells like a workaround for the gtk+ leak, it doesn't hurt to make the code robust across different gtk+ versions.

::: tp-account-widgets/tpaw-avatar-chooser.c
@@ +187,3 @@
   G_OBJECT_CLASS (tpaw_avatar_chooser_parent_class)->constructed (object);
 
+  avatar_chooser_clear_image (self);

Could you please add a comment pointing to the gtk+ bug?

Otherwise, it is too innocuous for someone to realize the importance of keeping this statement here. I, myself, wouldn't think of checking the git history before touching this line. :)
Comment 23 Debarshi Ray 2015-07-30 12:48:15 UTC
Review of attachment 308445 [details] [review]:

++
Comment 24 Debarshi Ray 2015-07-30 12:50:16 UTC
Review of attachment 308446 [details] [review]:

++
Comment 25 Debarshi Ray 2015-07-30 12:50:53 UTC
Thanks for tracking down these leaks, Christophe.
Comment 26 Christophe Fergeau 2015-07-30 19:15:47 UTC
(In reply to Debarshi Ray from comment #25)
> Thanks for tracking down these leaks, Christophe.

I'm just the messenger, the guy who did most of the work is called Valgrind ;)
Comment 27 Christophe Fergeau 2015-07-30 19:32:44 UTC
Review of attachment 308444 [details] [review]:

Yes it's definitely a workaround for that gtk+ bug, but it's simple enough that it doesn't hurt to have it, especially as it avoids head scratching when running in valgrind ;)

::: tp-account-widgets/tpaw-avatar-chooser.c
@@ +187,3 @@
   G_OBJECT_CLASS (tpaw_avatar_chooser_parent_class)->constructed (object);
 
+  avatar_chooser_clear_image (self);

Good point, added now.
Comment 28 Christophe Fergeau 2015-07-30 19:35:26 UTC
Created attachment 308484 [details] [review]
avatar-chooser: Don't call clear_image() from ::init()

GtkButton currently has a bug where calling gtk_button_set_image() in
the ::init() vfunc of a subclass will cause the GtkImage to be leaked.
This bug does not happen if the image is set after the GtkButton has
been constructed, so this commit moves the call to
avatar_chooser_clear_image() (which calls gtk_button_set_image()) from
::init() to ::constructed().

See https://bugzilla.gnome.org/show_bug.cgi?id=753048

This fixes:

==388== 768 (304 direct, 464 indirect) bytes in 1 blocks are definitely lost in loss record 33,655 of 34,
==388==    at 0x8792D9C: g_type_create_instance (gtype.c:1849)
==388==    by 0x8779AC5: g_object_new_internal (gobject.c:1774)
==388==    by 0x8779FD4: g_object_newv (gobject.c:1921)
==388==    by 0x8779676: g_object_new (gobject.c:1614)
==388==    by 0x7453142: gtk_image_new_from_icon_name (gtkimage.c:792)
==388==    by 0x4CC2CEE: avatar_chooser_clear_image (tpaw-avatar-chooser.c:394)
==388==    by 0x4CC45AA: tpaw_avatar_chooser_init (tpaw-avatar-chooser.c:1195)
==388==    by 0x8792E7D: g_type_create_instance (gtype.c:1870)
==388==    by 0x8779AC5: g_object_new_internal (gobject.c:1774)
==388==    by 0x877A71A: g_object_new_valist (gobject.c:2033)
==388==    by 0x87796C5: g_object_new (gobject.c:1617)
==388==    by 0x4CC4673: tpaw_avatar_chooser_new (tpaw-avatar-chooser.c:1213)
==388==    by 0x4CD41D2: tpaw_user_info_constructed (tpaw-user-info.c:543)
==388==    by 0x8779C10: g_object_new_internal (gobject.c:1814)
==388==    by 0x877A71A: g_object_new_valist (gobject.c:2033)
==388==    by 0x87796C5: g_object_new (gobject.c:1617)
==388==    by 0x4CD4826: tpaw_user_info_new (tpaw-user-info.c:663)
==388==    by 0x4C97906: edit_personal_details (goatelepathyprovider.c:667)
==388==    by 0x4C980A7: edit_personal_details_clicked_cb (goatelepathyprovider.c:859)
==388==    by 0x877427D: g_cclosure_marshal_VOID__VOIDv (gmarshal.c:905)
==388==    by 0x87714D1: _g_closure_invoke_va (gclosure.c:864)
==388==    by 0x878C771: g_signal_emit_valist (gsignal.c:3246)
==388==    by 0x878D8E9: g_signal_emit (gsignal.c:3393)
==388==    by 0x7326A22: gtk_button_clicked (gtkbutton.c:1488)
==388==    by 0x7327943: gtk_button_do_release (gtkbutton.c:1899)
==388==    by 0x7327D22: gtk_real_button_released (gtkbutton.c:2017)
==388==    by 0x877427D: g_cclosure_marshal_VOID__VOIDv (gmarshal.c:905)
==388==    by 0x877190F: g_type_class_meta_marshalv (gclosure.c:1021)
==388==    by 0x87714D1: _g_closure_invoke_va (gclosure.c:864)
==388==    by 0x878C771: g_signal_emit_valist (gsignal.c:3246)
==388==    by 0x878D8E9: g_signal_emit (gsignal.c:3393)
==388==    by 0x73244D2: multipress_released_cb (gtkbutton.c:613)
==388==    by 0x36E5A05DAF: ffi_call_unix64 (unix64.S:76)
==388==    by 0x36E5A05817: ffi_call (ffi64.c:525)
==388==    by 0x8772F7C: g_cclosure_marshal_generic_va (gclosure.c:1594)
==388==    by 0x87714D1: _g_closure_invoke_va (gclosure.c:864)
==388==    by 0x878C771: g_signal_emit_valist (gsignal.c:3246)
==388==    by 0x878D8E9: g_signal_emit (gsignal.c:3393)
==388==    by 0x7421792: gtk_gesture_multi_press_end (gtkgesturemultipress.c:273)
==388==    by 0x8775B90: g_cclosure_marshal_VOID__BOXEDv (gmarshal.c:1950)
Comment 29 Christophe Fergeau 2015-07-30 19:38:58 UTC
Created attachment 308485 [details] [review]
account-settings: Always take ownership of variant in ::set()

In one codepath, tpaw_account_settings_set() takes ownership of the
passed-in GVariant through g_variant_ref_sink, but in the other
codepath, ownership is left to the caller, which is unusual with
functions with GVariant parameters.

This commit unconditionnally calls g_variant_sink_ref() on the GVariant
arg rather than doing it in just one codepath, and then makes sure we
unref() the passed in GVariant when we don't want to keep it around.
This fixes a memory leak in tpaw_account_settings_set()

This fixes
==26855== 507 (240 direct, 267 indirect) bytes in 6 blocks are definitely lost in loss record 32,940 of 3
==26855==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==26855==    by 0x8A120AC: g_malloc (gmem.c:97)
==26855==    by 0x8A2B0F7: g_slice_alloc (gslice.c:1007)
==26855==    by 0x8A52C0B: g_variant_alloc (gvariant-core.c:476)
==26855==    by 0x8A52C9B: g_variant_new_from_bytes (gvariant-core.c:512)
==26855==    by 0x8A4A60F: g_variant_new_from_trusted (gvariant.c:295)
==26855==    by 0x8A4BAF6: g_variant_new_string (gvariant.c:1232)
==26855==    by 0x4CBBE2E: account_widget_entry_changed_common (tpaw-account-widget.c:281)
==26855==    by 0x4CBBED8: account_widget_entry_changed_cb (tpaw-account-widget.c:299)
==26855==    by 0x87741EA: g_cclosure_marshal_VOID__VOID (gmarshal.c:875)
==26855==    by 0x8771238: g_closure_invoke (gclosure.c:801)
==26855==    by 0x878E070: signal_emit_unlocked_R (gsignal.c:3581)
==26855==    by 0x878D3A7: g_signal_emit_valist (gsignal.c:3337)
==26855==    by 0x878DA56: g_signal_emit_by_name (gsignal.c:3433)
==26855==    by 0x73C0612: end_change (gtkentry.c:2928)
==26855==    by 0x73C5F0D: gtk_entry_real_insert_text (gtkentry.c:5360)
==26855==    by 0x7495B9A: _gtk_marshal_VOID__STRING_INT_POINTER (gtkmarshalers.c:7351)
==26855==    by 0x87719A3: g_type_iface_meta_marshal (gclosure.c:1045)
Comment 30 Debarshi Ray 2015-07-31 09:59:14 UTC
Review of attachment 308484 [details] [review]:

++
Comment 31 Debarshi Ray 2015-07-31 10:54:34 UTC
Review of attachment 308485 [details] [review]:

++
Comment 32 Christophe Fergeau 2015-07-31 19:02:56 UTC
Attachment 308440 [details] pushed as a46f431 - Properly fix tp_list_connection_managers_finish() leak
Attachment 308441 [details] pushed as 568fd67 - account-widget: Fix 'default_display_name' leak
Attachment 308442 [details] pushed as a72609e - account-widget: Fix 'login_id' leak in _get_default_display_name()
Attachment 308445 [details] pushed as f4738e4 - account-settings: Fix 'account_req' leak
Attachment 308446 [details] pushed as 906c887 - account-settings: Fix GSimpleAsyncResult leaks
Attachment 308484 [details] pushed as 45e652f - avatar-chooser: Don't call clear_image() from ::init()
Attachment 308485 [details] pushed as ef1e65f - account-settings: Always take ownership of variant in ::set()