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