! Please note that this is a snapshot of our old Bugzilla server, which is read only since May 29, 2020. Please go to gitlab.xfce.org for our new server !
Disconnect from DBus in dispose
Status:
RESOLVED: FIXED
Product:
Xfconf
Component:
Libxfconf

Comments

Description Nick Schermer editbugs 2009-06-22 09:20:17 CEST
It would be nice if the dbus_g_proxy_disconnect_signal() calls in xfconf_channel_finalize() are done in xfconf_channel_dispose(). This allows the following usage of XfconfChannel (which I'd like to use to centralize the xfconf code in the panel):

if (xfconf_init (NULL)) {
  channel = xfconf_channel_new ("foo");
  g_object_weak_ref (G_OBJECT (channel), (GWeakNotify) xfconf_shutdown, NULL);
}

Currently this spawns errors because the weak refs are executed between dispose and finalize and so _xfconf_get_dbus_g_proxy() will complain.

Duno if you can count those signals as "drop all references to other objects" which is what dispose should do. Also: "It may be run multiple times (due to reference loops)" is not really the case here, but if it would occur, dbus-glib will only spawn a warning [1].

Anyway, any thoughts about this? I have a patch and it runs fine here (will attach it later today).

[-] http://library.gnome.org/devel/gobject/unstable/gobject-The-Base-Object-Type.html#GObjectClass
[1] http://www.google.com/codesearch/p?hl=en&sa=N&cd=1&ct=rc#rWC-u-FD0gU/dbus-glib-0.72/dbus/dbus-gproxy.c&q=dbus_g_proxy_disconnect_signal
Comment 1 Nick Schermer editbugs 2009-06-22 16:25:53 CEST
Created attachment 2412 
Disconnect dbus signals during dispose
Comment 2 Brian J. Tarricone (not reading bugmail) 2009-09-05 04:00:15 CEST
Sure, looks fine.  I added a bool "disposed" member to the instance struct to be sure dispose doesn't get run twice.  Can't hurt, anyway... Mem usage is the same since I added the bool to a bitfield.
Comment 3 Brian J. Tarricone (not reading bugmail) 2009-09-05 04:00:28 CEST
Er, mark fixed...
Comment 4 Nick Schermer editbugs 2009-09-05 07:18:04 CEST
The parent class call should not be in the boolean dispose check (and you could set channel->cache to null and check for that).

Anyway, since the cache entered master there is more going on here. You also need to release the cache's dbus proxy in dispose and when using something like this for bindings:

xfconf_init(NULL);
channel = xfconf_get_channel(...)
g_object_weak_ref (G_OBJECT (channel), (GWeakNotify) xfconf_shutdown, NULL);

/* ... connect some bindings */
g_object_weak_ref (G_OBJECT (object), (GWeakNotify) g_object_unref, channel);

The bindings also cause problems and segfault. Anyway, I'm trying to fix all this and will attach a new patch once it's all fixed.
Comment 5 Brian J. Tarricone (not reading bugmail) 2009-09-05 07:25:34 CEST
(In reply to comment #4)
> The parent class call should not be in the boolean dispose check

You sure about that...?
> (and you could set channel->cache to null and check for that).

Eh, doesn't matter...

> Anyway, since the cache entered master there is more going on here. You also
> need to release the cache's dbus proxy in dispose

The cache doesn't own a proxy.

> and when using something like this for bindings:
> 
> xfconf_init(NULL);
> channel = xfconf_get_channel(...)
> g_object_weak_ref (G_OBJECT (channel), (GWeakNotify) xfconf_shutdown, NULL);
> 
> /* ... connect some bindings */
> g_object_weak_ref (G_OBJECT (object), (GWeakNotify) g_object_unref, channel);

Not sure what you mean here.

> The bindings also cause problems and segfault. Anyway, I'm trying to fix all
> this and will attach a new patch once it's all fixed.

Backtrace?
Comment 6 Nick Schermer editbugs 2009-09-05 07:48:58 CEST
(In reply to comment #5)
> (In reply to comment #4)
> > The parent class call should not be in the boolean dispose check
> 
> You sure about that...?

Yep, gobject is not (possibly) running it multiple times so you can block it ;-)

> > Anyway, since the cache entered master there is more going on here. You also
> > need to release the cache's dbus proxy in dispose
> 
> The cache doesn't own a proxy.

Maybe now you release the cache early it works fine, haven't tried it yet, but there are signals connected to the proxy.

If you really want to fix this, try the devel panel and uncomment the lines in common/panel-xfconf.c. That will sprew all the warnings and crashes ^_^. IIRC the backtraces were not very useful, will try to get you some later today. If you think what i do in the panel is wrong, I'd also like to hear that of course.
Comment 7 Brian J. Tarricone (not reading bugmail) 2009-09-05 08:39:20 CEST
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> 
> > > Anyway, since the cache entered master there is more going on here. You also
> > > need to release the cache's dbus proxy in dispose
> > 
> > The cache doesn't own a proxy.
> 
> Maybe now you release the cache early it works fine, haven't tried it yet, but
> there are signals connected to the proxy.

What signals?  What proxy?

The only dbus signals I see are connected in XfconfCache::init(), and are disconnected in XfconfCache::finalize().  Since XfconfChannel drops its reference on cache in XfconfChannel::dispose(), all dbus signals XfconfCache is connected to should be disconnected by the time XfconfChannel::dispose() is finished.

The only thing I can think of is something to do with the singleton channels created with xfconf_channel_get(), but still I don't see how that would cause a problem since xfconf_shutdown() will destroy those channels before shutting down.  It also 'breaks' all bindings as well (before destroying the channel singletons), so that kills any (GObject) signals connected by those.

> If you really want to fix this, try the devel panel and uncomment the lines in
> common/panel-xfconf.c. That will sprew all the warnings and crashes ^_^. IIRC
> the backtraces were not very useful, will try to get you some later today. If
> you think what i do in the panel is wrong, I'd also like to hear that of
> course.

I don't really have time to play with the new panel right now.  Backtraces with G_DEBUG=fatal_warnings would definitely be useful.
Comment 8 Nick Schermer editbugs 2009-09-08 09:06:35 CEST
Created attachment 2532 
Increase ref count when returning a channel singleton

Part of the problem was that all the singleton channels had a refcount of 1. Attached patch fixes this.
Comment 9 Nick Schermer editbugs 2009-09-08 09:15:59 CEST
Comment on attachment 2532 
Increase ref count when returning a channel singleton

Err sorry, I should've read the api doc of xfconf_channel_get().
Comment 10 Nick Schermer editbugs 2009-09-08 15:53:22 CEST
Well most of this is fixed in the panel now. Bindings still give some trouble but I've simplified the binding code in xfconf. With those changes applied the panel works flawless, so that's good news.

I'll open a new bug when the changes I made in xfconf have been fully tested. Probably with a test suite attached.

Bug #5493

Reported by:
Nick Schermer
Reported on: 2009-06-22
Last modified on: 2009-09-08

People

Assignee:
Brian J. Tarricone (not reading bugmail)
CC List:
0 users

Version

Version:
GIT Master

Attachments

Additional information