Add a default callback to the PurpleCredentialProvider async methods.

Review Request #464 — Created Jan. 29, 2021 and discarded

Information

pidgin/pidgin
default

Reviewers

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?

QuLogicQuLogic
QuLogic
  1. 
      
  2. Show all issues

    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?

    1. 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 the GTask being leaked. Since the GTask 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 calling g_task_propagate_* which in the case of g_task_propagate_pointer transfer ownership to the caller, in this case the _finish method which is why we have to free it.

    2. 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;
      }
      
    3. Looking at the GTask (and also the way you've implemented it in secret service), I think your PoC is missing a g_object_unref(task) after every g_task_return_pointer. Which means I probably wrote it incorrect in wincred...

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

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

    6. 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 the callback_no_propagate must be removed).

    7. g_task_return_* doesn't ref the object, so how would it survive to make it to the callback?

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

    9. 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?! 🤪

    10. 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 :)

  3. 
      
grim
grim
Review request changed
Status:
Discarded
Change Summary:

This was broke and completely misunderstood, see the comments for the full reason.