GNOME Bugzilla – Bug 683390
Individuals sometimes not removed when disabling their Telepathy account
Last modified: 2012-09-24 22:32:03 UTC
Hi! This happened with: empathy 3.5.91 (built with GOA disabled, UOA enabled) libfolks 0.7.3 mission-control 5.13.0 tp-glib 0.19.6 When disabling an account (I noticed this with google and facebook, but maybe other accounts are also affected), contacts are not removed from the contact list. When the account is re-enabled, I've seen different behaviours happening: - contacts just remain there (kind of correct) - duplicate (or triplicate, if you trigger that once more) contacts appear - contacts are not duplicated, but their link count is (that is, hovering on a contact tells me that they are linked to the same google-talk account twice) Doing this trick repeatedly eventually leads to a crash (see bug 683386) (sometimes the crash is immediate, sometimes it takes longer to happen).
Let's focus on contacts not being removed from the contact list when the Telepathy account is disabled. I managed to reproduce this, it's not specific to UOA.
That's a Folks issue, here is what happen: - Account is disabled and so disconnected - tpf-persona-store : _notify_connection_cb is called as the TpConnection of the TpAccount has been removed. It calls _reset() - _reset() : this._persona_set is recreated - As the account has been disabled, the callback of this._account_manager.account_disabled.connect is called, calling _remove_store() - _remove_store(): this._emit_personas_changed (null, this._persona_set); is called to notify about the removed personas BUT this._persona_set is now empty as we re-created it in _reset(). An easy way to fix this is to call this._emit_personas_changed (null, this._persona_set); in _reset() before resetting the set. I don't think if that's the proper fix though.
Created attachment 223643 [details] [review] tpf-persona-store: call _emit_personas_changed() when reseting _persona_set We need to notify when we reset the persona set.
That seems to work here. Merged the patch to master.
(In reply to comment #2) > An easy way to fix this is to call this._emit_personas_changed (null, > this._persona_set); in _reset() before resetting the set. I don't think if > that's the proper fix though. Indeed it is not the proper fix. Unless I’m mistaken it will mess up the ‘personas-changed’ notifications when loading and unloading the cache. Would it be possible to check the disabled/enabled status of the account from within _notify_connection_cb() and emit the signal from there?
(In reply to comment #5) > Would it be possible to check the disabled/enabled status of the account from > within _notify_connection_cb() and emit the signal from there? No, because at this stage the account is still enabled from tp-glib's pov (I just checked).
Created attachment 223825 [details] [review] Call _emit_personas_changed() if load_cache() isn’t called https://www.gitorious.org/folks/folks/commits/683390-tp-account-disables I think what’s actually happening is that TpAccount::enabled is going false between folks calling store_cache() and calling load_cache(). The load_cache() call is never made because the TpAccount is disabled by that point. load_cache() is what normally emits the personas-changed signal. I think the correct fix is to manually emit personas-changed if load_cache() is not called by _connection_notify_cb(). That’s what this patch does (as well as reverting the previous commit). I haven’t tested it, though, so I might have misunderstood the problem. In that case, ignore me and commit anything that fixes the problem. We can always tidy it up later. (I say this because I’m disappearing for a week or two now, and won’t have Internet access to review patches.)
Created attachment 223826 [details] [review] Call _emit_personas_changed() if load_cache() isn’t called (updated) Let’s actually upload the patch I said I was uploading…
Review of attachment 223826 [details] [review]: I tested this patch and it doesn't seem to fix the issue: sometimes the contacts are not removed when the account is disabled.
(In reply to comment #9) > I tested this patch and it doesn't seem to fix the issue: sometimes the > contacts are not removed when the account is disabled. I’ve tried again to reproduce the issue, and I still can’t. However, further code examination makes me think that perhaps it’s still being caused because the account-removed signal is being emitted (and thus causing _remove_store() to be called) during the store_cache() async call in _notify_connection_cb(). That’s causing the PersonaStore to emit its ‘removed’ signal (and thus be removed) before it reaches the point where it emits personas-changed. A tidy fix to this would be to remove the account-removed, account-validity-changed and account-disabled signal handlers from TpfPersonaStore, and just move their code into _notify_connection_cb() so that it’s only executed at the right point during disconnection. However, this is only possible if tp-glib guarantees that an account will always be disconnected when it’s removed, disabled, or its validity is changed. (More specifically, if tp-glib guarantees that TpAccount::connection is notified.) Simon, is that the case?
(In reply to comment #10) > However, this is only possible if tp-glib guarantees that an account will > always be disconnected when it’s removed, disabled, or its validity is changed. > (More specifically, if tp-glib guarantees that TpAccount::connection is > notified.) Simon, is that the case? It's not amazingly clear. The signals produced by telepathy-glib will depend on what Mission Control does, and as you might be aware, MC is a tangled swamp, particularly the interaction between accounts and connections. If TpAccount:connection has been non-NULL, then it will notify::connection via account.c:connection_invalidated_cb() whenever that TpConnection gets invalidated (disconnected, CM crash, etc.). So far so good. If TpAccount:connection is already NULL (including if the account has a TpConnection, but it was never successfully prepared and CONNECTED before the connection died), then it will not notify::connection, because there was no API-user-visible change. Is that going to be a problem? When an account is disabled, MC will try to disconnect it, but I don't think I can guarantee anything about the ordering; the TpConnection might persist for a bit (or forever, if the CM is woefully broken). When an account is removed, it gets disabled first, so the same thing happens. I consider this to be an implementation detail - Folks should not rely on the account getting disabled - but I do think it's OK to think that "MC will try to disconnect accounts that get deleted" is a guarantee. When an account becomes invalid, any existing connection will stay. However, it's hard to make an account go invalid during runtime - you'd have to either uninstall the CM from under it (in which case it should still be valid until the running instance exits), or set wrong Parameters (which MC isn't meant to let you do, but a buggy account storage backend could cause it to happen). It's not clear to me that Folks should handle the valid -> invalid and enabled -> disabled transitions at all: if there's still a connection somehow, then any contacts on that connection remain just as valid as they always were, until the connection actually drops. If you treat account removal specially (as "remove the persona store altogether"), then I think you should probably have a special case that does what you'd normally do on disconnection, first.
Created attachment 225015 [details] [review] Call _remove_store() if load_cache() isn’t called https://www.gitorious.org/folks/folks/commits/683390-tp-account-disables-rebase1 How about this? It turns out I can’t eliminate the account-removed (etc.) signal handlers completely, since they’re still needed for the case where an account is removed while already disconnected. Instead, account removals which occur while disconnecting are now effectively delayed until after disconnection is complete. This works for me when tested, but I couldn’t reproduce the original issue, so that doesn’t mean much.
(In reply to comment #11) > It's not amazingly clear. The signals produced by telepathy-glib will depend on > what Mission Control does, and as you might be aware, MC is a tangled swamp, > particularly the interaction between accounts and connections. > > *snip* Thanks for the detailed explanation Simon. Could this possibly make its way into the telepathy-glib Devhelp documentation, perhaps together with information about the ordering of TpProxy:invalidated signal emissions at the same time? It would be quite useful there, I think.
Review of attachment 225015 [details] [review]: The patch makes sense to me and I didn't experience the problem when testing it (but it's a race so we can't never be sure). ::: backends/telepathy/lib/tpf-persona-store.vala @@ +437,2 @@ { + if (this._disconnect_pending == true) we don't usually use explicit comparaisons with booleans but maybe you do in Folks?
Comment on attachment 225015 [details] [review] Call _remove_store() if load_cache() isn’t called Yes, folks uses explicit boolean comparisons just to make things more obvious. commit 0f078082df220a2de767887d2050875612eea939 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Sat Sep 8 23:32:32 2012 +0100 telepathy: Always notify of persona changes when disconnecting This reverts commit af500a4bd9fe88ae9f3832a37e19a86a4273ba5d and applies a different fix. The problem was that persona changes could get away without being notified when disconnecting due to an account being disabled. This is because, after storing the cache, an additional check is performed for whether the account was enabled. If it is, the cache is loaded (and persona changes notified). If not, the cache is not loaded, *and persona changes were not notified*. This situation could occur when a Telepathy account is disabled: 1. _notify_connection_cb() is called, but the TpAccount::enabled property is still true. 2. While in store_cache(), the TpAccount::enabled property goes false. 3. The check before load_cache() would fail, so load_cache() would never be called.
…aaaand the bottom half of the commit message: Similarly, it could occur when a Telepathy account is removed, with TpAccountManager:account-removed being emitted while in store_cache(). Closes: https://bugzilla.gnome.org/show_bug.cgi?id=683390 NEWS | 2 + backends/telepathy/lib/tpf-persona-store.vala | 58 ++++++++++++++++++++------- 2 files changed, 46 insertions(+), 14 deletions(-)