Bug 49373 - rethink TpContact<>TpConnection ref cycle
Summary: rethink TpContact<>TpConnection ref cycle
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://cgit.collabora.com/git/user/xc...
Whiteboard:
Keywords: patch
Depends on:
Blocks: tp-glib-1.0
  Show dependency treegraph
 
Reported: 2012-05-02 03:35 UTC by Xavier Claessens
Modified: 2012-06-26 02:39 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Abort preparing contact-list feature early if CONTACTS iface is missing (2.60 KB, patch)
2012-06-01 09:40 UTC, Xavier Claessens
Details | Splinter Review
Small coding style fix (1.23 KB, patch)
2012-06-01 09:40 UTC, Xavier Claessens
Details | Splinter Review
TpConnection: Fix leaked result when preparing contact-list feature (876 bytes, patch)
2012-06-01 09:40 UTC, Xavier Claessens
Details | Splinter Review
Rename _tp_contact_connection_invalidated to _disposed (3.55 KB, patch)
2012-06-01 09:40 UTC, Xavier Claessens
Details | Splinter Review
TpContact: stop keeping strong ref on its connection (4.27 KB, patch)
2012-06-01 09:40 UTC, Xavier Claessens
Details | Splinter Review
Add unit test for TpContact<>TpConnection refcounting (3.43 KB, patch)
2012-06-01 09:40 UTC, Xavier Claessens
Details | Splinter Review
NEWS (752 bytes, patch)
2012-06-01 09:57 UTC, Xavier Claessens
Details | Splinter Review

Description Xavier Claessens 2012-05-02 03:35:05 UTC
From connection.c::tp_connection_invalidated():
  /* Drop the ref we have on all roster contacts, this is to break the refcycle
   * we have between TpConnection and TpContact, otherwise self would never
   * run dispose.
   * Note that invalidated is also called from dispose, so self->priv->roster
   * could already be NULL.
   *
   * FIXME: When we decide to break tp-glib API/guarantees, we should stop
   * TpContact taking a strong ref on its TpConnection and force user to keep
   * a ref on the TpConnection to use its TpContact, this would avoid the
   * refcycle completely. */
Comment 1 Xavier Claessens 2012-06-01 09:40:17 UTC
Created attachment 62384 [details] [review]
Abort preparing contact-list feature early if CONTACTS iface is missing

Current code has issues:
1) It's possible it completes without returning to mainloop first
2) contact-list-state could regress from SUCCESS to FAILED
Comment 2 Xavier Claessens 2012-06-01 09:40:20 UTC
Created attachment 62385 [details] [review]
Small coding style fix
Comment 3 Xavier Claessens 2012-06-01 09:40:24 UTC
Created attachment 62386 [details] [review]
TpConnection: Fix leaked result when preparing contact-list feature
Comment 4 Xavier Claessens 2012-06-01 09:40:27 UTC
Created attachment 62387 [details] [review]
Rename _tp_contact_connection_invalidated to _disposed

That function is called from TpConnection::dispose, not when invalidated
Comment 5 Xavier Claessens 2012-06-01 09:40:31 UTC
Created attachment 62388 [details] [review]
TpContact: stop keeping strong ref on its connection

Reset TpContact::connection to NULL when connection is disposed
Comment 6 Xavier Claessens 2012-06-01 09:40:34 UTC
Created attachment 62389 [details] [review]
Add unit test for TpContact<>TpConnection refcounting
Comment 7 Xavier Claessens 2012-06-01 09:41:28 UTC
patches 1-4 probably needs backporting to master.
Comment 8 Xavier Claessens 2012-06-01 09:57:51 UTC
Created attachment 62390 [details] [review]
NEWS
Comment 9 Xavier Claessens 2012-06-04 04:03:00 UTC
(In reply to comment #7)
> patches 1-4 probably needs backporting to master.

Guillaume reviewed them and they are merged in master already.


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.