Comment on attachment 535603 [details] [diff] [review] patch v1 XP_UNIX as Mac as well. You probably want MOZ_WIDGET_GTK2.

s/XP_UNIX as Mac/XP_UNIX is Mac/ See also bug 621091 comment 17

> See also bug 621091 comment 17 It looks like bug 571619 might have fixed this.

Thanks for all this information! I'll use MOZ_WIDGET_GTK2 and send it to try to see if we're still having failures.

Now using MOZ_WIDGET_GTK2 and sent to try.

Could you please post the link to the try run?

(In reply to comment #7) > Could you please post the link to the try run? http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=3e6268f590de

looking at try, looks like tests are causing crashes and a bunch of glib assertions. This may make sense, gtk icons are handled through the gtk libs that are inited by widgets, since these are xpcshell tests, there is no widget code inited. We may fix the nsIconChannel::Init implementation to init gtk when needed (I can't find if there is a way to detect if it has already been inited though), but that may be hacky (see for example http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#243). Or we may just convert those tests to b-c tests or chrome tests, where we know gtk is inited. Doing the latter could even make sense since icons are only useful in the UI, but may also leave crash possibilities on Linux. I think you may want to cc biesi and karlt to evaluate the best approach here.

CC'ing biesi and karlt to evaluate the best approach.

It looks like either every XPCOM module that uses glib should call g_type_init or we should have a global place to call g_type_init during startup. For the browser, this happens in gtk_parse_args during XRE_main, but I gather xpcshell does not use XRE_main. I would probably have preferred a global init method to call g_type_init but I don't see a good place. NS_InitXPCOM2 doesn't seem to do any other platform-specific initialization. I hoping g_type_init is enough here (i.e. that no GDK display is required). g_type_init does some thread locking stuff, so it may be better to call that on the mozilla::Module::loadProc, or protect the call with a static variable, than to call it every time nsIconChannel::Init() is called.

(In reply to comment #11) > It looks like either every XPCOM module that uses glib Well, actually only those that use GObject, if that is any different, but only deal with nsIconModule in this bug.

(In reply to comment #11) > g_type_init does some thread locking stuff, so it may be better to call that > on the mozilla::Module::loadProc, or protect the call with a static variable, > than to call it every time nsIconChannel::Init() is called. Thanks! So, the first approach would hook here, right? http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/decoders/icon/nsIconModule.cpp#85 I see we already have a Dtor for #ifdef MOZ_WIDGET_GTK2, we may just add a Ctor. Tim, may you try to do that and see if tests are happier?

Yes, that's what I meant, but I'm not an expert on xpcom initialization. The other pattern I see, which I think would also work, is something like NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsIconProtocolHandler, Init). I don't know that there is much practical difference in this situation, and adding IconDecoderModuleCtor looks simpler here and has the nice symmetry with the existing Dtor.

(In reply to comment #13) > I see we already have a Dtor for #ifdef MOZ_WIDGET_GTK2, we may just add a > Ctor. Tim, may you try to do that and see if tests are happier? Yeah, I'll give that a try :) Thanks, Karl!

Will send to try when the tree re-opens.

Alas, the current patch still fails: http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=fbf4a8ae3f79 Karl, does the patch look like what you had in mind? Any idea what fails here?

Yes, the patch did what I had in mind. It fixed the initial problems (it was lucky enough not to crash), but it seems GTK's icon methods (gtk_icon_set_render_icon) expect GDK to have a default display. (I don't know what is happening in test_favicon_annotations.xul.) nsIconModule isn't really the right place to be trying to set up the default display. (There's clean-up required to avoid memory leaks, and it gets tricky trying to decide whether to clean-up a shared resource such as the default display.) I guess we could make xpcshell set up GTK and clean-up. It's fiddly, and seems a bit heavy-handed, though I guess if xpcshell was built with MOZ_WIDGET_GTK2, then GTK is likely to get used. What if we look at this another way?: The test is telling us that when there is no GDK display, the moz-icon stock icons are not available. Can nsFaviconService fall back to chrome://mozapps/skin/places/defaultFavicon.png when the stock icon is not available? (We can patch up the moz-icon code to detect when there is no display and fail early, before all the GLib assertions fail.)

Actually, I don't mind that much that favicon service may be used without a display, I mind that nsIconChannel doesn't crash or do bad stuff though. So I suppose we should have a patch to properly bail out in nsIconChannel, as you said (and we can write a small xpcshell test to check that nothing bad happens), and we should just convert any xpcshell test using the favicon service to a chrome test (it's matter of a hg rename, adding some xul builerplate and add to the Makefile.in). This requires we figure out the test_favicon_annotations.xul failure first.

Makes sense, Marco. gdk_display_get_default() can be used to check whether the display has been set up. With GDK 2, this doesn't need a g_type_init() and, if it returns non-NULL, it means g_type_init() has already been called. GDK 3 will be different, requiring g_type_init() for gdk_display_get_default(), so I don't mind whether g_type_init is added here or not. I see the InitWithGnome() path calls gnome_init() if that hasn't happened already. That would set up the necessary display, I expect, but calling gnome_init there is not right (bug 486925). I would be happy to see nsIconChannel::Init() return NS_ERROR_NOT_AVAILABLE whenever there is no default display.

Just a thought: for themes that are not using native elements, moz-icon://stock/gtk-file is for sure not the desirable default icon... It will be possible for third-party themes to change it with CSS?

Also note that gtk- prefixed icon names are deprecated ans no longer supported in gnome 3

When looking up by stock id such as "gtk-file", gtk will map stock id to icon name.

Won't fix that because bug 648668 is going to be implemented and we should rather take that new icon when the patch lands.