-
-
I'm confused; doesn't
g_task_return_pointer
take aGDestroyNotify
so that the task can free the result if it's not calling anything?-
From the docs:
Sets task 's result to result and completes the task. If result is not NULL, then result_destroy will be used to free result if the caller does not take ownership of it with g_task_propagate_pointer().
So from the top, we have code that calls the
_async
methods without a callback. In my testing this resulted in a memory leak from at least theGTask
being leaked. Since theGTask
is leaked and never finalized, the destroy notify is never called (I'll update my poc to verify this).However, by adding the callback to call the
_finish
methods, the providers are then callingg_task_propagate_*
which in the case ofg_task_propagate_pointer
transfer ownership to the caller, in this case the_finish
method which is why we have to free it. -
Proof of concept with a custom destory notify with no callback (and there for no
_finish
call).grim@spectre:~/p/poc/async-no-callback$ gcc -o poc main.c `pkg-config --cflags --libs glib-2.0 gio-2.0` grim@spectre:~/p/poc/async-no-callback$ ./poc ** (process:1273629): WARNING **: 05:05:19.376: callback_no_propagate ** (process:1273629): WARNING **: 05:05:19.376: destroy notify for 'callback no propagate' ** (process:1273629): WARNING **: 05:05:19.376: callback_with_propagate ** (process:1273629): WARNING **: 05:05:20.994: hit our timeout grim@spectre:~/p/poc/async-no-callback$ cat main.c /* * A proof of concept to test GTasks's with no callbacks. * * compile with: * gcc -o poc main.c `pkg-config --cflags --libs glib-2.0 gio-2.0` */ #include <glib.h> #include <gio/gio.h> static void destroy_notify(gpointer data) { gchar *str = (gchar *)data; g_warning("destroy notify for '%s'", str); g_free(str); } gboolean get_outta_here(gpointer data) { g_warning("hit our timeout"); g_main_loop_quit((GMainLoop *)data); return G_SOURCE_REMOVE; } static void callback_no_propagate(GObject *obj, GAsyncResult *result, gpointer data) { g_warning("callback_no_propagate"); g_object_unref(G_OBJECT(result)); } static void callback_with_propagate(GObject *obj, GAsyncResult *result, gpointer data) { gchar *str = NULL; g_warning("callback_with_propagate"); str = g_task_propagate_pointer(G_TASK(result), NULL); g_free(str); } gint main(gint argc, gchar *argv[]) { GMainLoop *loop = NULL; GTask *task = NULL; loop = g_main_loop_new(NULL, FALSE); /* create a task with no callback */ task = g_task_new(NULL, NULL, NULL, NULL); g_task_return_pointer(task, g_strdup("no callback"), destroy_notify); /* create a task with a callback that doesn't propagate */ task = g_task_new(NULL, NULL, callback_no_propagate, NULL); g_task_return_pointer(task, g_strdup("callback no propagate"), destroy_notify); /* create a task with a callback that does propagate */ task = g_task_new(NULL, NULL, callback_with_propagate, NULL); g_task_return_pointer(task, g_strdup("callback with propagate"), destroy_notify); /* add a timeout to exit */ g_timeout_add_seconds(1, get_outta_here, loop); g_main_loop_run(loop); g_main_loop_unref(loop); return 0; }
-
Looking at the
GTask
(and also the way you've implemented it in secret service), I think your PoC is missing ag_object_unref(task)
after everyg_task_return_pointer
. Which means I probably wrote it incorrect in wincred... -
Well, that might be true... Let me double check what
g_task_propagate_*
does as you can only call it once, so it seems like you can't use it after that which would make sense for it to free it there. But let's be certain. -
Nope looks like we need to free it which means we've been leaking these for a bit now 🤦
So, I'll update this PR and create some tickets to address the others as I'm assuming we're not going to get through all of them right now.
-
I think you misunderstood; the tasks must be unref'd after
g_return_*
calls. This fixes the leak in your poc (and the unref in thecallback_no_propagate
must be removed). -
g_task_return_*
doesn't ref the object, so how would it survive to make it to the callback? -
It does; in fact, it must, because that's how you've written secret service, but also the
GTask
doc examples are written that way as well. -
So burried in a
g_task_return
this is a ref on the object... And you're right that is how I wrote the secret service. Gah why can't I keep this damn API straight?! 🤪 -
So... After updating the proof of concept to unref after
g_task_return
we get this in valgrind..==1291365== ==1291365== HEAP SUMMARY: ==1291365== in use at exit: 41,672 bytes in 318 blocks ==1291365== total heap usage: 581 allocs, 263 frees, 263,667 bytes allocated ==1291365== ==1291365== LEAK SUMMARY: ==1291365== definitely lost: 0 bytes in 0 blocks ==1291365== indirectly lost: 0 bytes in 0 blocks ==1291365== possibly lost: 0 bytes in 0 blocks ==1291365== still reachable: 1,826 bytes in 34 blocks ==1291365== of which reachable via heuristic: ==1291365== length64 : 40 bytes in 1 blocks ==1291365== newarray : 1,552 bytes in 17 blocks ==1291365== suppressed: 39,630 bytes in 282 blocks ==1291365== Reachable blocks (those to which a pointer was found) are not shown. ==1291365== To see them, rerun with: --leak-check=full --show-leak-kinds=all ==1291365== ==1291365== For lists of detected and suppressed errors, rerun with: -s ==1291365== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 22 from 22)
In other words, this totally isn't necessary :)
-
-
Add a default callback to the PurpleCredentialProvider async methods.
Review Request #464 — Created Jan. 29, 2021 and discarded
If the caller didn't provide a callback the GTask and the result will remain in memory, so instead of just letting that leak we consume the tasks in the default handlers by calling the
_finish
methods and free the password if one was returned.
Compiled and verified everything was okay when removing an account (the only easy way to trigger one of the calls that don't have a callback).
Also, I couldn't add unit tests because since there's no callback, and I can't modify the default, everything just times out and makes the tests take forever.
Description | From | Last Updated |
---|---|---|
I'm confused; doesn't g_task_return_pointer take a GDestroyNotify so that the task can free the result if it's not calling anything? |
QuLogic |