Bug 63326 - haze should support purple_account_request_password()
Summary: haze should support purple_account_request_password()
Alias: None
Product: Telepathy
Classification: Unclassified
Component: haze (show other bugs)
Version: 0.6
Hardware: Other All
: medium enhancement
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
Depends on:
Reported: 2013-04-09 16:44 UTC by Stefan Becker
Modified: 2013-04-11 18:43 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:

Patch to support purple_request_fields() (6.34 KB, text/plain)
2013-04-09 16:44 UTC, Stefan Becker
Version 2 of the proposed change (12.92 KB, patch)
2013-04-10 14:00 UTC, Stefan Becker
Details | Splinter Review
Version 3 of the proposed change (19.57 KB, patch)
2013-04-11 12:51 UTC, Stefan Becker
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Becker 2013-04-09 16:44:46 UTC
Created attachment 77679 [details]
Patch to support purple_request_fields()

libpurple plugins which support accounts with optional passwords don't work with telepathy-haze, because it doesn't purple_account_request_password(), which boils down to a purple_request_fields() call to the UI client.

One example is SIPE since version 1.14.1, which no longer works with Empathy (Fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=754395)

Attached is a proposal how this could be implemented. The patch is based on the 0.6.x branch.

Unfortunately this change requires --enable-leaky-request-stubs, so this will need some more work.
Comment 1 Stefan Becker 2013-04-10 14:00:24 UTC
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 2 Will Thompson 2013-04-11 10:49:20 UTC
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.
Comment 3 Stefan Becker 2013-04-11 12:51:12 UTC
Created attachment 77806 [details] [review]
Version 3 of the proposed change

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

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.