GNOME Bugzilla – Bug 680335
empathy crashed with SIGSEGV in _tpf_persona_contact_weak_notify_cb()
Last modified: 2012-09-05 23:06:18 UTC
I did not do anything with the empathy client at the time of the crash. ProblemType: Crash DistroRelease: Ubuntu 12.10 Package: empathy 3.5.3-0ubuntu1 ProcVersionSignature: Ubuntu 3.5.0-4.4-generic 3.5.0-rc6 Uname: Linux 3.5.0-4-generic x86_64
Created attachment 219340 [details] stacktrace
Looks like it crashes in Folks.
originally reported at https://bugs.launchpad.net/ubuntu/+source/empathy/+bug/1024217
Bilal, what version of folks do you have installed?
Might be bug #675141. That was fixed in 0.7.1.
I had folks 0.7.2.2 installed at that time, and still have that version.
I'm experiencing this crash a lot here at guadec with folks 0.7.2.2 (lt-empathy:32671): GLib-GObject-WARNING **: g_object_weak_unref: couldn't find weak ref 0x7ffff701d3e0(0x3f3a000) Program received signal SIGTRAP, Trace/breakpoint trap. 0x00007ffff3e561e9 in g_logv (log_domain=0x7ffff418704f "GLib-GObject", log_level=G_LOG_LEVEL_WARNING, format=0x7ffff4187e90 "%s: couldn't find weak ref %p(%p)", args1=0x7fffffffcd68) at gmessages.c:758 758 G_BREAKPOINT (); (gdb) bt
+ Trace 230600
Looks like the connection is salut, thay may explain why I'm experiencing this crash much more here.
Created attachment 219868 [details] [review] WIP work to fix contact weak notification So here’s the order of events causing this: 1. The TpContact is finalised, triggering its weak notifications. Before the weak notification callbacks are called, the list of weak notifications is cleared (so g_object_weak_unref() will print a warning). 2. _contact_weak_notify_cb() in Tpf.PersonaStore is called first, and it removes the Persona (corresponding to the TpContact) from various maps. It turns out that the Tpf.PersonaStore holds the last reference to the Persona, so at the end of _contact_weak_notify_cb(), the Persona is finalised. 3. The finalise function of the Persona checks whether Persona.contact != null. This check is true, because the Persona’s _contact_weak_notify_cb() hasn’t yet been called by GLib, since we’re still in a descendent call of the Tpf.PersonaStore’s _contact_weak_notify_cb(). 4. As a result, the Persona calls g_object_weak_unref() on its TpContact, and GLib gets unhappy. I see two ways to fix this: A. Extend the lifetime of the Persona a little longer so that its _contact_weak_notify_cb() is correctly called before the Persona is finalised. This is what the attached patch does, in a horrible and hacky way. B. Explicitly clear the Persona.contact from the Tpf.PersonaStore’s _contact_weak_notify_cb() so that finalisation of the Persona from within _contact_weak_notify_cb() succeeds. Option A seems a little cleaner than option B (to me), but both seem fairly nasty and suggestions are welcome for better solutions. Otherwise, I can tidy up this patch (add a NEWS entry, expand the commenting, etc.) and commit it.
Review of attachment 219868 [details] [review]: Looks good to me. I agree it's not the finest piece of code ever but sometimes that's the easiest way to fix such nasty bug.
I’m wondering if it’s perhaps a GLib bug that g_object_weak_unref() explodes if called from within a weak notification for the same object. I’ll ask around a bit and see if a better solution can be found. If that fails, I’ll commit this patch.
Created attachment 223214 [details] [review] Fix contact weak notification https://www.gitorious.org/folks/folks/commits/680335-contact-weak-notification-attempt2 Here’s a much better attempt. desrt suggested using a GWeakRef in Tpf.Persona to handle setting Tpf.Persona._contact to null when the TpContact is destroyed. This eliminates the race condition. The code’s a bit messy because Tpf.Persona.contact doesn’t return a strong ref., but there’s nothing we can do about that until folks next breaks API. This also includes some associated fixes to ensure folks uses a strong ref. for TpContacts when performing operations on them (e.g. setting their properties).
The code is not that bad, and is documented where it should be changed later on. If this fixes the bug I say ship it.