Created attachment 311705 [details] [review]
lastfm: Remove unneeded g_strdup

It's not assigned to any variable and causes a leak:

==30698== 5 bytes in 1 blocks are definitely lost in loss record 145 of 18,389
==30698==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==30698==    by 0x8D780AC: g_malloc (gmem.c:97)
==30698==    by 0x8D783C5: g_malloc_n (gmem.c:336)
==30698==    by 0x8D93B9A: g_strdup (gstrfuncs.c:356)
==30698==    by 0x4C9C629: check_cb (goalastfmprovider.c:525)
==30698==    by 0x4F16A4E: _call_message_completed_cb (rest-proxy-call.c:698)
==30698==    by 0x51A875D: soup_session_process_queue_item (soup-session.c:2106)
==30698==    by 0x51A88D7: async_run_queue (soup-session.c:2145)
==30698==    by 0x51A89FE: idle_run_queue (soup-session.c:2179)
==30698==    by 0x8D72470: g_idle_dispatch (gmain.c:5397)
==30698==    by 0x8D6FAB8: g_main_dispatch (gmain.c:3122)
==30698==    by 0x8D708FC: g_main_context_dispatch (gmain.c:3737)

Created attachment 311706 [details] [review]
lastfm: Fix memory leaks in lastfm_login()

Created attachment 311707 [details] [review]
lastfm: Don't leak AdAccountData::access_token

It's created as a dup'ed string so we can transfer ownership with
g_variant_new_take_string() (instead of g_variant_new_string())

This fixes:
==4326==-
==4326== 33 bytes in 1 blocks are definitely lost in loss record 8,436 of 18,511
==4326==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==4326==    by 0x8D780AC: g_malloc (gmem.c:97)
==4326==    by 0x8D783C5: g_malloc_n (gmem.c:336)
==4326==    by 0x8D93B9A: g_strdup (gstrfuncs.c:356)
==4326==    by 0x4C9C67D: check_cb (goalastfmprovider.c:531)
==4326==    by 0x4F16A4E: _call_message_completed_cb (rest-proxy-call.c:698)
==4326==    by 0x51A875D: soup_session_process_queue_item (soup-session.c:2106)
==4326==    by 0x51A88D7: async_run_queue (soup-session.c:2145)
==4326==    by 0x51A89FE: idle_run_queue (soup-session.c:2179)
==4326==    by 0x8D72470: g_idle_dispatch (gmain.c:5397)
==4326==    by 0x8D6FAB8: g_main_dispatch (gmain.c:3122)
==4326==    by 0x8D708FC: g_main_context_dispatch (gmain.c:3737)

Review of attachment 311705 [details] [review]:

Thanks Christophe! The bug URL is missing from the commit message.

::: src/goabackend/goalastfmprovider.c
@@ +546,3 @@
   session_obj = json_node_get_object (session);
 
+  if (json_object_get_string_member (session_obj, "name") == NULL)

There are two other cases in lastfm_login_sync. (We should really merge the async and sync flows.)

Created attachment 312515 [details] [review]
lastfm: Remove unneeded g_strdup

Added the bug URL, and fixed the two other uses of g_strdup.

Review of attachment 311707 [details] [review]:

The bug URL is missing from the commit message.

::: src/goabackend/goalastfmprovider.c
@@ +724,3 @@
   g_variant_builder_init (&credentials, G_VARIANT_TYPE_VARDICT);
   g_variant_builder_add (&credentials, "{sv}", "password", g_variant_new_string (password));
+  g_variant_builder_add (&credentials, "{sv}", "access_token", g_variant_new_take_string (data.access_token));

Nitpick: I would prefer to use g_variant_new_string, but g_free data.access_token at the end, as we do with the other members of the AddAccountData struct.

I agree that we will be fragmenting memory a bit by needlessly copying and freeing it, but I would rather go for consistency (and avoid surprises) than try to micro-optimize.

@@ +872,3 @@
   g_variant_builder_init (&builder, G_VARIANT_TYPE_VARDICT);
   g_variant_builder_add (&builder, "{sv}", "password", g_variant_new_string (password));
+  g_variant_builder_add (&builder, "{sv}", "access_token", g_variant_new_take_string (data.access_token));

Ditto.

Review of attachment 311706 [details] [review]:

Thanks, Christophe!

Created attachment 313216 [details] [review]
lastfm: Don't leak AddAccountData::access_token

It's created as a dup'ed string so we can transfer ownership with
g_variant_new_take_string() (instead of g_variant_new_string())

This fixes:
==4326==-
==4326== 33 bytes in 1 blocks are definitely lost in loss record 8,436 of 18,511
==4326==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==4326==    by 0x8D780AC: g_malloc (gmem.c:97)
==4326==    by 0x8D783C5: g_malloc_n (gmem.c:336)
==4326==    by 0x8D93B9A: g_strdup (gstrfuncs.c:356)
==4326==    by 0x4C9C67D: check_cb (goalastfmprovider.c:531)
==4326==    by 0x4F16A4E: _call_message_completed_cb (rest-proxy-call.c:698)
==4326==    by 0x51A875D: soup_session_process_queue_item (soup-session.c:2106)
==4326==    by 0x51A88D7: async_run_queue (soup-session.c:2145)
==4326==    by 0x51A89FE: idle_run_queue (soup-session.c:2179)
==4326==    by 0x8D72470: g_idle_dispatch (gmain.c:5397)
==4326==    by 0x8D6FAB8: g_main_dispatch (gmain.c:3122)
==4326==    by 0x8D708FC: g_main_context_dispatch (gmain.c:3737)

Review of attachment 313216 [details] [review]:

Thanks, Christophe. Pushed to both master and gnome-3-18.