GNOME Bugzilla – Bug 683267
[new roster] Warning when starting in non group mode
Last modified: 2012-09-10 07:25:05 UTC
./tests/interactive/test-empathy-roster-model-aggregator (lt-test-empathy-roster-model-aggregator:24017): GLib-WARNING **: Accessing a sequence while it is being sorted or searched is not allowed Program received signal SIGTRAP, Trace/breakpoint trap. 0x00007ffff3deb1e9 in g_logv (log_domain=0x7ffff3e71589 "GLib", log_level=G_LOG_LEVEL_WARNING, format=0x7ffff3e71540 "Accessing a sequence while it is being sorted or searched is not allowed", args1=0x7fffffff8ee8) at gmessages.c:758 758 buf[i] = c + 'a' - 10; (gdb) bt
+ Trace 230787
This is a side effect of bug #682809 : - A contact is added, Empathy creates a widget for it - EggListBox asks to Empathy to sort the widget - Empathy asks Folks for the contact's groups - Folks sets the group and fire the "group-changed" signal - Empathy asks to EggListBox to resort the widget I'm not sure what's the best way to fix this. I guess EggListBox could be more careful not trying to resort twice the same item?
Philip, Alexander: thoughts?
(In reply to comment #1) > I'm not sure what's the best way to fix this. I guess EggListBox could be > more careful not trying to resort twice the same item? That seems like the simplest and most robust solution.
Its not generally workable to have the sort comparison operation actually change the data in the container being sorted. Just ignoring the second call to sort will result in an incorrectly sorted state. I don't think its ever right to reenter in this fashion inside a simple getter function. If you really want to read the item data incrementally like that it needs to be done in an idle, not by reentering. Sorting is just one case of the sort of problems this reentrancy can cause. Any code that calls a getter during modification of internal state which is based on folks will break when the internal state is unexpectedly modified in the middle of an operation.
*** Bug 683454 has been marked as a duplicate of this bug. ***
I agree with Alexander, firing signals in a getter function isn't great. If anything, it should be fired in an idle. Re-assigning to Folks.
Philip, One fix for this would be to not send the group changed signal when lazy loading groups, would that cause any issue that you can foresee? Alternatively we could make groups not lazy load. Either way we need a fix for this in order for 0.7.4 to be ready to release imo.
(In reply to comment #7) > Either way we need a fix for this in order > for 0.7.4 to be ready to release imo. Agreed. This should block the release. > One fix for this would be to not send the group changed signal when lazy > loading groups, would that cause any issue that you can foresee? That shouldn’t cause any issues (it’s what we were doing before my branch was merged). As discussed on IRC, I suggest passing a second boolean to _update_groups() which specifies whether a notification may be emitted.
Created attachment 223717 [details] [review] Added emit_notification parameter to _update This should fix the bug, I am able to sign in and out repeatedly without issue here (but I can do the same with master, so not quite sure if this fixes the issue tbh).
Review of attachment 223717 [details] [review]: Almost. This also needs to be done in edsf-persona.vala and tpf-persona.vala as per http://git.gnome.org/browse/folks/commit/?id=57f84d50f5fcaf3bf821d368a7ff309bc2edb3d2 and http://git.gnome.org/browse/folks/commit/?id=eb75447e8eb6a95aad8f95ee19a2cd4a945fefa1. ::: folks/individual.vala @@ +1440,3 @@ { /* Notify and return. */ + if (create_if_not_exist == false && emit_notification) This isn’t right. If (create_if_not_exist == false) but (emit_notification == false) then the method won’t return immediately like it does without the patch. @@ +1464,3 @@ * changed — we can't be sure, but emitting is a safe over-estimate) and * return. */ + if (this._groups == null && create_if_not_exist == false && emit_notification) As above, this won’t return immediately if (create_if_not_exist == false && emit_notification == false). @@ +1518,3 @@ this._groups.remove (group); + if (emit_notification) + this.group_changed (group, false); Please put braces around single-line ‘if’ blocks.
Created attachment 223718 [details] [review] Added emit_notification parameter to _update methods Ah, right. Ok I believe I got all the create_if_not_exist methods this time.
Review of attachment 223718 [details] [review]: Looks good to commit after making the fix below. Don’t forget to update NEWS. Thanks! ::: backends/eds/lib/edsf-persona.vala @@ +1728,3 @@ } + if (in_google_personal_group != this._in_google_personal_group && + emit_notification) If (in_google_personal_group != this._in_google_personal_group) but (emit_notification == false), the _in_google_personal_group member will not be updated.
It works; thanks for the fix!