GNOME Bugzilla – Bug 657680
Ensure that persona sets are immutable to clients
Last modified: 2012-07-23 23:56:49 UTC
This is reproducible 1/3 or 1/4 of the times: $ folks-inspect > personas ** ERROR:hashmap.c:2322:gee_hash_map_node_iterator_next: assertion failed: (self->_stamp == self->_map->priv->_stamp) Program received signal SIGABRT, Aborted. 0x00000033a5c352d5 in __GI_raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 64 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig); (gdb) bt
+ Trace 228265
Assuming you're running the `personas` command as soon as you're starting folks-inspect, I suspect this is crashing because the individual whose personas are being listed at that moment has their list of personas modified, this invalidating folks-inspect's iterator. This is basically the same problem as covered in bug #653233: we should be replacing the container properties of objects (such as sets of personas) with new ones, rather than clearing and re-filling them when they change. That way, we don't interfere with clients who already have a pointer to the existing set. (In contrast, bug #653233 concerns the opposite direction.)
(In reply to comment #1) > Assuming you're running the `personas` command as soon as you're starting > folks-inspect, I suspect this is crashing because the individual whose personas > are being listed at that moment has their list of personas modified, this > invalidating folks-inspect's iterator. Yup - that is exactly the case.
(Punting bugs to 0.6.4)
Created attachment 196965 [details] [review] Request for comment patch Have you considered alternate approaches? I was thinking we could, for efficiency's sake, just return a freshly created Set of references for the public getter (in the case of Folks.Individuals#personas we'd return a new Set<Personas>). By doing so we'll only be incurring in extra costs when the getter is accessed, instead of instantiating a fresh new Set<> every time we have to deal with a related signal. The attached patch is just an RFC attempt, though it fixes the problem of folks-inspect crashing on the first run. One problem of this approach is that we'd be changing the ownership transfer for these getters.
(In reply to comment #4) > The attached patch is just an RFC attempt, though it fixes the problem of > folks-inspect crashing on the first run. One problem of this approach is that > we'd be changing the ownership transfer for these getters. Changing the ownership transfer for the getters is an API break, and a particularly nasty one which I think would be best to avoid if possible. Do you have any figures for how many times (e.g.) the getter for IndividualAggregator.individuals is called compared to the number of times we modify that Set in response to related signals? It would be good to know the numbers so we're not just guessing about which case to optimise.
(mass bump to gnome-3.4 milestone in our new milestone scheme; search this string mass move bug mail)
(In reply to comment #5) > (In reply to comment #4) > > The attached patch is just an RFC attempt, though it fixes the problem of > > folks-inspect crashing on the first run. One problem of this approach is that > > we'd be changing the ownership transfer for these getters. > > Changing the ownership transfer for the getters is an API break, and a > particularly nasty one which I think would be best to avoid if possible. > > Do you have any figures for how many times (e.g.) the getter for > IndividualAggregator.individuals is called compared to the number of times we > modify that Set in response to related signals? It would be good to know the > numbers so we're not just guessing about which case to optimise. I'll second this (in an effort to bump the attention on this bug). Raul isn't the only person running into this now (though I haven't seen it myself).
Comment on attachment 196965 [details] [review] Request for comment patch Rejecting patch, as it’s out of date and served its purpose as an RFC.
Didn't make gnome-3.4 release, punting to gnome-3.6 milestone
Philip, Is the best solution to this bug to make an api change as Raul suggested with his rfc patch? if so I'll set the milestone to the next abi break and get a new branch ready for this.
(In reply to comment #10) > Philip, > > Is the best solution to this bug to make an api change as Raul suggested with > his rfc patch? if so I'll set the milestone to the next abi break and get a > new branch ready for this. Some random thoughts: • Breaking API is bad. • Atomically replacing the object instance behind IndividualAggregator.individuals would work, but would incur a lot of extra allocations/deallocations. Folks already has a problem with memory overhead, so I don’t think we should be making it worse. • This behaviour looks very bad. • However at the time, folks-inspect ran the IndividualAggregator in a second thread, and accessed its members from the main thread. As shown with the recent locking changes, we don’t really support this access model at all. • folks-inspect now uses a single thread, so I don’t think this bug will actually be reproducible (please correct me if I’m wrong). As such, I suggest we close it as INVALID. Thoughts?
Agreed, closing now.