Bug 63326 - haze should support purple_account_request_password()
Summary: haze should support purple_account_request_password()
Status: RESOLVED FIXED
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
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
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:


Attachments
Patch to support purple_request_fields() (6.34 KB, text/plain)
2013-04-09 16:44 UTC, Stefan Becker
Details
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

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. How we collect and use information is described in our Privacy Policy.