+++ This bug was initially created as a clone of Bug #36793 +++ If you do this: * create a GObject * export it on D-Bus (at n >= 1 object paths) * unref the object enough times that it is freed there should be no leak. What actually happens is: * each object registration "o" has a weak ref which results in a call to object_registration_object_died * object_registration_object_died sets o->object to NULL * object_registration_object_died calls dbus_connection_unregister_object_path * this results in a call to object_registration_unregistered * which calls object_registration_free * which does not remove the dbus_glib_object_registrations qdata from the object, because the object has already been freed at this point so it's too late (and also because o->object is NULL) Because the object registrations are already qdata, we shouldn't really need a weak ref at all...
Created attachment 46316 [details] [review] [PATCH 1/5] dbus-gobject: wrap the GSList of registrations in a simple struct For simplicity, the ObjectExport struct isn't freed until the object is actually destroyed, even if there are no more registrations.
Created attachment 46317 [details] [review] [PATCH 2/5] dbus_g_connection_register_g_object: attach ObjectExport first This means we no longer need the unnecessarily subtle "steal and put back" pattern.
Created attachment 46318 [details] [review] [PATCH 3/5] dbus_g_connection_register_g_object: only hook onto signals on first use This fixes a bug in which an empty list of registrations was considered to be synonymous with the object never having been exported, resulting in this failure mode: * register object at n locations - the first time, export_signals() is called * unregister all of those locations * register object at a new location - export_signals() is wrongly called again * emit a signal - the D-Bus signal gets emitted twice
Created attachment 46319 [details] [review] [PATCH 4/5] Add a regression test for removing all registrations and starting again This has been confirmed to fail when the previous commit is reverted.
Created attachment 46320 [details] [review] [PATCH 5/5] dbus-gobject: move weakref to ObjectExport, with ObjectRegistration pointing there This has the following results: * each exported object only needs one weak ref to itself, however many times it is registered * because the ObjectRegistration now points to the ObjectExport, it can always remove itself from the list of registrations even if the actual object has been disposed, avoiding a leak (fd.o #36811)
Review of attachment 46316 [details] [review]: Not a reviewer, but seems sane to me. ::: dbus/dbus-gobject.c @@ +2507,3 @@ GObject *object) { + ObjectExport *oe = g_object_get_data (object, "dbus_glib_object_registrations"); Does g_object_get_data not complain when @object is not a GObject? Although, it wasn't checked before also. @@ +2567,3 @@ * the object. */ + oe = g_object_steal_data (object, "dbus_glib_object_registrations"); + As before, @object is not checked and it's a public method. It does not change much though, as there will be some assertion in _steal_data doing the same.
Review of attachment 46317 [details] [review]: Not a reviewer, but OK for me
Review of attachment 46318 [details] [review]: Again, no hat, but sane to me
Review of attachment 46319 [details] [review]: test OK
Review of attachment 46320 [details] [review]: OK to me
(In reply to comment #6) > Does g_object_get_data not complain when @object is not a GObject? > > Although, it wasn't checked before also. ... > As before, @object is not checked and it's a public method. You're right, that'd be an improvement, but it's not a regression, and I think one of my other branches might fix it already? Any thoughts on the patches I did for the two bugs blocking this one?
> Any thoughts on the patches I did for the two bugs blocking this one? All fine. Comments published for each bugs.
Created attachment 46538 [details] [review] [bonus patch] dbus-gobject: check various preconditions for methods Cosimo pointed out that there was a missing precondition check, although that wasn't a regression. I went through all of the public API in dbus-gobject.c adding such checks.
Fixed in git for 0.94, based on review from Cosimo and no objections from the reviewer group.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.