While working on icon support in the ewmh branch I came across what I think is a memory leak with icons that change depending on a window's title.
Here is the scenario as I think it is supposed to work. There is this
method to choose icons for windows:
Icons { win-list }
This variable specifies a list of window names and the bitmap
filenames that should be used as their icons. For example:
Icons
{
"XTerm" "xterm.icon"
"xfd" "xfd_icon"
}
Windows that match `XTerm' and would not be iconified by
unmapping, would try to use the icon bitmap in the file
`xterm.icon'.If ForceIcons is specified, this bitmap will be
used even if the client has requested its own icon pixmap.
In the function CreateIconWindow() in icons.c, it searches that list and
potentially finds a match. If so, it creates the mentioned icon and,
some time later, puts that icon on tmp_win->iconslist.
if (pattern != NULL)
AddToList (&tmp_win->iconslist, pattern, (char*) icon);
In fact it looks at the list twice. First if ForceIcons is specified.
If after that no icon is determined yet, it looks at WM_HINTS and
_NET_WM_ICON for an icon. If still not found, it searches the list
if it hadn't done so before (i.e. if ForceIcons is not specified).
Some time later, the function RedoIcon() in events.c may be called. That
happens when properties XA_WM_ICON_NAME or XA_WM_NAME changes the name.
In RedoIcon() it does some lookup again in the same list, using the new
name. If it finds no match, it is done.
If there is a match, it looks in the tmp_win->iconslist for a cached
icon. If found, it overwrites the window's icon with a new one.
If no cached icon was found, it calls CreateIconWindow() again.
When a window is eventually deleted, DeleteIconsList() in icons.c is
called. There is a comment there
/*
* Only the list itself needs to be freed, since the pointers it
* contains point into various lists that belong to Scr.
*/
However...
RedoIcon() makes various assumptions which may be false.
1. The icon that is displaced is already on the iconslist (so that it
will eventually be cleaned up).
This may not be true, if ForceIcons is not specified, and the window
supplies its own icon (via _NET_WM_ICON or WM_HINTS property).
Note that a window may certainly in the Icons { win-list }. In that
case the list search at the start of RedoIcon() may still succeed.
If this assumption is not true, the displaced icon leaks.
2. The new icon created by CreateIconWindow() is always placed on the
iconslist.
This may not be true, in the same cases as with 1.
If this assumption is not true, the displaced icon won't be found
again in a later call to RedoIcon(), won't be re-used, and may leak
at that time.
DeleteIconsList(), likewise.
3. The comment about not needing to free the Icons is not entirely true.
The Images that are pointed to are indeed on the Scr->ImageCache
(done by utils.c:GetImage()).
But since CreateIconWindow() always malloc()s an Icon, the Icon
structures itself do need to be freed, and any Windows it refers to
too.
I used this to start an xterm, then if you start Vim inside it will
change the title and the second line will match.
Icons {
"xterm" "xpm:xterm.xpm"
"* - VIM" "xpm:spider.xpm"
}
Am I overlooking something or is this indeed leaky?
Aside: When trying this out I noticed that the OtpCheckConsistencyVS()
check also fails in an interesting place:
assertion "((owl->type == IconWin) && (owl == twm_win->icon->otp)) ||
((owl->type == WinWin) && (owl == twm_win->otp))" failed: file "otp.c",
line 234, function "OtpCheckConsistencyVS"
That's probably the old, displaced icon which is not not "the icon" of
its window any more.
-Olaf.
--
___ Olaf 'Rhialto' Seibert -- The Doctor: No, 'eureka' is Greek for
\X/ rhialto/at/xs4all.nl -- 'this bath is too hot.'
pgptZPYH7t6dy.pgp
Description: PGP signature
