GNOME Bugzilla – Bug 729150
GWeakRefs not cleared by g_object_run_dispose()
Last modified: 2018-05-24 16:29:14 UTC
As the GWeakRef gtk-doc describes: > Before the object's GObjectClass.dispose method is called, every GWeakRef associated with becomes empty (i.e. points to NULL). But that doesn't actually happen for g_object_run_dispose(). I've been working on fixing a race condition in bug #726973 and tried solving it with GWeakRefs. My patch to gdbusnamewatching.c did nothing until I added weak ref clearing to g_object_run_dispose().
Created attachment 275384 [details] [review] Clear out weak refs in g_object_run_dispose() This clears the weak refs before calling the object's dispose() vfunc. I think I've got the locking correct, but it could certainly use a little careful examination from someone more familiar with the codebase. This is based on the code from g_object_unref() but it leaves out the consideration of changes to the ref_count (on the assumption that we can ignore them since we're in the process of disposal, which g_object_dispose() seemed to assume already) and anything to do with toggle refs for the same reason (though I'm not terribly familiar with toggle refs).
Thanks to Simon McVittie for pointing out this inconsistency between the docs and behavior.
I'm not sure this is the right thing to do -- particularly if the object in question is GDBusConnection...
For context, our original analysis of this part of the bug was here: <https://bugzilla.gnome.org/show_bug.cgi?id=726973#c29> <https://bugzilla.gnome.org/show_bug.cgi?id=726973#c30>
(In reply to comment #3) > I'm not sure this is the right thing to do -- particularly if the object in > question is GDBusConnection... If you mean "I'm not sure [calling g_object_run_dispose()] is the right thing to do" then yes, I basically agree; but it's part of the API whether we like it or not, and GTestDBus calls it during teardown. If you would prefer to amend the documentation so GWeakRef is conceptually tied to "object has reached 0 refs, even for a moment" rather than "object has been disposed", I'd be OK with that, but we'd still need a solution to Bug #729152, probably using the older non-thread-safe/arbitrary-callback weak refs which *do* run from g_object_run_dispose(). However, I think the way it's documented makes more sense than the way it's implemented - if g_object_run_dispose() is going to run destroy-notifications and the callbacks for non-thread-safe weak refs (which it does, because those are in g_object_real_dispose()), then it should also invalidate thread-safe weak refs, IMO. The fact that it didn't was an implementation error, and was probably my fault - I missed g_object_run_dispose() when looking for ways in which weak refs could be invalidated.
I reopened bug 711807 because I want to stop calling dispose() in GTestDBus teardown (since it is causing additional troubles there).
(In reply to comment #5) > However, I think the way it's documented makes more sense than the way it's > implemented - if g_object_run_dispose() is going to run destroy-notifications > and the callbacks for non-thread-safe weak refs (which it does, because those > are in g_object_real_dispose()), then it should also invalidate thread-safe > weak refs, IMO. The fact that it didn't was an implementation error, and was > probably my fault - I missed g_object_run_dispose() when looking for ways in > which weak refs could be invalidated. I agree. Ryan, do you have any specific ideas for fixing bug #711807?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/865.