After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 702165 - Empathy crash when enabling/disabling accounts repeatidly
Empathy crash when enabling/disabling accounts repeatidly
Status: RESOLVED OBSOLETE
Product: folks
Classification: Platform
Component: general
git master
Other Linux
: High major
: 0.12.x
Assigned To: folks-maint
folks-maint
Depends on:
Blocks:
 
 
Reported: 2013-06-13 10:57 UTC by Xavier Claessens
Modified: 2018-09-21 16:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
TpfPersona: Manage the TpContact weak ref ourself instead of using GWeakRef (6.52 KB, patch)
2013-07-17 10:56 UTC, Xavier Claessens
rejected Details | Review
TpfPersona: Manage the TpContact weak ref ourself instead of using GWeakRef (7.16 KB, patch)
2013-07-17 11:30 UTC, Xavier Claessens
committed Details | Review

Description Xavier Claessens 2013-06-13 10:57:25 UTC
Steps:

1) configure a salut and gtalk accounts, put them online and open empathy contact list.
2) disable then enable salut account
3) disable then enable gtalk account
4) repeat 2 & 3 until empathy crash. It can take a while...

Here is the backtrace:

  • #0 folks_persona_store_get_trust_level
    at persona-store.c line 672
  • #1 _folks_individual_aggregator_add_personas
    at individual-aggregator.c line 3653
  • #2 _folks_individual_aggregator_personas_changed_cb
    at individual-aggregator.c line 5112
  • #3 g_cclosure_user_marshal_VOID__OBJECT_OBJECT_STRING_OBJECT_ENUM
    at persona-store.c line 758
  • #4 g_closure_invoke
    at /build/buildd/glib2.0-2.36.2/./gobject/gclosure.c line 777
  • #5 signal_emit_unlocked_R
    at /build/buildd/glib2.0-2.36.2/./gobject/gsignal.c line 3584
  • #6 g_signal_emit_valist
    at /build/buildd/glib2.0-2.36.2/./gobject/gsignal.c line 3328
  • #7 g_signal_emit_by_name
    at /build/buildd/glib2.0-2.36.2/./gobject/gsignal.c line 3424
  • #8 _folks_persona_store_emit_personas_changed
    at persona-store.c line 446
  • #9 ??
    from /usr/lib/x86_64-linux-gnu/libfolks-telepathy.so.25
  • #10 ??
    from /usr/lib/x86_64-linux-gnu/libfolks-telepathy.so.25
  • #11 g_simple_async_result_complete
    at /build/buildd/glib2.0-2.36.2/./gio/gsimpleasyncresult.c line 777
  • #12 ??
    from /usr/lib/x86_64-linux-gnu/libfolks-telepathy.so.25
  • #13 g_simple_async_result_complete
    at /build/buildd/glib2.0-2.36.2/./gio/gsimpleasyncresult.c line 777
  • #14 folks_object_cache_store_objects_co
    at object-cache.c line 1275
  • #15 g_task_return_now
    at /build/buildd/glib2.0-2.36.2/./gio/gtask.c line 1105
  • #16 g_task_return
    at /build/buildd/glib2.0-2.36.2/./gio/gtask.c line 1158
  • #17 g_task_return
    at /build/buildd/glib2.0-2.36.2/./gio/gtask.c line 1126
  • #18 replace_contents_close_callback
    at /build/buildd/glib2.0-2.36.2/./gio/gfile.c line 6913
  • #19 g_task_return_now
    at /build/buildd/glib2.0-2.36.2/./gio/gtask.c line 1105
  • #20 g_task_return
    at /build/buildd/glib2.0-2.36.2/./gio/gtask.c line 1158
  • #21 g_task_return
    at /build/buildd/glib2.0-2.36.2/./gio/gtask.c line 1126
  • #22 async_ready_close_callback_wrapper
    at /build/buildd/glib2.0-2.36.2/./gio/goutputstream.c line 1131
  • #23 g_task_return_now
    at /build/buildd/glib2.0-2.36.2/./gio/gtask.c line 1105
  • #24 complete_in_idle_cb
    at /build/buildd/glib2.0-2.36.2/./gio/gtask.c line 1114
  • #25 g_main_dispatch
    at /build/buildd/glib2.0-2.36.2/./glib/gmain.c line 3054
  • #26 g_main_context_dispatch
    at /build/buildd/glib2.0-2.36.2/./glib/gmain.c line 3630
  • #27 g_main_context_iterate
    at /build/buildd/glib2.0-2.36.2/./glib/gmain.c line 3701
  • #28 g_main_context_iteration
    at /build/buildd/glib2.0-2.36.2/./glib/gmain.c line 3762
  • #29 g_application_run
    at /build/buildd/glib2.0-2.36.2/./gio/gapplication.c line 1623
  • #30 main
    at empathy.c line 847

Comment 1 Xavier Claessens 2013-06-14 14:01:03 UTC
Here is a more complete backtrance:

  • #0 g_object_ref
    at gobject.c line 3049
  • #1 _g_object_ref0
    at /home/xclaesse/programmation/folks/folks/individual.vala line 84
  • #2 _folks_individual_set_personas
    at /home/xclaesse/programmation/folks/folks/individual.vala line 2499
  • #3 folks_individual_set_personas
    at /home/xclaesse/programmation/folks/folks/individual.vala line 976
  • #4 object_set_property
    at gobject.c line 1358
  • #5 g_object_new_with_custom_constructor
    at gobject.c line 1718
  • #6 g_object_new_internal
    at gobject.c line 1736
  • #7 g_object_new_valist
    at gobject.c line 1994
  • #8 g_object_new
    at gobject.c line 1551
  • #9 folks_individual_construct
    at /home/xclaesse/programmation/folks/folks/individual.vala line 1170
  • #10 folks_individual_new
    at /home/xclaesse/programmation/folks/folks/individual.vala line 1168
  • #11 _folks_individual_aggregator_add_personas
    at /home/xclaesse/programmation/folks/folks/individual-aggregator.vala line 1227
  • #12 _folks_individual_aggregator_personas_changed_cb
    at /home/xclaesse/programmation/folks/folks/individual-aggregator.vala line 1562
  • #13 g_cclosure_user_marshal_VOID__OBJECT_OBJECT_STRING_OBJECT_ENUM
    at /home/xclaesse/programmation/folks/folks/persona-store.vala line 323
  • #14 g_closure_invoke
    at gclosure.c line 777
  • #15 signal_emit_unlocked_R
    at gsignal.c line 3582
  • #16 g_signal_emit_valist
    at gsignal.c line 3326
  • #17 g_signal_emit_by_name
    at gsignal.c line 3422
  • #18 _folks_persona_store_emit_personas_changed
    at /home/xclaesse/programmation/folks/folks/persona-store.vala line 449
  • #19 _tpf_persona_store_self_contact_changed_cb
    at /home/xclaesse/programmation/folks/backends/telepathy/lib/tpf-persona-store.vala line 1177
  • #20 _tpf_persona_store_notify_connection_cb_async_co
    at /home/xclaesse/programmation/folks/backends/telepathy/lib/tpf-persona-store.vala line 869
  • #21 g_simple_async_result_complete
    at gsimpleasyncresult.c line 777
  • #22 connection_get_alias_flags_cb
    at tp-lowlevel.c line 52
  • #23 ??
    from /usr/lib/x86_64-linux-gnu/libtelepathy-glib.so.0
  • #24 ??
    from /usr/lib/x86_64-linux-gnu/libtelepathy-glib.so.0
  • #25 g_main_dispatch
    at gmain.c line 3058
  • #26 g_main_context_dispatch
    at gmain.c line 3634
  • #27 g_main_context_iterate
    at gmain.c line 3705
  • #28 g_main_context_iteration
    at gmain.c line 3766
  • #29 g_application_run
    at gapplication.c line 1624
  • #30 main
    at empathy.c line 831

What I understand so far is that for some reason, when an account connects it will add its self persona. The aggregator then link it with another persona that comes from a disposed store.
Comment 2 Guillaume Desmottes 2013-07-16 14:12:59 UTC
Here is another crash which seems to be related.

(lt-empathy:28315): telepathy-DEBUG: tpf-persona-store.vala:1210: contact list changed: 0 added, 1 removed
(lt-empathy:28315): telepathy-DEBUG: tpf-persona-store.vala:1087: Remove persona 0x8ebb120 with uid telepathy:/org/freedesktop/Telepathy/Account/salut/local_xmpp/account0:xclaesse@X230
(lt-empathy:28315): folks-DEBUG: individual-aggregator.vala:1488: Removing Personas:
(lt-empathy:28315): folks-DEBUG: individual-aggregator.vala:1492:     telepathy:/org/freedesktop/Telepathy/Account/salut/local_xmpp/account0:xclaesse@X230 (is user: no, IID: local-xmpp:xclaesse@X230)
(lt-empathy:28315): folks-DEBUG: individual-aggregator.vala:1516: Removing Individuals due to removed links:
(lt-empathy:28315): folks-DEBUG: individual-aggregator.vala:1523:     50dc7cf77b81a088a063d487aa3ace088288762b
(lt-empathy:28315): folks-DEBUG: individual-aggregator.vala:1442: Removing Individual '50dc7cf77b81a088a063d487aa3ace088288762b' from the link map.
(lt-empathy:28315): folks-DEBUG: individual-aggregator.vala:1455:     local-xmpp:xclaesse@X230 → 50dc7cf77b81a088a063d487aa3ace088288762b (0x7479a10)
(lt-empathy:28315): folks-DEBUG: individual-aggregator.vala:1545: Adding Personas:
(lt-empathy:28315): folks-DEBUG: individual-aggregator.vala:1561: Relinking Personas:
(lt-empathy:28315): folks-DEBUG: individual-aggregator.vala:1037: Emitting individuals-changed-detailed with 1 mappings:
(lt-empathy:28315): folks-DEBUG: individual-aggregator.vala:1047:     50dc7cf77b81a088a063d487aa3ace088288762b (0x7479a10) →  ((nil))
(lt-empathy:28315): folks-DEBUG: individual-aggregator.vala:1054:       Removed individual's personas:
(lt-empathy:28315): folks-DEBUG: individual-aggregator.vala:1639: Replacing Individuals due to linking:
(lt-empathy:28315): folks-DEBUG: individual.vala:1183: Destroying Individual '50dc7cf77b81a088a063d487aa3ace088288762b': 0x7479a10
(lt-empathy:28315): telepathy-DEBUG: tpf-persona-store.vala:1210: contact list changed: 0 added, 1 removed
(lt-empathy:28315): telepathy-DEBUG: tpf-persona-store.vala:1108: Weak notify for TpContact xclaesse@X230
(lt-empathy:28315): telepathy-DEBUG: tpf-persona.vala:1236: Destroying Tpf.Persona 'telepathy:/org/freedesktop/Telepathy/Account/salut/local_xmpp/account0:xclaesse@X230': 0x8ebb120
(lt-empathy:28315): telepathy-DEBUG: tpf-persona.vala:606: TpContact 0x8fc2000 destroyed; setting ._contact = null in Persona 0x8ebb120

(lt-empathy:28315): GLib-GObject-CRITICAL **: g_object_notify: assertion `G_IS_OBJECT (object)' failed

Program received signal SIGTRAP, Trace/breakpoint trap.
0x00007ffff08f5982 in g_logv (log_domain=0x7ffff0c2d0cf "GLib-GObject", 
    log_level=G_LOG_LEVEL_CRITICAL, format=0x7ffff097cfec "%s: assertion `%s' failed", 
    args=0x7fffffffcdd8) at gmessages.c:974
974			G_BREAKPOINT ();
(gdb) bt
  • #0 g_logv
    at gmessages.c line 974
  • #1 g_log
    at gmessages.c line 1010
  • #2 g_return_if_fail_warning
  • #3 g_object_notify
    at gobject.c line 1157
  • #4 _tpf_persona_contact_weak_notify_cb
    at tpf-persona.c line 1741
  • #5 __tpf_persona_contact_weak_notify_cb_gweak_notify
    at tpf-persona.c line 517
  • #6 weak_refs_notify
    at gobject.c line 2470
  • #7 g_data_set_internal
    at gdataset.c line 411
  • #8 g_datalist_id_set_data_full
    at gdataset.c line 674
  • #9 g_object_real_dispose
    at gobject.c line 1013
  • #10 tp_contact_dispose
    at contact.c line 900
  • #11 g_object_unref
    at gobject.c line 2987
  • #12 g_ptr_array_foreach
    at garray.c line 1456
  • #13 ptr_array_free
    at garray.c line 1081
  • #14 g_ptr_array_unref
    at garray.c line 1031
  • #15 contacts_changed_head_ready
    at connection-contact-list.c line 130
  • #16 process_queued_contacts_changed
    at connection-contact-list.c line 187
  • #17 contacts_changed_cb
    at connection-contact-list.c line 218
  • #18 _tp_cli_connection_interface_contact_list_invoke_callback_for_contacts_changed_with_id
    at _gen/tp-cli-connection-body.h line 11295
  • #19 tp_proxy_signal_invocation_run
    at proxy-signals.c line 268
  • #20 g_idle_dispatch
    at gmain.c line 5205
  • #21 g_main_dispatch
    at gmain.c line 3054
  • #22 g_main_context_dispatch
    at gmain.c line 3630
  • #23 g_main_context_iterate
    at gmain.c line 3701
  • #24 g_main_context_iteration
    at gmain.c line 3762
  • #25 g_application_run
    at gapplication.c line 1623
  • #26 main
    at empathy.c line 842

Comment 3 Xavier Claessens 2013-07-16 14:35:05 UTC
Ok so here is how I understand this Guillaume's crash (I hope other crash are related):

a) TpfPersonaStore takes a weak_ref on the TpContact
b) TpfPersona creates a GWeakRef for its this._contact
c) TpfPersona takes a weak_ref on the TpContact as well

When a TpContact is disposed, here what happens:

1) From the code of g_object_unref() we can learn than GWeakRef is the first to be set to NULL when an object's ref count hit 0, before weak notify callbacks
2) TpfPersonaStore's _contact_weak_notify_cb() is called first and it drops its strong ref on the TpfPersona. Sometimes it can happen that this was the last ref on the TpfPersona...
3) ... so ~Persona is called and does this._contact.get() which returns NULL because of step 1 so TpfPersona's weak ref on the TpContact is NOT removed!
4) _contact_weak_notify_cb() is called on TpfPersona, after it has been finalized => crash.

Now the question is how to avoid that ?
Comment 4 Xavier Claessens 2013-07-16 14:39:57 UTC
This bug is probably introduced by the fix of bug #680335.
Comment 5 Xavier Claessens 2013-07-17 10:56:35 UTC
Created attachment 249393 [details] [review]
TpfPersona: Manage the TpContact weak ref ourself instead of using GWeakRef

See https://bugzilla.gnome.org/show_bug.cgi?id=702165#c3 for an
explaination.
Comment 6 Guillaume Desmottes 2013-07-17 11:01:37 UTC
Review of attachment 249393 [details] [review]:

Looks good to me but may be best to ask Philip's advice as well as he wrote the original hack.
Comment 7 Xavier Claessens 2013-07-17 11:26:03 UTC
Review of attachment 249393 [details] [review]:

Actually this is not working. What happens now when a TpContact dies:

1) TpfPersonaStore::_contact_weak_notify_cb() is called first and unref the persona
2) The TpfPersona dies and does this._contact.weak_unref() which raise a warning because we cannot weak_unref() an object from within the weak_notify of step1
3) If it's not running with fatal-warnings then TpfPersona::_contact_weak_notify_cb() gets called on finalized persona => crash.
Comment 8 Xavier Claessens 2013-07-17 11:30:58 UTC
Created attachment 249396 [details] [review]
TpfPersona: Manage the TpContact weak ref ourself instead of using GWeakRef
Comment 9 Xavier Claessens 2013-07-17 11:33:33 UTC
That really fix crash from comment #2, but I still get the crash from comment #1.
Comment 10 Philip Withnall 2013-07-29 06:26:41 UTC
Review of attachment 249396 [details] [review]:

Looks good. Please commit.

::: backends/telepathy/lib/tpf-persona-store.vala
@@ +1105,3 @@
         }
 
+      persona._contact_weak_notify();

Need a space before the ‘()’.
Comment 11 Xavier Claessens 2013-07-29 08:27:27 UTC
Thanks, pushed to both master and 0.8 branch.

I'm not closing this bug since the initial crash is still reproducible.
Comment 12 Emilio Pozuelo Monfort 2013-08-21 17:35:14 UTC
I have been able to reproduce the crash in comment #1 with folks 0.9.1 and folks 0.9.2, empathy 3.8.3. I haven't been able to reproduce it with folks master after trying for a while, but given how hard it is to reproduce...

In individual.vala:2512 we have:

                  /* Increment the Persona count for this PersonaStore */
                  var store = p.store;

That translates into:

                  _tmp25_ = p;
                  _tmp26_ = folks_persona_get_store (_tmp25_);
                  _tmp27_ = _tmp26_;
                  _tmp28_ = _g_object_ref0 (_tmp27_);
                  store = _tmp28_;

and it asserts in _g_object_ref0 because store is not an object.

I don't know whether p is a valid object at this stage. folks_persona_get_store(persona) only checks for `persona != NULL', not for IS_PERSONA(persona). Same for folks_persona_get_is_user() which is called a few lines before. So I'm not sure yet where the bug really.
Comment 13 Simon McVittie 2013-08-22 15:14:05 UTC
I had a brief look at this a while ago, and my immediate thought was that Folks seems to be relying on last-unref as having some sort of semantic meaning. That's not right, particularly if you're binding into JavaScript (which has GC, not refcounting, which means extra refs can lurk for an arbitrarily long time). If there is an external event that means "this PersonaStore is dead, stop referring to its Personas" or something, then there should be a flag/signal for that instead, like Telepathy's TpProxy::invalidated.

Doing things in response to a weak ref being released is always a little precarious: you're dealing with a "near-death" object which might already have released the state you need in order to do cleanup. g_object_weak_ref() and g_object_add_weak_pointer() are worse than GWeakRef for this, because they only "go off" during GObjectClass.dispose (at which point the dying object is probably already unusable), whereas a GWeakRef is released before the dying object's dispose callback and can't call into arbitrary user-supplied code.
Comment 14 Simon McVittie 2013-08-22 15:18:56 UTC
From the other bug:

> 1. The TpContact is finalised, triggering its weak notifications.
> ...
> 2. _contact_weak_notify_cb() in Tpf.PersonaStore is called first,
> and it removes the Persona (corresponding to the TpContact) from
> various maps.

Is this relying on the fact that a disconnected TpConnection unrefs the list of TpContacts that it would return from tp_connection_dup_contact_list()? That seems unreliable. Shouldn't it listen for TpConnection::invalidated instead, and clear all this stuff out in response to that?
Comment 15 Philip Withnall 2014-04-06 20:33:58 UTC
(In reply to comment #14)
> From the other bug:
> 
> > 1. The TpContact is finalised, triggering its weak notifications.
> > ...
> > 2. _contact_weak_notify_cb() in Tpf.PersonaStore is called first,
> > and it removes the Persona (corresponding to the TpContact) from
> > various maps.
> 
> Is this relying on the fact that a disconnected TpConnection unrefs the list of
> TpContacts that it would return from tp_connection_dup_contact_list()? That
> seems unreliable. Shouldn't it listen for TpConnection::invalidated instead,
> and clear all this stuff out in response to that?

I haven’t had a chance to look closely, but what you say does sound about right. Will the ::invalidated signals and associated machinery in Telepathy change for Telepathy 1.0? Is it worthwhile spending time fixing this now, or waiting for the Tp 1.0 branch to get merged into folks?
Comment 16 Simon McVittie 2014-04-07 11:04:34 UTC
(In reply to comment #15)
> I haven’t had a chance to look closely, but what you say does sound about
> right. Will the ::invalidated signals and associated machinery in Telepathy
> change for Telepathy 1.0?

The one change I've considered is this: at the moment, TpProxy's dispose() emits TpProxy::invalidated; but as part of the principle I alluded to above ("last-unref shouldn't have side-effects") I've considered removing that. If I do, getting the full effect of Telepathy 0.x's ::invalidated would require both a weak-ref and an ::invalidated listener.

I think it might be too late for that at this point, though, if we want Telepathy 1 to reach a somewhat stable state within the next 2 weeks (which it has to, really, if we want it to go in GNOME 3.14 rather than slipping yet another 6 months). Semantic changes that don't break compilation are annoying.
Comment 17 GNOME Infrastructure Team 2018-09-21 16:02:52 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/folks/issues/63.