[Proposed to be fixed for 0.16.] When you get a GAsyncResult, it's meant to be valid to ref it and keep it around. From the GIO reference manual: Applications may also take a reference to the GAsyncResult and call "_finish()" later; however, the "_finish()" function may be called at most once. Some of our GAsyncResults had a borrowed pointer as their payload, which means that if you did this, by the time you tried to "_finish()" them, the pointer could have become invalid.
Created attachment 56541 [details] [review] [1/7] TpTestsSimpleAccount: add Avatar interface
Created attachment 56542 [details] [review] [2/7] TpAccount: ensure that async-returned objects live as long as the result It is valid to keep a GAsyncResult for as long as you like, so it will have to take a copy of the result data. Otherwise, a change as simple as replacing g_simple_async_result_complete with ..._complete_in_idle will result in the data having already been freed by the time the caller sees it.
Created attachment 56543 [details] [review] [3/7] account test: deliberately keep async results after the callback This is valid usage, and often (as in this case!) uncovers bugs. It also makes the flow of the code more obvious.
Created attachment 56544 [details] [review] [4/7] tp_account_manager_create_account_finish: warn that the return has a limited lifetime
Created attachment 56545 [details] [review] [5/7] tp_account_update_parameters_async: fix lifetime of result, and test it Without this change to TpAccount, the test would fail with a use-after-free while inspecting reconnect_required. The TpAccount code assumed that _finish would always be called directly from the callback, but it is perfectly valid not to do so. In the test, also test Reconnect (which is currently unimplemented), since UpdateParameters and Reconnect are so closely related.
Created attachment 56546 [details] [review] [6/7] TpSimplePasswordManager: copy the string into the result Otherwise, if a caller kept a ref to the GAsyncResult after control had returned to the channel, the channel could have freed the GString already.
Created attachment 56547 [details] [review] [WiP 7/7] Channel contacts machinery: ref the queue item in the result Otherwise, if a caller kept a ref to the GAsyncResult after control had returned to the channel, the channel could have freed the item and its contents already. --- Unfortunately, this one is in fact a refleak: the result and item ref each other, circularly. We'll need to think about this one a bit more. One possible implementation is to reverse the ownership, and turn the queue of ContactsQueueItem into a list of GSimpleAsyncResult? In the meantime, 1/7 to 6/7 can be merged, if reviewers are happy with them?
Comment on attachment 56545 [details] [review] [5/7] tp_account_update_parameters_async: fix lifetime of result, and test it Review of attachment 56545 [details] [review]: ----------------------------------------------------------------- ::: tests/dbus/account.c @@ +262,5 @@ > + const gchar *unset[] = { "unset", NULL }; > + GStrv reconnect_required; > + > + test->account = tp_account_new (test->dbus, > + "/org/freedesktop/Telepathy/Account/what/ev/er", NULL); Please use TP_ACCOUNT_OBJECT_PATH_BASE here so merging this stuff into next is easier.
Okay I reviewed patches 1-6 and they all look fine apart from the change above.
(In reply to comment #9) > Okay I reviewed patches 1-6 and they all look fine apart from the change above. Thanks, I went one better and used ACCOUNT_PATH. Fixed in git for 0.16.5 and 0.17.5, with the exception of 7/7 which still needs work (and I'm tempted to only fix it in 0.17.x since the fix is relatively intrusive).
Created attachment 59796 [details] [review] [1/2] channel-contacts: merge contacts_queue_item_new into contacts_queue_item
Created attachment 59798 [details] [review] [2/2] channel-contacts: reverse ownership of ContactsQueueItem and result Previously, the ContactsQueueItem assumed that it owned the only reference to the GSimpleAsyncResult. This could result in the GSAR still existing after the CQI had been freed, if someone else took a reference to the GSAR. Refcounting the CQI would not solve this problem, because then the CQI and the GSAR would have a cyclic reference. Instead, change the queue of CQIs into a queue of GSARs, and have the CQI owned by the GSAR. This means the GSAR's refcounting protects the CQI from being freed prematurely.
Unblocks Bug #45514.
Comment on attachment 59796 [details] [review] [1/2] channel-contacts: merge contacts_queue_item_new into contacts_queue_item Review of attachment 59796 [details] [review]: ----------------------------------------------------------------- Looks fine. ::: telepathy-glib/channel-contacts.c @@ +435,4 @@ > GAsyncReadyCallback callback, > gpointer user_data) > { > + ContactsQueueItem *item = g_slice_new (ContactsQueueItem); I always prefer g_slice_new0() but I guess whatevs as you're assigning to all values right away.
Comment on attachment 59798 [details] [review] [2/2] channel-contacts: reverse ownership of ContactsQueueItem and result Review of attachment 59798 [details] [review]: ----------------------------------------------------------------- Nicely done!
0.19.0
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.