GNOME Bugzilla – Bug 705570
Check ref_count in g_object_notify_by_pspec
Last modified: 2013-08-14 17:11:58 UTC
g_object_notify is easy on 0 return values, making it easier to skip calls when for one reason or another notify is called during ->finalize(). Would be nice if g_object_notify_by_pspec has this as well, so avoid warning like: g_object_ref: assertion `object->ref_count > 0' failed
I meant 0 ref_counts.
Created attachment 250979 [details] [review] Add refcount check like g_object_notify
Er...you *should* see the warning, since this is not a valid state. I could buy a patch that copied the g_return_if_fail code from g_object_ref() so that we returned and continued to stumble on (but then realistically, we'd most likely crash later).
It is indeed probably better to do the late stuff in dispose, but this case it simple. In my case I have a couple of functions that store files in the object (so a _set_files() function, a property where gobject's set_property PROP_FILES calls this set_files() function. But the function also cleanups the old files, so in finalize() it calls _set_files (foo, NULL) to cleanup. Now _set_files also notifies the "files" property, with g_object_notify this always worked without problems when called from within finalize, but with g_object_notify_by_pspec it crashes because of the zero ref_count. That said, I think its also "expected" g_object_notify and g_object_notify_by_pspec behave the same, but they don't.
(In reply to comment #4) > It is indeed probably better to do the late stuff in dispose, but this case it > simple. In my case I have a couple of functions that store files in the object > (so a _set_files() function, a property where gobject's set_property PROP_FILES > calls this set_files() function. But the function also cleanups the old files, > so in finalize() it calls _set_files (foo, NULL) to cleanup. Ahh, I see. Hmmm, this is kind of a grey area. Ok, I don't see much harm in adding this patch. But of course, your app will still crash with earlier GLib versions... So I think we still need to recommend that authors not invoke the notification machinery in finalize. Can you format this patch using https://live.gnome.org/GnomeLove/SubmittingPatches and add a test case?
Created attachment 251100 [details] [review] Check refcount and a test Haven't compiled it, but it seems to be pretty straightforward.
Review of attachment 251100 [details] [review]: Looks good to me! I'll add a comment to both lines which links to this bug.
(In reply to comment #7) > Review of attachment 251100 [details] [review]: > > Looks good to me! I'll add a comment to both lines which links to this bug. Ok, turns out my hotel is blocking ssh; can someone else do so and push this patch?
Comment on attachment 251100 [details] [review] Check refcount and a test https://git.gnome.org/browse/glib/commit/?id=4b334ef8f1393c997a2d83de4ffe0976dff238c3