GNOME Bugzilla – Bug 661576
fix handling of constructors that destroy half-constructed objects
Last modified: 2014-02-15 15:22:09 UTC
slightly weird... I have a class that overrides constructor(), chains down, and then looks at the constructed object to see if it "matches" any of the objects in its cache. If it does match, then the newly-created object is unreffed, and the constructor() returns the cached object (adding a ref) instead. (It would be possible in theory to skip the chain-down-construct-unref step, and just process all the construct params by hand to figure out whether or not this object was going to end up matching another one... but that would be annoying, so I didn't.) Anyway, in this case, the infanticided object ends up getting left behind on gobject.c's construction_objects list, which will cause problems later when that memory location gets reused for a different object. (In particular, when I try to return that new object from the cache, I get: GLib-GObject-CRITICAL **: g_object_notify_queue_thaw: assertion `nqueue->freeze_count > 0' failed because gobject thinks it's newly-constructed now, because it was on the list.) Even if the answer is "you can't do that", maybe it's worth throwing an error when the infanticide occurs, rather than non-deterministically throwing one later?
Created attachment 198870 [details] [review] gobject: fix handling of constructors that destroy half-constructed objects If a GObject constructor implementation chains down to create an object, but then destroys it and returns a different object instead, then the object would get left in the construction_objects list, which would confuse things if another object was created later at the same location in memory.
Created attachment 242917 [details] [review] gobject: fix a crash when constructor() disposes the object it creates If a constructor() implementation created an object but then didn't return it, that object would get left on the construction_objects list, which would cause problems later when that memory location got reused by another object. Fix this by keep the "in construction" state in the object's qdata rather than in a global variable. Add some tests to verify that this case and the more common constructor-returning-a-singleton case work, and another test verifying that construct-only properties work as expected. ===== (inspired by bug 698983...)
Review of attachment 242917 [details] [review]: I like this other than the fact that we're fixing bugs in a broken behaviour during the same release cycle as we officially declare it as "this has always been broken, don't use it". In general, I don't want to increase the incentive to use constructor() more.... That said, this patch is very obviously an improvement. In fact, the only bad thing I can say about it is that it might encourage more people to use this code... ::: gobject/gobject.c @@ +1641,3 @@ { + g_critical ("Custom constructor for class %s returned NULL (which is invalid)." + "Please use GInitable instead.", G_OBJECT_CLASS_NAME (class)); thanks for keeping this...
Created attachment 242963 [details] [review] to be squashed: forbid finalization-during-construction As an alternative to the previous patch, we could make finalization-during-construction intentionally fail, rather than having it merely accidentally fail. (The rest of the previous patch is still worth applying, since it makes the in-construction handling nicer.)
Created attachment 257758 [details] [review] gobject: forbid finalization-during-construction If a constructor() implementation created an object but then unreffed it rather than returning it, that object would get left on the construction_objects list, which would cause problems later when that memory location got reused by another object. "Fix" this by making it fail intentionally, and add a test for it (and for the normal, working singleton case).
Created attachment 257759 [details] [review] gobject: simplify object-in-construction handling Rather than keeping a global list of objects that are being constructed, use qdata on the object itself like we do with several other properties now.
Review of attachment 257758 [details] [review]: I'm pretty happy with this. Thanks for formalising it.
Review of attachment 257759 [details] [review]: I don't have a strong opinion on this topic because this code is so rarely used. One less mutex with complicated list-mangling code is good, I guess. Commit it if you prefer it this way.
Attachment 257758 [details] pushed as 0d62eb4 - gobject: forbid finalization-during-construction Attachment 257759 [details] pushed as efecfe0 - gobject: simplify object-in-construction handling
this is turning out to have some unintended fallout
Created attachment 263315 [details] [review] gobject: re-allow finalization from constructor() Although returning NULL from constructor is strongly discouraged, some old libraries need to keep doing it for ABI-compatibility reasons. Given this, it's rude to forbid finalization from within constructor(), since it would otherwise work correctly now anyway (and the critical when returning NULL should discourage any new uses of returning NULL from constructor()).
Review of attachment 263315 [details] [review]: ::: gobject/gobject.c @@ -1028,3 @@ - if (object_in_construction (object)) - { - g_error ("object %s %p finalized while still in-construction", Can we just change this to a g_critical()? I'd rather keep both warnings.
ok, pushed with the g_error() changed to g_critical() rather than being removed entirely Attachment 263315 [details] pushed as 5cab3fc - gobject: re-allow finalization from constructor()