Bug 63326

Created attachment 77742 [details] [review]
Version 2 of the proposed change

Version 2:

 - commit 1: cleaned up version of version 1
 - commit 2: fix resource leakage in haze_request_field()

Comment on attachment 77742 [details] [review]
Version 2 of the proposed change

Review of attachment 77742 [details] [review]:
-----------------------------------------------------------------

I think this will still leak, actually. Take a look at pidgin/request.c's implementation of request_fields: if the user closes the window without picking a response, it makes sure to call the cancel_cb before purple_request_close(). This allows whatever bit of libpurple made the request to clean up.

But if that's fixed, I think it looks good.

::: src/request.c
@@ +27,3 @@
>  
>  #include "debug.h"
>  #include "request.h"

What happens if libpurple gives us a password request, and before the user answers the challenge, libpurple cancels it?

I think what will happen is: haze_close_request() is called by libpurple, freeing this stuff up.

Then later, haze_request_password_cb() will be called, which will call back into libpurple with a freed request and crash.

@@ +33,2 @@
>  static gpointer
>  haze_request_input (const char *title,

Use g_slice_free (fields_data, fd);

@@ +104,5 @@
> +struct password_data {
> +    PurpleRequestFields *fields;
> +    PurpleRequestField *password;
> +    GCallback ok_cb;
> +    GCallback cancel_cb;

I think it would be better to cast these to PurpleRequestFieldsCb in haze_request_fields, not at the point you call them. I think that would be clearer.

@@ +138,4 @@
>                       PurpleConversation *conv,
>                       void *user_data)
>  {
> +    /*

Use g_slice_new0.

@@ +166,2 @@
>  
>      return NULL;

I think the idle callback should call the cancel_cb, so that whatever made the request doesn't leak.

Created attachment 77806 [details] [review]
Version 3 of the proposed change

Thanks for the review comments. Please see attached version 3.

Merged to master:

http://cgit.freedesktop.org/telepathy/telepathy-haze/commit/?id=66b7fff2f65b65a5d298bf868bb8e21a32492a58
http://cgit.freedesktop.org/telepathy/telepathy-haze/commit/?id=eef971c3aeb61b9b60013f3b728f62e7ac5580bf
http://cgit.freedesktop.org/telepathy/telepathy-haze/commit/?id=4068d8504b2155168b82144a4d45b138e0579ef2

Thanks!