|
Description
Simon McVittie
2011-05-03 10:50:01 UTC
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.