GNOME Bugzilla – Bug 650597
clutter-id-pool: fix warning on bad pick
Last modified: 2011-05-26 12:48:54 UTC
https://bugzilla.redhat.com/show_bug.cgi?id=697964 is a crash involving a bad pick that we're having trouble reproducing/debugging. Our best guess currently is that a pick buffer is getting reused when it should not be somehow, thus causing a destroyed actor to be returned. Anyway, clutter's behavior in this case is to cause a crash somewhere down the road, which there's really no good reason for, so let's at least fix that.
Created attachment 188133 [details] [review] clutter-id-pool: fix warning on bad pick Commit 13ac1fe7 purported to extend the _clutter_id_pool_lookup() warning to the case where the id referred to a deleted actor, but did not actually do so, because _clutter_id_pool_remove() set deleted IDs to 0xdecafbad, not NULL. Fix this.
We don't really do pointer poisoning in most places in GNOME, so I guess this is generally right, but it's also not really a substitute for a plan to figure out the actual problem...
Review of attachment 188133 [details] [review]: mmh, I'd rather fix the pool_lookup() to check of 0xdecafbad along with NULL instead.
In the current code nothing ever sets id pool entries to NULL, so there's no point in checking both values. (ie, currently _clutter_id_pool_remove() sets unused entries to one value, and _clutter_id_pool_lookup() checks for a *different* value.) And I can't see any good reason to return 0xdecafbad to callers either; if the goal is to cause a crash, then we should crash right away in _clutter_id_pool_lookup(), rather than returning an invalid pointer that will cause a crash at some indeterminate point in the future. And if we don't want to crash, then we should return NULL and g_warn
I'd vote for setting it to NULL as well. I don't think we want to crash because picking can quite commonly end up with an invalid id due to various bugs and rounding errors in all of the layers and it's an easy error to recover from.
fine by me then, feel free to commit the patch.
Attachment 188133 [details] pushed as e83a785 - clutter-id-pool: fix warning on bad pick