GNOME Bugzilla – Bug 555701
libgnome-desktop:gnome_bg_create_pixmap() leaks pixmaps and X clients
Last modified: 2008-10-18 20:05:16 UTC
Please describe the problem: On my system, after a while, all attempts to open a new X application fail with a message such as 'Maximum number of clients reachedxlsclients: unable to open display ":0.0"' I've tracked this down to an interaction between gnome-screensaver, libgnome-desktop and gdk, which I think is best classified as a bug in libgnome-desktop. Here's the summary of what happens: - gnome-screensaver calls gnome_bg_create_pixmap() from libgnome-desktop with root == TRUE every so often. I haven't tracked down what triggers this yet, but it seems like it should be an OK thing to do, and gnome-screensaver appears to get the pixmap reference counting correct. - When root == true, gnome_bg_create_pixmap() uses make_root_pixmap(), which creates a new X client, does XSetCloseDownMode (display, RetainPermanent) on that client, and creates a pixmap with that client connection. It passes that pixmap into gdk_pixmap_foreign_new(), then closes the client connection and forgets about the native pixmap handle it has. - gdk_pixmap_foreign_new() creates a gdk pixmap with is_foreign set, which means that when the gdk pixmap is destroyed, gdk makes no attempt to free the underlying X pixmap. The net result of this is that every time make_root_pixmap() is called, a new X client is created and leaked, which means that eventually the X server's client table fills up and no more clients can be created. I'm not sure what the best way to fix this really is -- one simple band-aid would be for libgnome-desktop to create a single X client (per process) with CloseDownMode set to RetainPermanent, and use that for all make_root_pixmap() calls. This would still leak pixmaps, and also slowly leak clients if multiple processes use gnome_bg_create_pixmap(), but it would at least solve the immediate issue of gnome-screensaver rendering systems unusable after a while. Perhaps a better (but much more complicated) solution would be for libgnome-desktop to subclass GdkPixmap and have a destructor that frees the underlying X pixmap and kills the X client when pixmap is destroyed. Anyway, I'm not enough of an Xlib expert to know what the right way to handle this is, but I hope this at least explains the issue clearly. Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
By the way, I think the call to gdk_pixmap_foreign_new() in make_root_pixmap() should really be a call to gdk_pixmap_foreign_new_for_screen(), since there's no guarantee that the screen passed into make_root_pixmap() is the default screen.
Created attachment 120352 [details] [review] Proposed patch Attached a possible patch, this simple calls gnome_bg_create_pixmap with root=FALSE so that only gdk_pixmap_new() is called instead of the much more complex (and deadly) make_root_pixmap() function Both resulting pixmaps should have the same window, screen, width, height and colormap - I think.
(Note that the above patch is to gnome-screensaver to avoid the problem, would that patch do the right thing for it?)
Note gnome-desktop does XKillClient to clean up the pixmap. I think your patch is right, though. gnome-screensaver is probably using the api wrong.
Re #2: If that patch to gnome-screensaver still works, then it makes sense to use that. (And I see Ubuntu's latest gnome-screensaver has it, so we'll soon know if it causes any other problems) Re #4: Do you mean that someone calling gnome_bg_create_pixmap() with root == TRUE should only pass that GdkPixmap to gnome_bg_set_pixmap_as_root() and any other use is a bug? Because I can't see any other way that the pixmap would get cleaned up (without serious layering violations using knowledge about libgnome-desktop's internal implementation). Also, it seems that any use of gnome_bg_create_pixmap(), even when followed by gnome_bg_set_pixmap_as_root(), is going to leak one X client, since nothing does XKillClient until the next call to gnome_bg_set_pixmap_as_root(). And the use of XKillClient there seems fragile, since nothing ties the reference count of the GdkPixmap to when the underlying X pixmap is freed. Finally, I think my comment #1 is still correct: it really seems the gdk_pixmap_foreign_new() in make_root_pixmap() should be gdk_pixmap_foreign_new_for_screen().
gnome_bg_set_pixmap_as_root was written to be paired with gnome_bg_create_pixmap / root == TRUE . The api could probably be improved, but I believe it was mainly made to factor out duplicate code between nautilus and gnome-settings-daemon. I don't think it was really designed to be a general purpose api. It's not stable api either, so we could improve it. CCing Søren since it's his api. One pixmap isn't really leaked, it's more like disowned. That's the typical X model for setting a root window pixmap. The xid of the pixmap is stored as property on the root window, so the next client that wishes to change the root window pixmap handles clean up. The property name is de facto standard, so things like xsetroot, xloadimage, etc all play along. You can think of the remaining pixmap reference as floating. gdk_pixmap_foreign_new is fine. It doesn't use the default screen, it uses the screen of the underlying pixmap. Using gdk_pixmap_foreign_new_for_screen would be an optimization that would save a server roundtrip though.
Thanks for the reply. I see now about the way the pixmap is freed. Re: > gdk_pixmap_foreign_new is fine. It doesn't use the default screen, it uses the > screen of the underlying pixmap. Looking at the gdk source I don't see that. We have: GdkPixmap* gdk_pixmap_foreign_new (GdkNativeWindow anid) { return gdk_pixmap_foreign_new_for_display (gdk_display_get_default (), anid); } and indeed I don't know how to get a display from a pixmap (which after all is just an arbitrary 32-bit ID). Maybe it's all theoretical, since probably no one ever uses this code with multiple displays, but I don't think this code would work right if you were trying to talk to multiple X servers.
Right it uses the default display, but not the default screen. The default display is normally fine unless for some reason you have more than one GdkDisplay in play, which is pretty rare, and is the result of code the app author does, not the result of the way the user has their computer configured. When you create a pixmap with XCreatePixmap you have to pass a drawable to the function to say which Screen the pixmap should be associated with. Why it doesn't just take a Screen, I don't know. A common drawabble to pass to this function is RootWindowOfScreen (the_screen_to_go_with) When you query the dimensions of a drawable with XGetGeometry, the root window of the screen the drawable is associated with is returned. From the root window you can determine the screen. That's what gtk does. Again, though, using the gdk_pixmap_foreign_new_for_screen isn't a bad idea. It saves the XGetGeometry call.
Created attachment 120608 [details] [review] Use gdk_pixmap_foreign_new_for_screen() in make_root_pixmap() Maybe my terminology is sloppy, but it does seem to me that gdk_pixmap_foreign_new() is not multihead-safe. Probably system/application situations where that matters are very rare and no such apps call make_root_pixmap(). Anyway we seem to agree that using gdk_pixmap_foreign_new_for_screen() is a good idea for whatever reason, so I'm attaching a patch to do that.
Committed the gnome-screensaver part. Thanks! Re-assigning to gnome-desktop for the rest.
I basically agree with Ray that there is no bug, but that using the _from_foreign() version is better in principle if not practice, so feel free to cmmit.
Sorry to keep belaboring the point, but I'll just note that <gdk/gdkpixmap.h> defines gdk_pixmap_foreign_new inside #ifndef GDK_MULTIHEAD_SAFE, which means (as the GTK+ changelog says) that the function is "inherently non-multihead safe." So make_root_pixmap() also is "inherently non-multihead safe." So any application that has multiple GdkDisplays in use and that calls make_root_pixmap() will get incorrect results. Maybe "bug" is too strong a word for the issue with the current code, but it is certainly at least an undocumented booby trap set for applications (albeit one that very few applications are likely to step into). And happily one that can be fixed in a way that makes the code minutely more efficient.
+Sat Oct 18 16:03:10 2008 S�ren Sandmann <sandmann@redhat.com> + + * gnome-bg.c (make_root_pixmap): Use foreign_new_for_screen() + instead of _foreign_new(). Bug 555701, patch from Roland Dreier. +