After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 705570 - Check ref_count in g_object_notify_by_pspec
Check ref_count in g_object_notify_by_pspec
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gobject
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-08-06 14:24 UTC by Nick Schermer
Modified: 2013-08-14 17:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add refcount check like g_object_notify (463 bytes, patch)
2013-08-06 14:54 UTC, Nick Schermer
none Details | Review
Check refcount and a test (1.71 KB, patch)
2013-08-07 19:09 UTC, Nick Schermer
committed Details | Review

Description Nick Schermer 2013-08-06 14:24:38 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
Comment 1 Nick Schermer 2013-08-06 14:25:19 UTC
I meant 0 ref_counts.
Comment 2 Nick Schermer 2013-08-06 14:54:09 UTC
Created attachment 250979 [details] [review]
Add refcount check like g_object_notify
Comment 3 Colin Walters 2013-08-06 15:02:01 UTC
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).
Comment 4 Nick Schermer 2013-08-06 18:11:46 UTC
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.
Comment 5 Colin Walters 2013-08-07 12:38:55 UTC
(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?
Comment 6 Nick Schermer 2013-08-07 19:09:28 UTC
Created attachment 251100 [details] [review]
Check refcount and a test

Haven't compiled it, but it seems to be pretty straightforward.
Comment 7 Colin Walters 2013-08-08 09:21:06 UTC
Review of attachment 251100 [details] [review]:

Looks good to me!  I'll add a comment to both lines which links to this bug.
Comment 8 Colin Walters 2013-08-09 09:14:16 UTC
(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?