+ Trace 230149

Created attachment 213103 [details] [review]
TpAccount: ensure connection is set to NULL before it unref all its contacts

This is to let a chance to applications to properly remove all contacts
at once instead of getting weak notify on them.

With that tp-glib patch, TpfPersonaStore::_notify_connection_cb() is called before all TpContact gets weak notified. But now the problem is the async _store_cache() that keeps personas.

Created attachment 213115 [details] [review]
TpfPersonaStore::_load_cache(): use _add_persona()

Created attachment 213116 [details] [review]
TpfPersonaStore: Immediately call _reset() when disconnected

Otherwise we get weak-notify for each TpContact when TpConnection
unref them.

Review of attachment 213115 [details] [review]:

Nice cleanup.

Review of attachment 213116 [details] [review]:

I’m not sure I understand what’s going on in this change. _load_cache() already calls _reset(), so this is just moving the _reset() call earlier. What’s the timeline of refs/unrefs on TpContacts which causes us to get weak-notifies without this patch, and how does this patch change it?

What happens when connection goes offline now:

1) TpConnection's TpProxy parent class emits "invalidated" signals.
2) TpConnection catch that signal first and unref all its TpContact.
3) folks get _contact_weak_notify_cb() for each contact, warn, and emits personas-changed for each contact one by one.
4) folks get _notify_connection_cb() to tell connection becomes NULL, but it's too late, folks already dropped all contacts.

What happens with patched tp-glib's with attachment #213103 [details]:

1) TpConnection's TpProxy parent class emits "invalidated" signals.
2) TpAccount catch that signal first and emits "notify::connection" to tell connection becomes NULL.
3) folks get _notify_connection_cb() and starts storing roster to cache asyncronously.
4) TpConnection catch the "invalidated" signal emitted in 1 and unref all its TpContact
5) folks get _contact_weak_notify_cb() for each contact, warn, and emits personas-changed for each contact one by one.

What happens with tp-glib patched AND folks patched with attachment # 213116 [details]:
1) TpConnection's TpProxy parent class emits "invalidated" signals.
2) TpAccount catch that signal first and emits "notify::connection" to tell connection becomes NULL.
3) folks get _notify_connection_cb() and calls immediately _reset() which weak_unref() all contacts, then start storing the cache asyncronously.
4) TpConnection catch the "invalidated" signal emitted in 1 and unref all its TpContact, nothing happens because folks does not have weak notify cb on them anymore.
5) folks finishes to store the cache, load it (yes, suboptimal that store and load what we stored...) and emits only one personas-changed with all previous personas removed and all from cache added.

Review of attachment 213116 [details] [review]:

OK, that makes a lot more sense now; thanks. :-)

Please commit once making the changes below.

::: backends/telepathy/lib/tpf-persona-store.vala
@@ +626,3 @@
             {
+              var old_personas = this._persona_set;
+              this._reset ();

Probably a good idea to add a reference to this bug report here, so we don’t break this again in future.

@@ +793,3 @@
    * view of personas from the cache.
    */
+  private async void _load_cache (HashSet<Persona>? old_personas = null)

Might be useful to add a comment stating that the old_personas will be notified as removed using _emit_personas_changed().

@@ +824,3 @@
+        {
+          old_personas = this._persona_set;
+        }

I’d say it’s a bit cleaner to make the old_personas parameter mandatory, and remove this code. Just change the other caller of this._load_cache() to pass this._persona_set in explicitly.

both patches applied. Thanks.