410215 - GTK's .xpm decoding is weird, stop using XPM as the default window icon

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Justin Dolske [:Dolske]]

While looking at Firefox's disk IO in bug 410131, I noticed some bizarre behavior when opening the Download Manager window (or any window, really).

The process makes 1096 read() calls in a row, from objdir/dist/bin/chrome/icons/default.xpm. The first 885 are 8K reads, then they start dropping by 16 bytes per read until the final read of 4817 bytes. The file size (default.xpm) is 22346 bytes.

I grabbed the call stack for these reads with DTrace:

      libc.so.1`_read+0x15
      libc.so.1`getc+0x6c
      libpixbufloader-xpm.so`file_buffer+0x112
      libpixbufloader-xpm.so`pixbuf_create_from_xpm+0x151
      libpixbufloader-xpm.so`gdk_pixbuf__xpm_image_load+0x3f
      libgdk_pixbuf-2.0.so.0.1000.12`_gdk_pixbuf_generic_image_load+0x3d
      libgdk_pixbuf-2.0.so.0.1000.12`gdk_pixbuf_new_from_file+0x102
      libwidget_gtk2.so`nsWindow::SetWindowIconList()+0x9c
      libwidget_gtk2.so`nsWindow::SetDefaultIcon()+0x1e2
      libwidget_gtk2.so`nsWindow::NativeCreate()+0x8f8
      libwidget_gtk2.so`nsWindow::Create()+0x4a
      libnsappshell.so`nsWebShellWindow::Initialize()+0x24a
      libnsappshell.so`nsAppShellService::JustCreateTopWindow() +0x286
      libnsappshell.so`nsAppShellService::CreateTopLevelWindow()+0x57
      libnsappshell.so`nsXULWindow::CreateNewChromeWindow()+0x167
      libnsappshell.so`nsXULWindow::CreateNewWindow()+0x83
      libtoolkitcomps.so` nsAppStartup::CreateChromeWindow2()+0x1bf
      libembedcomponents.so`nsWindowWatcher::OpenWindowJSInternal()+0xdea
      libembedcomponents.so` nsWindowWatcher::OpenWindowJS()+0xf0

So, it's not the XPM decoder in Mozilla's libpr0n doing this, but the decoder in GTK/GDK. Looking at nsWindow::SetWindowIconList() in the debugger confirms that all this stuff is happening with a single invocation the function, with only 1 entry ("default.xpm") in the icon list.

The behavior on Linux (Ubuntu 7.04, Feisty) is a little different. Strace shows it making 6 4K reads and 1 1866 byte read. But those reads are embedded in 1085 _llseek() calls, which are also rather bizarre:

      ...
      _llseek(84, 4096, [4096], SEEK_SET) = 0
      _llseek(84, 4096, [4096], SEEK_SET) = 0
      _llseek(84, 4096, [4096], SEEK_SET) = 0
      _llseek(84, 4096, [4096], SEEK_SET) = 0
      read(84, "169ECF\",\n\"E+\tc #1D99CC\",\n\"F+\tc #"..., 4096) = 4096
      _llseek(84, 8192, [8192], SEEK_SET) = 0
      _llseek(84, 8192, [8192], SEEK_SET) = 0
      _llseek(84, 8192, [8192], SEEK_SET) = 0
      _llseek(84, 8192, [8192], SEEK_SET) = 0
      ...

The repeated seeks are basically no-ops, but are incurring overhead from the user/kernel context switches.

This weird file IO takes about 45ms on Solaris, and 25ms on Linux. [Different hardware, so apples-to-oranges.] I had previously opened/closed a window to ensure default.xpm was in the OS disk cache. Txul on the Linux perf test box is about 200ms (on yet another box, so apples-to-oranges-to-pears).


Between this not being Mozilla code and XPM being such a crufty old format, I think the best fix here is to replace the .xpm usage with PNG files. I wouldn't be surprised if the distros are already doing this. It looks like gdk_pixbuf_new_from_file() will happily accept arbitrary file formats, and I should think PNG wouldn't be a problem with our relatively-modern reference platform requirements. Indeed, a quick test of "cat foo.png > default.xpm" on my Solaris box shows that this works fine.

Also, I suppose someone should file an upstream GDK bug on this?

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Perhaps related to bug 262265?

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 for review

This changes the icon setting code to prefer a PNG, and if no PNG is found it falls back to looking for an XPM. This allow us to start switching apps over to PNG without forcing everyone to change at once.

I went ahead and rolled in support for additional icon sizes, which would be needed by bug 404402. I only added the sizes recommended by:

http://library.gnome.org/devel/gtk/2.8/GtkWindow.html#gtk-window-set-icon-list

There's overhead for each size queried -- looks like we stat() in each extension's directory, each time a window is opened. So, we probably don't want this list to include every conceivable icon size.

SetDefaultIcon() seemed to be needlessly duplicating code, so I just have it use SetIcon() now too.

Tested on Solaris, confirmed the XPM fallback works and that when multiple icons are count GTK seems to prefer the right size.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The SetIcon API for setting icons by loading them from files seems ultra-lame. Why don't we just load image(s) using our own image loader(s), and convert to a pixbuf using pixbuf conversion code already in our tree (and similar for other platforms). Then the platform-specific code would be reduced quite a bit and we'd be able to use whatever image formats we support. Oh well.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about just adding those .xpm extensions to the extensions array? You can have an extra check inside the loop so that if we've just looked up the last .png extension and the list is nonempty, we exit early.

"extensions" would be better as an array of chars. So
  const char extensions[6][7] = { ".png", ... };
and use an nsAutoString local to copy the extension, or for bonus points, make ResolveIconName take a char* for the extension and do the conversion internally.

Comment 5

[Justin Dolske [:Dolske]]

(In reply to comment #4)
> How about just adding those .xpm extensions to the extensions array?

I think it would be better to leave them separate, since it should be removed as soon as projects migrate away from .xpm.

> "extensions" would be better as an array of chars. So
>   const char extensions[6][7] = { ".png", ... };

ok.

> and use an nsAutoString local to copy the extension, or for bonus points, make
> ResolveIconName take a char* for the extension and do the conversion
> internally.

How do I pound a char* into a nsAutoString? Wouldn't a nsDependentCString be better?

*sigh* I hate our string API.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Like so?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> I think it would be better to leave them separate, since it should be removed
> as soon as projects migrate away from .xpm.

there's no telling when that might happen. In the meantime we can save code.

I would just write
  nsAutoString extension;
  extension.AppendASCII(...);

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Third time's the charm!

Comment 9

[Robert Kaiser]

That version is probably better also for the reason that extensions can set their own icons and might use XPMs for some time to come, just to stay compatible with our old versions.

Comment 10

[Justin Dolske [:Dolske]]

cvs commit: Examining widget/src/gtk2
Checking in widget/src/gtk2/nsWindow.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.248; previous revision: 1.247