It's not feasible to load both version 2 and version 3 of Gtk+ into the same process, so if we ever want to move Gecko proper to Gtk3, we are probably going to need to build the plugin host twice - once with the older and once with the newer libraries. A general mechanism for shipping multiple plugin hosts and selecting the appropriate one at plugin launch time could also make 32/64-bit cross compatibility easier and enable us to remove some of the kludges related to support for really old Xt-based plugins. I think if we did this we would want to make strenuous efforts to cut down on the core (libxul) code required by the plugin host, so that we didn't end up rebuilding libxul twice or more. (Maybe that belongs in its own bug?) Excerpts from the email discussion I had last year on this topic: From: Karl Tomlinson <karlt@karlt.net> > > http://library.gnome.org/devel/gtk/unstable/migrating.html > > http://library.gnome.org/devel/gtk/unstable/api-index-3-0.html > > I didn't notice any major differences, if all we are going to do > is choose at compile-time which GTK library to use, and we are > only going load plugins using the same GTK library. > > The difficult bits are going to be whether we are going to try to > support (single) builds that run against either library. If it > weren't for plugins, then I'd tend to just suggest treating GTK+2 > and GTK+3 as separate platforms, and produce different builds for > each platform. > > It looks like we can't have both GTK+2 and GTK+3 libraries loaded > into the same dynamic symbol scope. > http://library.gnome.org/devel/gtk/unstable/ch24s04.html > > libflashplayer.so uses GTK+2 and assumes that the host is using > GDK's default display. > > So it sounds like we'll need to ensure either that > > a) the plugin host does not load any GTK library if the > plugin depends on a GTK library, or > > b) the plugin host does not load any plugins that use a > different GTK library. From: Karl Tomlinson <karlt@karlt.net> > Zack Weinberg writes: > > I know right now the plugin container links an awful lot of code from > > Gecko proper, but do you think it would be possible to prune that down > > to the point where we could build it repeatedly - once for Gtk2, once > > for Gtk3, maybe even once again for Xt - including only the relevant > > libraries each time, and then at runtime dispatch to the appropriate > > container process for the plugin we're asked to load? It might be > > more work short term but it seems likely to give less grief than > > anything else in the long run. > > It may be worth checking out what is happening with running 32-bit > plugins in 64-bit browsers, as that is another situation where a > trimmed down plugin container would make sense. > > https://bugzilla.mozilla.org/show_bug.cgi?id=559142 > https://bugzilla.mozilla.org/show_bug.cgi?id=588749 > > For the normal container-matches-browser situation there is an > advantage in having the plugin container being the same libxul and > the browser because that is already loaded in memory (except for > relocations). > > To answer your question, I suspect a trimmed down plugin container > would be quite feasible, though I haven't looked into it. I > probably wouldn't bother with a separate container for Xt, but > it's sounding like we'll need a separate container for GTK2. IRC conversation, not sure of location: <karlt> Company: do you know whether there has been any consideration given to versioning symbols for GTK+ 3? <karlt> that would help a GTK3 browser load a GTK2 plugin maybe <otaylor> karlt: GObject has a big hash table from type name to type information <otaylor> (among many other reasons it wouldn't work) <Company> karlt: as owen says <Company> karlt: the webkit-gtk people are already very excited about loading the flash plugin in a gtk3 browser ;) <lewing> the plugin people are pretty excited too <karlt> otaylor: do the type names actually need to stay the same? <otaylor> karlt: lets' say yes <karlt> well thanks for the answer, i guess <otaylor> karlt: Nothing in the basic infrastructure really requires it, but language bindings tend to make assumptions, etc <karlt> ah, i see <otaylor> karlt: plus, there are the other things like event delivery - you'd need separate Display connections for gtk2 and gtk3 <karlt> otaylor: yes, and i'm guessing flashplayer assumes it has a GdkDisplay, but we can provide that <otaylor> karlt: I basically consider gtk2 and gtk3 in the same process to equal an infinite series of really hard to track down weird problems From: Karl Tomlinson <karlt@karlt.net> > Robert O'Callahan writes: > > Can't we solve this by using GTK2 in the plugin process? > > If we build a different libxul for the plugin host or build the > plugin host differently, then that will work until there are GTK3 > plugins. > > We could either have both GTK2 and GTK3 plugin hosts and I assume > we can detect which to use, or the plugin host could detect which > GTK library it should access, based on the plugin. > > We also could have fun loading the plugin into the browser process > to check the mime type. RTLD_LOCAL or even RTLD_DEEPBIND might > help there.

Is this only an issue because Flash loads GTK2? Do we know why it needs to do that? We could skip this whole problem by shipping a QT Firefox instead ;-) This is really really low priority, I think, so I'm not sure it's worth worrying about any time soon.

A fix for it is already a part of patch from bug 627699

Was this fixed by one of your patches from bug 627699?

The fix is a part of the original patch in Bug 627699 but the solution is inefficient - it creates extra libxul for gtk2. The right way is to create a small plugin-container which is link with gtk2 libraries.

To be clear, the plugins are disabled in the gtk3 firefox right now and plugin fix is needed.

I'm working on a solution which moves plug-in code and toolkit code out of the linxul. There is going to be one libxul, two toolkit libraries (gtk3/gtk2) and two plugin libraries (gtk3/gtk2). The link scenario is then: firefox <- libxul + toolkit library (gtk3) plugin-container <- libxul + toolkit library (gtk2 and gtk3) + plugin code (gtk2 and gtk3) Thoughts?

A quick scan indicates that those modules call gtk code and should be available as gtk2 and gtk3 versions. The real challenge is to separate them from libxul and put them into extra library, if that's even possible: dom/plugins gfx toolkit widget Another solution is to build two libxul, one for gtk2 and one for gtk3. This approach is used in the original patch.

(In reply to Martin Stránský from comment #7) > Another solution is to build two libxul, one for gtk2 and one for gtk3. This > approach is used in the original patch. While, as you say, that is not the most efficient, that seems reasonable to me, at least as a starting point. This is even what happens on MacOS IIUC. Separating GTK usage from libxul will be quite a task, I expect. Most of libxul will not be used by the plugin-container, so minimizing what is built into a GTK2 plugin-container might be much easier than separating GTK code from a standard libxul, but this can be an optional follow-up to just building a standard but GTK2 libxul. Have a look at how universal binaries are built for MacOS. I don't know how that works, but ISTR someone saying there were two different builds, perhaps in different object directories, that were then packaged together somehow. This will need to be reviewed by a build config peer, so I suggest outlining a proposal and asking for feedback.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1) > Is this only an issue because Flash loads GTK2? Do we know why it needs to > do that? Yes, and Flash is linked against the GTK2 libraries. It uses GTK for drawing, XEmbed communication, opening dialogs, etc.

Thanks for the feedback! So the plan is to create an extra libxul for gtk2 plugins and then try to remove the extra code from plugin-container.

(In reply to Karl Tomlinson (:karlt) from comment #8) > Have a look at how universal binaries are built for MacOS. I don't know how > that works, but ISTR someone saying there were two different builds, perhaps > in different object directories, that were then packaged together somehow. Our Mac builds are in fact built in two passes, one for x86 and one for x86-64, which are then merged into a single set of universal binaries. Note that there's no equivalent on Linux, and this also doubles our build time on Mac. I'd rather not do that on Linux.

Defines two new values: MOZ_ENABLE_GTK2_PLUGIN - turns on/off extra gtk2 plugin build LIBXUL_GTK2_PLUGIN_LIBS - flags for linking with libxul_gtk2 library

Comment on attachment 805962 [details] [diff] [review] configure.in patch btw. It's possible to construct the solution without MOZ_ENABLE_GTK2_PLUGIN switch but IMHO it may be useful to build Gtk3 only browser, without Gtk2 parts.

Comment on attachment 805962 [details] [diff] [review] configure.in patch Having a MOZ_ENABLE_GTK2_PLUGIN Makefile variable makes sense to me. I'm not sure we can review the other changes yet, without more understanding of how things work, but passing this to a build config peer.

I spent last week to shrink libxul_gtk2 size needed for gtk2 plugin-container. It results to changes in sources which may be difficult to maintain beside the fact I still don't have a linkable solution. Even If we manage to produce a small libxul subset it will need some dummy classes and workarounds which will be difficult to review and keep up to date along the regular code base.

An updated one.

Comment on attachment 819744 [details] [diff] [review] configure.in v2 Clearing review until dependencies are figured out.

How is the mac build composed from the two builds? Is that a part of Firefox build script? I assume there are two libxul libraries (i386 and x86_64). Does the i386 libxul have a different name or so?

On OS X libxul and other binaries are delivered as "universal binaries" (or "fat binaries"), which bundle both architectures together in one file.

Well, I guess the build takes two passes - one for i386 and one for x86_64. I'd like to emulate it with Gtk3 - to build one build as gtk2 and one build as gtk3 and combine Firefox+gtk3 and plugin-container+gtk2. The question is if the two pass build is already a part of the build system (I haven't found it) or the build is just run twice and composed by some external script.

(In reply to comment #20) As best I can tell, we no longer do a good job of explaining how to do Mac "universal builds". But here (attached) is the mozconfig file *I* use to do them. I hope it helps. (After the build is finished, you can cd to either objdir/i386 or objdir/x86_64 and run "make package". Last I knew, you can't use mach -- it doesn't work properly with Mac universal builds.)

Mac universal builds are basically two separate objdirs--one for each CPU arch. They're combined at the end of the build using a postflight makefile (invoked from client.mk): http://mxr.mozilla.org/mozilla-central/source/build/macosx/universal/flight.mk This is horrible and is why I would rather not have to have more of this sort of build.

Thanks, I guess it's pretty straightforward. AFAIK we can't combine the binaries on Linux that way so we need a different name for the libxul linked to plugin-container. It's because firefox bin directory has to contain both of them - one for firefox itself(gtk3) and one for the plugin-container(gtk2). And for the final build we can copy plugin-container + gtk2 libxul to the Gtk3 Firefox bin directory.

Hm, something like this one? It enables to build libxul under alternative name.

It depends what your end goal is. If it's about building that for your redhat/fedora packages, you can apply the patch in your package, and build however you like. If it's about building that for mozilla.org builds using gtk3, then that approach is not welcome.

(In reply to Mike Hommey [:glandium] from comment #25) > It depends what your end goal is. If it's about building that for your > redhat/fedora packages, you can apply the patch in your package, and build > however you like. If it's about building that for mozilla.org builds using > gtk3, then that approach is not welcome. Hm, I don't understand what do you want then...how would you like to build the plugin support for the gtk3 browser? And I guess mozilla is going to ship gtk3 browser some day...

And yes, we're going to build that for Fedora first and test it there.

(In reply to Martin Stránský from comment #26) > Hm, I don't understand what do you want then...how would you like to build > the plugin support for the gtk3 browser? And I guess mozilla is going to > ship gtk3 browser some day... If mozilla does that, we won't replicate the horror show that mac builds do.

Note we may have a solution to build multiple libxul with slightly different setups for gtest in the coming weeks. That could be reused here. That's however orthogonal to whether we actually want to go the heavyweight way or not.

(In reply to Mike Hommey [:glandium] from comment #29) > Note we may have a solution to build multiple libxul with slightly different > setups for gtest in the coming weeks. That could be reused here. That's > however orthogonal to whether we actually want to go the heavyweight way or > not. Can you point me to a bug which covers that? I'd like to utilize it then. Thanks!

Actually we need to provide a GtkSocket support in Gtk3 browser.

Comment on attachment 8360390 [details] [diff] [review] nsPluginNativeWindowGtk patch Try build: https://tbpl.mozilla.org/?tree=Try&rev=2dd358bc828a

Comment on attachment 8360390 [details] [diff] [review] nsPluginNativeWindowGtk patch I defer to Karl Tomlinson on this.

Comment on attachment 8360390 [details] [diff] [review] nsPluginNativeWindowGtk patch >- GdkWindow *gdkWindow = gdk_window_lookup(GetWindow()); >+ GdkWindow *gdkWindow = gdk_x11_window_lookup_for_display(display, (XID)window); Please continue to use GetWindow here. It is more careful with the cast, and consistent with other code.

Thanks! There's the updated version.

> From: Karl Tomlinson <karlt@karlt.net> > > Zack Weinberg writes: > > > I know right now the plugin container links an awful lot of code from > > > Gecko proper, but do you think it would be possible to prune that down > > > to the point where we could build it repeatedly - once for Gtk2, once > > > for Gtk3 > > To answer your question, I suspect a trimmed down plugin container > > would be quite feasible, though I haven't looked into it.[...]it's > > sounding like we'll need a separate container for GTK2. > > We could either have both GTK2 and GTK3 plugin hosts and I assume > > we can detect which to use, or the plugin host could detect which > > GTK library it should access, based on the plugin. > > > > We also could have fun loading the plugin into the browser process > > to check the mime type. RTLD_LOCAL or even RTLD_DEEPBIND might > > help there. We are kicking off new efforts on that front at the moment. As broadly discussed in bug 929408, Stransky's dual-libxul solution was not kept because of maintenance/build problems and lib overhead/duplication. So we're going down the "prune down plugin-container" road. Here's a description of what's being done atm: Plugin-container is currently linking against libxul mainly for XRE_InitChildProcess() and al. Doing so, it also pulls GTK-dependant stuff through libxul, tough we want to make it gtk-INdependant. So the idea here, since that XRE_InitChildProcess stuff is also required/called by libxul internally, would be to strip that code out of libxul and put it into a new shared lib, say, libpluginhelper.so, so that libxul can still link to this new object and get the functionality, and plugin-container can stop linking against libxul and only link again libpluginhelper in order *not* to pull the GTK stuff. Here's a simple diagram explaining that twist : ============================================= firefox | libxul.so-----gtk+ (2 or 3 selected at build time) | |---> libpluginhelper.so <---| | | ---------------------------------------------- process boundary / IPC | | libpluginhelper.so libpluginhelper.so | | plugin-container plugin-container | | [[GTK2 plugin ]] [[GTK3 plugin ]] mainly Flash there is NONE yet!

An alternative proposal to consider: Anthony suggested something similar. I can't recall whether the details are the same or not. Create two GTK kind-of-wrapper libraries with the same SONAME, libmozgtk.so.0 or something. libmozgtk2 depends on libgtk-x11-2.0.so.0 and has stubs for GTK3-only functions. libmozgtk3 depends on libgtk-3.so.0 and has stubs for GTK2-only functions. The stubs can assert in debug builds that they are not called. libxul.so is linked against either of the wrapper libraries, instead of against gtk/gdk libs. The browser app requires only GTK3. It loads libmozgtk3 before loading libxul. libxul depends on libmozgtk. I expect ld.so will see that this has already been provided, and so won't need to search for the dependency. I expect the dependencies (libgtk-3, etc.) of libmozgtk3 will resolve the GTK3 symbols required by libxul.so. plugin-container requires only GTK2. It loads mozgtk2 before loading libxul. This approach may require considerable boilerplate, but shouldn't require fundamental changes to the structure of Gecko code or its build system.

I've been looking at this for a while. The first approach was to get the code that plugin-container uses out of libxul and into another shared library, say libpluginhelper, which wouldn't link to gtk+. This way libxul could build against gtk+ 3, and plugin-container could load plugins that linked to gtk+ 2. This is what Fred explained in comment #38. That turned out to not be suitable. plugin-container calls XRE_InitChildProcess, which requires lots of stuff from libxul (ipc/chromium, xpcom, dom/ipc, ...). Some of those depend on gtk+ and we can't move that into libpluginhelper, so this didn't seem like a good approach, mostly because of the big dependency chain. The dependency chain looked roughly like: 1: plugin-container -> ipc/chromium/ (for MessageLoop) 2: ipc/chromium/ -> ipc/glue/ (for UTF8ToWide & friends) 3: ipc/chromium and ipc/glue/ -> xpcom/ (for NS_*) 4: ipc/glue -> ipc/ipdl/ (for mozilla::ipc::GenericURIParams and others) ipc/glue -> dom/ipc/ (for Blob) xpcom/base, xpcom/threads, ipc/glue -> toolkit/crashreporter/ (for CrashReporter) xpcom/base, xpcom/threads -> toolkit/components/telemetry/ (for TimeHistogram and others) xpcom/ds/ -> intl/unicharutil/util/internal/ (for HashUTF8AsUTF16) xpcom/ -> js/xpconnect/ (for xpc_GCThingIsGrayCCThing, XPCStringConvert and others) 5: dom/ipc/ -> layout/ (for mozilla::layout::RenderFrame{Child,Parent}) toolkit/components/telemetry/ -> tools/profiler/ (for IOInterposer and others) dom/ipc/, xpcom/base -> dom/power/ (for PowerManagerService, WakeLock) dom/ipc/ -> dom/devicestorage/ (for FileUpdateDispatcher) dom/ipc/ -> modules/libpref/ (for Preferences) dom/ipc/ -> netwerk/ (for NeckoParent) dom/ipc/ -> js/ipc/ (for JavaScriptParent) 6: layout/svg/ -> gfx/layers (for ClientLayerManager) *** PROBLEM *** layout/svg/ -> gfx/thebes (for gfxContext and others) layout/xul/ -> gfx/src/ (for nsRenderingContext and others) 7: *** PROBLEM *** layout/ -> accessible/ (for nsAccessibilityService) *** PROBLEM *** layout/ -> dom/plugins/base/ (for nsPluginInstanceOwner and others) *** PROBLEM *** layout/ -> widget/ (for LookAndFeel, WidgetEvent, maybe others) And after that I still had lots of undefined references (so more code would need to be moved).

Hi Karl, this looks promising! I have some questions: (In reply to Karl Tomlinson (:karlt) from comment #39) > An alternative proposal to consider: > Anthony suggested something similar. I can't recall whether the details are > the same or not. > > Create two GTK kind-of-wrapper libraries with the same SONAME, libmozgtk.so.0 > or something. They have the same SONAME so that we can load one or the other and the linker will always be happy as the dependency from the ELF binary will be resolved... right? So then we put one of them in a sub-directory, e.g. in my system we would have: /usr/lib/xulrunner-24.0/libxul.so /usr/lib/xulrunner-24.0/libmozgtk.so (linking to gtk+ 3) /usr/lib/xulrunner-24.0/gtk2/libmozgtk.so (linking to gtk+ 2) > libmozgtk2 depends on libgtk-x11-2.0.so.0 and has stubs for GTK3-only > functions. libmozgtk3 depends on libgtk-3.so.0 and has stubs for GTK2-only > functions. The stubs can assert in debug builds that they are not called. > > libxul.so is linked against either of the wrapper libraries, instead of > against gtk/gdk libs. Both wrapper libraries have the same SONAME (libmozgtk.so) so libxul would just link to that. > The browser app requires only GTK3. It loads libmozgtk3 before loading > libxul. How does it load the gtk+3 version of libmozgtk.so ? By having the gtk+3 version in the library path? > libxul depends on libmozgtk. I expect ld.so will see that this has > already been provided, and so won't need to search for the dependency. I > expect the dependencies (libgtk-3, etc.) of libmozgtk3 will resolve the GTK3 > symbols required by libxul.so. I'll do some experiments tomorrow with simple libraries to see if this would indeed work. > plugin-container requires only GTK2. It loads mozgtk2 before loading > libxul. How? Perhaps through LD_LIBRARY_PATH? We would then need to have: /usr/lib/xulrunner-24.0/plugin-container (shell script setting LD_LIBRARY_PATH) /usr/lib/xulrunner-24.0/plugin-container.real Or perhaps with an rpath. > This approach may require considerable boilerplate, but shouldn't require > fundamental changes to the structure of Gecko code or its build system. Could you please expand on this? What boilerplate would be needed? Is that just the wrappers for gtk+2-only and gtk+3-only functions? I suppose we only need to wrap those that libxul or plugin-container call and that are only in one of gtk+2 or gtk+3. That's probably not a very big set. Or am I missing something?

(In reply to Emilio Pozuelo Monfort from comment #40) > I've been looking at this for a while. The first approach was to get the > code that plugin-container uses out of libxul and into another shared > library, say libpluginhelper, which wouldn't link to gtk+. This way libxul > could build against gtk+ 3, and plugin-container could load plugins that > linked to gtk+ 2. This is what Fred explained in comment #38. > > That turned out to not be suitable. plugin-container calls > XRE_InitChildProcess, which requires lots of stuff from libxul > (ipc/chromium, xpcom, dom/ipc, ...). Some of those depend on gtk+ and we > can't move that into libpluginhelper, so this didn't seem like a good > approach, mostly because of the big dependency chain. The dependency chain > looked roughly like: > > 1: plugin-container -> ipc/chromium/ (for MessageLoop) > 2: ipc/chromium/ -> ipc/glue/ (for UTF8ToWide & friends) > 3: ipc/chromium and ipc/glue/ -> xpcom/ (for NS_*) You should be able to chop it off between either (1) and (2) or (2) and (3), because ipc/chromium is (as the name implies) copied from Chromium, and Chromium doesn't *use* XPCOM. The path of least resistance, much as I hate to suggest this, may well be to reimplement ipc/glue so it depends only on NSPR.

(In reply to Frederic Plourde (:fred23) from comment #38) > [[GTK3 plugin ]] > there is NONE yet! If no GTK3 plugins exist atm, do we even need to add support for them when we are going to reduce NPAPI support more and more?

(In reply to Masatoshi Kimura [:emk] from comment #43) > If no GTK3 plugins exist atm, do we even need to add support for them when > we are going to reduce NPAPI support more and more? The "GTK3-plugins" branch of that diagram was just for explanatory purposes. The focus was to be put on the GTK2-plugin branch (for flash, basically) + *GTK3* libxul that is undergoing intensive effort at the moment and should ship with GTK3 browser soon. In that case, the diagram would read : firefox | libxul.so-----gtk+3 | |---> libpluginhelper.so | ---------------------------------------------- process boundary / IPC | libpluginhelper.so | plugin-container | [[GTK2 plugin ]] mainly Flash ... hence the title of the bug. But in the end, looking at all the good exploratory work done by Emilio, this whole diagram is deprecated now and does not represent the newer solutions mentioned for example in Comment 41.

(In reply to Emilio Pozuelo Monfort from comment #41) > (In reply to Karl Tomlinson (:karlt) from comment #39) > They have the same SONAME so that we can load one or the other and the > linker will always be happy as the dependency from the ELF binary will be > resolved... right? Yes. > So then we put one of them in a sub-directory, e.g. in my > system we would have: > > /usr/lib/xulrunner-24.0/libxul.so > /usr/lib/xulrunner-24.0/libmozgtk.so (linking to gtk+ 3) > /usr/lib/xulrunner-24.0/gtk2/libmozgtk.so (linking to gtk+ 2) > > The browser app requires only GTK3. It loads libmozgtk3 before loading > > libxul. > > How does it load the gtk+3 version of libmozgtk.so ? By having the gtk+3 > version in the library path? IIUC XPCOMGlueLoad is used to load libxul into the app and explicitly loads the dependencies in dependentlibs.list using dlopen expecting them to be in the same directory as libxul. Perhaps dependentlibs.list can be tweaked. Note that the library filename need not match the soname if it is explicitly listed on the link command and opened with dlopen, so it could be called libmozgtk3.so, if that helps, but see below. > > plugin-container requires only GTK2. It loads mozgtk2 before loading > > libxul. > > How? Perhaps through LD_LIBRARY_PATH? We would then need to have: > > /usr/lib/xulrunner-24.0/plugin-container (shell script setting > LD_LIBRARY_PATH) > /usr/lib/xulrunner-24.0/plugin-container.real > > Or perhaps with an rpath. plugin-container seems different from the app in that it uses ld.so to load the libraries. GeckoChildProcessHost::PerformAsyncLaunchInternal sets LD_LIBRARY_PATH, and the library must then have the filename corresponding to the soname, so perhaps it is easiest to extend LD_LIBRARY_PATH to include /usr/lib/xulrunner-24.0/gtk2/libmozgtk.so > > This approach may require considerable boilerplate, but shouldn't require > > fundamental changes to the structure of Gecko code or its build system. > > Could you please expand on this? What boilerplate would be needed? Is that > just the wrappers for gtk+2-only and gtk+3-only functions? I suppose we only > need to wrap those that libxul or plugin-container call and that are only in > one of gtk+2 or gtk+3. That's probably not a very big set. Or am I missing > something? That's probably all that's necessary, and perhaps it is not a particularly big set.

we could launch the plugin-container with LD_PRELOAD=libmozgtk2.so instead of relying on LD_LIBRARY_PATH.

(In reply to Karl Tomlinson (:karlt) from comment #45) > (In reply to Emilio Pozuelo Monfort from comment #41) > > (In reply to Karl Tomlinson (:karlt) from comment #39) > > > This approach may require considerable boilerplate, but shouldn't require > > > fundamental changes to the structure of Gecko code or its build system. > > > > Could you please expand on this? What boilerplate would be needed? Is that > > just the wrappers for gtk+2-only and gtk+3-only functions? I suppose we only > > need to wrap those that libxul or plugin-container call and that are only in > > one of gtk+2 or gtk+3. That's probably not a very big set. Or am I missing > > something? > > That's probably all that's necessary, and perhaps it is not a particularly > big > set. Actually we need *all* the wrappers so that we don't get undefined references when linking libxul to gtk+ symbols, since a libxul -> libmozgtk -> libgtk+ dependency isn't enough to avoid undefined references when linking (and build errors as we build libxul with -z defs, for a good reason). That's not a big deal though. I've gone this way and got it to work! What I have done so far is: 0/ Build with --enable-default-toolkit=cairo-gtk3 --enable-system-cairo. The resulting Nightly works fine, but flash doesn't (the "this plugin crashed" notice is shown e.g. on youtube videos). 1/ Build a libmozgtk.so DSO with xul_gtk_foo() functions that call the gtk_foo() version. libmozgtk.so links to libgtk-3.so (GTK+3). 2/ Build a gtk2/libmozgtk.so similar to the one above but linking to libgtk-x11-2.0.so. It provides the same xul_gtk_foo() symbols, but some of them just print a warning. 3/ Make libxul include mozgtk.h instead of gtk/gtk.h, gtk/gtkx.h, gdk/gdk.h, gtk/gtkprinter.h... Link libxul with -lmozgtk and stop linking it to gtk+. 4/ Added "#define gtk_foo xul_gtk_foo" in mozgtk.h, so that libxul gets references to xul_gtk_foo, satisfied by its dynamic link to libmozgtk.so, adding the desired level of indirection. We could also s/g[dt]k_/xul_\0/ all over libxul if desired, but I went the #define way as it was easier and cleaner, and works for testing purposes. 5/ Added the gtk2/ path to plugin-container's LD_LIBRARY_PATH so that it loads the libmozgtk.so that links to libgtk-x11-2.0.so. 6/ Watched youtube videos \o/ Now what still needs work: 1/ A few (4) gtk calls that I had to wrap use varargs. One of them has a _valist equivalent in gtk+ that I used to wrap it. The others don't and need to be wrapped somehow or other non-vararg equivalent functions need to be used in libxul. 2/ There's only one mozgtk.h header right now. That probably should be split, particularly for the X11 functions. 3/ The gtk+2 build of the wrapper has a bunch of warnings for argument mismatches because the gtk+2 and gtk+3 versions differ. Most of those functions are probably unused from plugin-container but either we make proper calls into gtk+ or we turn those into warnings (without calling into gtk+) in the GTK+2 version. 4/ There is a crash in the browser when closing tabs with flash videos (or when navigating to another page, or when closing the whole browser, or when clicking on the "Large player" button in youtube). The crash is related to X11, but I haven't dug into it yet. 5/ Add wrappers for the functions that a gtk+2 libxul would call (i.e. when building with --enable-default-toolkit=cairo-gtk2) so that there's a transition period where you can build mozilla for gtk+2 or gtk+3 and both work through libmozgtk. So far I have only tested this when building libxul for gtk+3. 6/ I need to integrate the libmozgtk wrapper into mozilla (I've built it separately so far as that was quicker). I have a quick question: I'm reusing the same header and implementation for both the gtk+2 & 3 versions, building with a different define -DGTK2 / -DGTK3 for the minor differences. I could very easily have two completely different implementations but I think having one with a few #ifdefs makes the whole thing easier to maintain. The question is, can I build two libraries from the same sources with moz.build files? I was thinking something like: mozgtk/sources/mozgtk[.ch] mozgtk/gtk2/moz.build: LIBRARY_NAME=mozgtk UNIFIED_SOURCES=../sources/mozgtk.c mozgtk/gtk3/moz.build: LIBRARY_NAME=mozgtk UNIFIED_SOURCES=../sources/mozgtk.c But that probably won't work because of the same LIBRARY_NAME and because the libraries will probably end in the same place in obj-*/dist/lib/libmozgtk.so... Any ideas? If not I'll investigate this on Monday but perhaps somebody knows if this is possible and can save me some work. Unrelatedly: I noticed some screen corruption on the lower part of the screen (where the control overlay appears) when watching an ogv video and going fullscreen (whether through F11 or through the in-video fullscreen control). This might be gtk+3 only and probably unrelated to the libmozgtk wrapper, but I haven't confirmed that yet. I should open a bug report when I confirm it's not caused by my changes. There may be other things to improve, and the whole solution needs some testing. But it's already working which is a big step! \o/

> 4/ There is a crash in the browser when closing tabs with flash videos (or > when navigating to another page, or when closing the whole browser, or when > clicking on the "Large player" button in youtube). The crash is related to > X11, but I haven't dug into it yet. That's Bug 968196 > Unrelatedly: I noticed some screen corruption on the lower part of the > screen (where the control overlay appears) when watching an ogv video and > going fullscreen (whether through F11 or through the in-video fullscreen > control). This might be gtk+3 only and probably unrelated to the libmozgtk > wrapper, but I haven't confirmed that yet. I should open a bug report when I > confirm it's not caused by my changes. That may be an incarnation of Bug 979839

Sounds promising, Emilio. (In reply to Emilio Pozuelo Monfort from comment #47) > Actually we need *all* the wrappers so that we don't get undefined > references when linking libxul to gtk+ symbols, since a libxul -> libmozgtk > -> libgtk+ dependency isn't enough to avoid undefined references when > linking (and build errors as we build libxul with -z defs, for a good > reason). Interesting. It seems that the build-time linker requires that all required symbols are in the libraries listed on the link command, but the run-time linker does not. That means there is the option of building against a libmozgtk with dummy symbols for *all* the functions, but running against a libmozgtk with stubs only for the unused functions and with dependencies on the appropriate gtk libraries for the used functions. Doing that would not require any special treatment for varargs functions, nor special headers. (I don't know what moz.build can and can't do.)

(In reply to Martin Stránský from comment #48) > > 4/ There is a crash in the browser when closing tabs with flash videos (or > > when navigating to another page, or when closing the whole browser, or when > > clicking on the "Large player" button in youtube). The crash is related to > > X11, but I haven't dug into it yet. > > That's Bug 968196 > > > Unrelatedly: I noticed some screen corruption on the lower part of the > > screen (where the control overlay appears) when watching an ogv video and > > going fullscreen (whether through F11 or through the in-video fullscreen > > control). This might be gtk+3 only and probably unrelated to the libmozgtk > > wrapper, but I haven't confirmed that yet. I should open a bug report when I > > confirm it's not caused by my changes. > > That may be an incarnation of Bug 979839 Thanks for the pointers!

I'm posting this unfinished patch for feedback and comments! The most important problem right now is that the gtk+2 wrapper is built as libmozgtk2.so and the LD_LIBRARY_PATH change doesn't load it. In order to get LD_LIBRARY_PATH to load it, the library would need to be named the same and be in another directory, e.g. obj-*/dist/lib/gtk2/, and /usr/lib/xulrunner/gtk2/ when installed. In order to test this version you can do: cp obj-*/dist/lib/libmozgtk2.so /usr/lib/gtk2/libmozgtk.so Suggestions on how to install the library with the same name to a subdir (or how to solve this problem in a different way) are more than welcome.

This is more complex than what's described in comment 49. Specifically, you don't need mozgtk.h, and you don't need most symbols in the mozgtk lib. Only those that are not in common between gtk2 and gtk3. So, in the end, you should have a dist/lib/libmozgtk.so, linked against gtk3, with essentially no symbols and dist/lib/libmozgtk2.so, linked against gtk2, and exporting symbols from gtk3 that aren't in gtk2. Both with the "libmozgtk.so" soname. Then instead of linking libxul.so against gtk, you link it against libmozgtk. That would work out of the box for firefox itself. Then you'd need to add code to make the plugin-container execute with LD_PRELOAD=libmozgtk2.so. We're already setting LD_PRELOAD for b2g, just check how it's done.

Also, it seems to me it would be better to put that under widget/gtk/.

(In reply to Mike Hommey [:glandium] from comment #52) > This is more complex than what's described in comment 49. > > Specifically, you don't need mozgtk.h, and you don't need most symbols in > the mozgtk lib. Only those that are not in common between gtk2 and gtk3. > So, in the end, you should have a dist/lib/libmozgtk.so, linked against > gtk3, with essentially no symbols and dist/lib/libmozgtk2.so, linked against > gtk2, and exporting symbols from gtk3 that aren't in gtk2. Both with the > "libmozgtk.so" soname. That is what we thought earlier (see the end of comment #41 and also comment #47). However from my tests that works well for runtime linking, but during the build you get warnings for undefined references, which means -z defs causes a build error. I didn't test this approach with mozilla but only with a minimal setup that was trying to mimic libxul, and given the results, I went for a full wrapper. Of course doing what you say would be much better, but there is the above problem plus some others, e.g. a few functions have different signatures for gtk+2 and gtk+3. If you build libxul for gtk+3 but then load gtk+2 for plugin-container, if those functions are called they'll get wrong arguments. We could wrap only those though, assuming we could solve the undefined references problem. > Then instead of linking libxul.so against gtk, you link it against > libmozgtk. That would work out of the box for firefox itself. Then you'd > need to add code to make the plugin-container execute with > LD_PRELOAD=libmozgtk2.so. We're already setting LD_PRELOAD for b2g, just > check how it's done. Right, that's better than LD_LIBRARY_PATH as we can have different .so file names (e.g. libmozgtk.so and libmozgtk2.so as I was doing with the previous patch) but have SONAME be libmozgtk.so for both thanks to ld's -soname, and preload libmozgtk2.so when needed. I've just tried this and it works as expected! I'm attaching a new patch with that fix.

The only changes here wrt v1 are to toolkit/mozgtk/gtk2/Makefile.in for the soname and to ipc/glue/GeckoChildProcessHost.cpp to set LD_PRELOAD.

(In reply to Emilio Pozuelo Monfort from comment #54) > (In reply to Mike Hommey [:glandium] from comment #52) > > This is more complex than what's described in comment 49. > > > > Specifically, you don't need mozgtk.h, and you don't need most symbols in > > the mozgtk lib. Only those that are not in common between gtk2 and gtk3. > > So, in the end, you should have a dist/lib/libmozgtk.so, linked against > > gtk3, with essentially no symbols and dist/lib/libmozgtk2.so, linked against > > gtk2, and exporting symbols from gtk3 that aren't in gtk2. Both with the > > "libmozgtk.so" soname. > > That is what we thought earlier (see the end of comment #41 and also comment > #47). However from my tests that works well for runtime linking, but during > the build you get warnings for undefined references, which means -z defs > causes a build error. I didn't test this approach with mozilla but only with > a minimal setup that was trying to mimic libxul, and given the results, I > went for a full wrapper. Indeed if I keep linking libxul to libmozgtk, but drop #define gtk_dialog_set_alternative_button_order xul_gtk_dialog_set_alternative_button_order from mozgtk.h, the build fails at the linking stage with: 3:48.90 /home/emilio/src/mozilla/mozilla-central/widget/gtk/nsFilePicker.cpp:376: error: undefined reference to 'gtk_dialog_set_alternative_button_order' 3:48.90 /home/emilio/src/mozilla/mozilla-central/widget/gtk/nsPrintDialogGTK.cpp:72: error: undefined reference to 'gtk_dialog_set_alternative_button_order'

What you can do is create a build-time-only libmozgtk.so exporting the gtk symbols, and link libxul.so against that. Then you don't need mozgtk.h and you don't need to have accurate definitions for those symbols (you can make them all void foo(void))

We can actually avoid listing all common GTK2+3 symbols in a special build-time mozgtk by using standard gtk/gdk sonames like libgtk-3.so.0 and libgdk-3.so.0. LD_PRELOAD or whatever can be used to load stubs for the plugin container. I originally imagined linking linking libxul against gtk3 and gtk2 versions, with the gtk2 versions being stubs for the browser process, but glandium pointed out that a simpler approach could be only link libxul against only the gtk3 versions, but to LD_PRELOAD a libgtk-3.so.0 (and libgdk-3.so.0) with dummy symbols and linked against gtk2 when loading plugin-container. glandium's suggestion is best if there are no problems requiring gtk2-only symbols from libxul. We could have a gdk_window_get_type function in our fake gdk3 lib that calls gdk_window_object_get_type, but, if we need that, there may be a problem due to compiling with gtk3 headers code that runs against gtk2 libraries. PluginModuleChild should be compiled against gtk2 headers because it accesses a GtkPlugClass struct member.

(In reply to Mike Hommey [:glandium] from comment #57) > What you can do is create a build-time-only libmozgtk.so exporting the gtk > symbols, and link libxul.so against that. Then you don't need mozgtk.h and > you don't need to have accurate definitions for those symbols (you can make > them all void foo(void)) I see! That's a nice trick, implemented in this new patch, which simplifies things quite a lot. Build-time wise, we link libxul with -lmozgtk_stub. libmozgtk_stub.so has a SONAME of libmozgtk.so, so libxul gets a link to libmozgtk.so Then at runtime, given the link (NEEDED), libmozgtk.so (which is an empty shared library but linking against libgtk-3.so) is loaded, and so libgtk-3.so is loaded, thus bringing in the gtk / gdk symbols. Likewise for plugin-container, with the LD_PRELOAD trick, libmozgtk2.so is loaded, which satisfies the libmozgtk.so NEEDED dependency in libxul because libmozgtk2.so has a SONAME of libmozgtk.so. libmozgtk2.so brings in libgtk-x11-2.0.so and so the necessary symbols. plugin-container / flash work fine with this patch. I suppose we'll want to keep libxul-GTK2 working for a while... if so this patch needs some changes for that (e.g. don't build libmozgtk.so for gtk+3, and have libmozgtk2.so be named libmozgtk.so). I'll look into that.

(In reply to Karl Tomlinson (:karlt) from comment #58) > PluginModuleChild should be compiled against gtk2 headers because it accesses > a GtkPlugClass struct member. Right. That could be why currently scrolling in flash doesn't work, resulting in: [9078] ###!!! ASSERTION: deltaX or deltaY must be non-zero: 'wheelEvent.deltaX || wheelEvent.deltaY', file /home/emilio/src/mozilla/mozilla-central/widget/gtk/nsWindow.cpp, line 3093 I have tried building dom/plugins/ipc/ for GTK2, but then some code calls into xt_ which is only built when we build libxul for GTK2 (see widget/gtkxtbin/moz.build) and so the linking fails. Possible solutions could be: - force build gtkxtbin against gtk+2 when MOZ_WIDGET_GTK == 3, like we're doing for moz/plugins/ipc/. - don't call xt_* in dom/plugins/ipc/, just like we don't call it when MOZ_WIDGET_GTK == 3. - maybe something else... I'll look into that, but suggestions are welcome

(In reply to Emilio Pozuelo Monfort from comment #59) > I suppose we'll want to keep libxul-GTK2 working for a while... if so this > patch needs some changes for that (e.g. don't build libmozgtk.so for gtk+3, > and have libmozgtk2.so be named libmozgtk.so). I'll look into that. Ideally building libxul against gtk2 wouldn't use libmozgtk at all. (In reply to Emilio Pozuelo Monfort from comment #60) > Right. That could be why currently scrolling in flash doesn't work, > resulting in: > > [9078] ###!!! ASSERTION: deltaX or deltaY must be non-zero: > 'wheelEvent.deltaX || wheelEvent.deltaY', file > /home/emilio/src/mozilla/mozilla-central/widget/gtk/nsWindow.cpp, line 3093 I don't know. That might be something else. > Possible solutions could be: > - force build gtkxtbin against gtk+2 when MOZ_WIDGET_GTK == 3, like we're > doing for moz/plugins/ipc/. That would be the preferred approach. In-process plugins should also be disabled in GTK3 builds, so this code won't be needed from the browser process, and I think the GTK parts of gtkxtbin won't be used at all. > - don't call xt_* in dom/plugins/ipc/, just like we don't call it when > MOZ_WIDGET_GTK == 3. Acroread and VLC are the most common Xt plugins IIRC. I don't care about Acroread - users are better off without it. There was also some game with a proprietary plugin. We should consider these nice to have, but we can drop support for these if we need to.

(In reply to Karl Tomlinson (:karlt) from comment #61) > (In reply to Emilio Pozuelo Monfort from comment #59) > > I suppose we'll want to keep libxul-GTK2 working for a while... if so this > > patch needs some changes for that (e.g. don't build libmozgtk.so for gtk+3, > > and have libmozgtk2.so be named libmozgtk.so). I'll look into that. > > Ideally building libxul against gtk2 wouldn't use libmozgtk at all. Right. I have done that now and building libxul against gtk2 works as before (no wrapper built, no LD_PRELOAD...). > (In reply to Emilio Pozuelo Monfort from comment #60) > > Right. That could be why currently scrolling in flash doesn't work, > > resulting in: > > > > [9078] ###!!! ASSERTION: deltaX or deltaY must be non-zero: > > 'wheelEvent.deltaX || wheelEvent.deltaY', file > > /home/emilio/src/mozilla/mozilla-central/widget/gtk/nsWindow.cpp, line 3093 > > I don't know. That might be something else. Yeah, it still happens with that code built against gtk+2. > > Possible solutions could be: > > - force build gtkxtbin against gtk+2 when MOZ_WIDGET_GTK == 3, like we're > > doing for moz/plugins/ipc/. > > That would be the preferred approach. I've done that. > In-process plugins should also be disabled in GTK3 builds, so this code > won't be needed from the browser process, and I think the GTK parts of > gtkxtbin won't be used at all. > > > - don't call xt_* in dom/plugins/ipc/, just like we don't call it when > > MOZ_WIDGET_GTK == 3. > > Acroread and VLC are the most common Xt plugins IIRC. I don't care about > Acroread - users are better off without it. There was also some game with a > proprietary plugin. We should consider these nice to have, but we can drop > support for these if we need to. VLC here doesn't link against libXt, so it just works with the wrapper solution, whether I build gtkxtbin or not. Acroread is only available for x86 and I am on x86_64... I can probably build firefox for x86 to test this, but if you have a pointer to vlc-using-Xt, that should be much faster.

(In reply to Emilio Pozuelo Monfort from comment #62) > VLC here doesn't link against libXt, so it just works with the wrapper > solution, whether I build gtkxtbin or not. That's good news. I guess VLC have changed their plugin, perhaps so that it works with chromium, or perhaps just so they don't need Xt. > Acroread is only available for > x86 and I am on x86_64... I can probably build firefox for x86 to test this, > but if you have a pointer to vlc-using-Xt, that should be much faster. I don't think we need to worry about old vlc plugins, and there's probably more important things to do than test acroread.

(In reply to Mike Hommey [:glandium] from comment #53) > Also, it seems to me it would be better to put that under widget/gtk/. That indeed seems like a more appropriate place. Done. (In reply to Karl Tomlinson (:karlt) from comment #61) > In-process plugins should also be disabled in GTK3 builds, so this code > won't be needed from the browser process, and I think the GTK parts of > gtkxtbin won't be used at all. OPP forced in dom/plugins/base/nsNPAPIPlugin.cpp This also builds moz/plugins/ipc/ and gtkxtbin for gtk+2 as mentioned before. It also makes libxul-gtk2 not go through the wrapper, so if you're not building for gtk+3, this should be a NOP for you. I'm investigating the scrolling issue I mentioned before, which seems related to smooth scroll event support in gtk+3.

(In reply to Karl Tomlinson (:karlt) from comment #58) > We can actually avoid listing all common GTK2+3 symbols in a special > build-time > mozgtk by using standard gtk/gdk sonames like libgtk-3.so.0 and > libgdk-3.so.0. > LD_PRELOAD or whatever can be used to load stubs for the plugin container. > > I originally imagined linking linking libxul against gtk3 and gtk2 versions, > with the gtk2 versions being stubs for the browser process, but glandium > pointed out that a simpler approach could be only link libxul against only > the gtk3 versions, but to LD_PRELOAD a libgtk-3.so.0 (and libgdk-3.so.0) > with dummy symbols and linked against gtk2 when loading plugin-container. > > glandium's suggestion is best if there are no problems requiring gtk2-only > symbols from libxul. > > We could have a gdk_window_get_type function in our fake gdk3 lib that calls > gdk_window_object_get_type, but, if we need that, there may be a problem due > to > compiling with gtk3 headers code that runs against gtk2 libraries. I tried something like this. It works, but it requires us to provide both libgtk-3.so and libgdk-3.so wrappers, otherwise plugin-container loads libgtk-x11-2.0.so, libgdk-x11-2.0.so and libgdk-3.so. Also if we call gtk+2 only symbols (like we do in the last patch because we build dom/plugins/ipc/ and gtkxtbin for gtk+2), we need a stub wrapper as well with dummy symbols, and just linking to gtk+3 directly is not enough. So I don't think this solution is better / cleaner than the current one with a stub wrapper with all required symbols.

Comment on attachment 8406797 [details] [diff] [review] gtkwrapper-v7.patch I didn't understand what provides the GTK3 symbols when mozgtk2 is loaded. Have you tested with LD_BIND_NOW=1 in the environment? Mike has the final say on most of this, but here's a few things that can be tidied up. >- if test "$MOZ_ENABLE_GTK2"; then >+ if test "$MOZ_ENABLE_GTK2" || test "$MOZ_ENABLE_GTK3"; then There is a MOZ_ENABLE_GTK variable. >+#if defined(MOZ_WIDGET_GTK) && MOZ_WIDGET_GTK == 3 MOZ_WIDGET_GTK will behave like 0 if not defined, so no need for the defined(). >+EXTRA_DSO_LDOPTS += -lgobject-2.0 -lglib-2.0 -latk-1.0 -lgio-2.0 -lgdk_pixbuf-2.0 -lmozgtk_stub Library names usually come from pkg-config. MOZ_GIO_LIBS and GLIB_LIBS would cover most of these. >+CXXFLAGS += -include gtkdefine.h $(MOZ_GTK2_CFLAGS) I guess -DMOZ_WIDGET_GTK=2 would fail to achieve this because -include ../../mozilla-config.h is processed after command line defines? I wonder whether it would be easier to just assume that MOZ_WIDGET_GTK means GTK2 in the plugin ipc child code. >- GDK_THREADS_ENTER (); >+ gdk_threads_enter (); What is the issue being fixed here? If these are causing a problem then we can remove them. Gecko doesn't try to use these consistently so there is not point in having a few of these calls.

Comment on attachment 8406797 [details] [diff] [review] gtkwrapper-v7.patch Review of attachment 8406797 [details] [diff] [review]: ----------------------------------------------------------------- Not repeating Karl's comments ::: dom/plugins/ipc/gtkdefine.h @@ +1,2 @@ > +#undef MOZ_WIDGET_GTK > +#define MOZ_WIDGET_GTK 2 I think we should switch to using GTK_MAJOR_VERSION in many parts of the C++ codebase. We could start with the plugins code. ::: widget/gtk/moz.build @@ +4,4 @@ > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +DIRS += ['mozgtk'] Seems to me this should be conditional to gtk3, and the condition in mozgtk/moz.build lifted. ::: widget/gtk/mozgtk/gtk2/Makefile.in @@ +6,5 @@ > + > +include $(topsrcdir)/config/rules.mk > + > +CFLAGS += $(MOZ_GTK2_CFLAGS) > +LDFLAGS += -Wl,-soname=libmozgtk.so This should be derived from DLL_PREFIX/DLL_SUFFIX (openbsd likes to change DLL_SUFFIX) Ideally, we would avoid the original -Wl,-h to be passed on the command line from MKSHLIB. Ideally, we'd define a SONAME here (or moz.build), that defaults to $(notdir $@) in config/rules.mk, and change the various MKSHLIB definitions in configure.in. That could probably be done in a followup bug. ::: widget/gtk/mozgtk/stub/mozgtk.c @@ +1,3 @@ > +#include "mozilla/Types.h" > + > +MOZ_EXPORT void gdk_atom_intern (void) { } How about defining a macro for the boilerplate and using MACRO(symbol) ? the macro could be called STUB or something. ::: widget/gtkxtbin/gtk2xtbin.c @@ +83,4 @@ > { > int mask; > > + gdk_threads_enter(); AIUI, the GDK_THREADS_ENTER/LEAVE macros have been deprecated in gtk3. Since you're forcing the code to build with gtk2 anyways, why care to change them?

(In reply to Karl Tomlinson (:karlt) from comment #66) > Comment on attachment 8406797 [details] [diff] [review] > gtkwrapper-v7.patch > > I didn't understand what provides the GTK3 symbols when mozgtk2 is loaded. > Have you tested with LD_BIND_NOW=1 in the environment? No, nothing special in the environment (except for what libxul itself sets). It might be that there are no gtk+3-only symbols used in libxul at this point. > Mike has the final say on most of this, but here's a few things that can be > tidied up. > > >- if test "$MOZ_ENABLE_GTK2"; then > >+ if test "$MOZ_ENABLE_GTK2" || test "$MOZ_ENABLE_GTK3"; then > > There is a MOZ_ENABLE_GTK variable. Done. > >+#if defined(MOZ_WIDGET_GTK) && MOZ_WIDGET_GTK == 3 > > MOZ_WIDGET_GTK will behave like 0 if not defined, so no need for the > defined(). Done. > >+EXTRA_DSO_LDOPTS += -lgobject-2.0 -lglib-2.0 -latk-1.0 -lgio-2.0 -lgdk_pixbuf-2.0 -lmozgtk_stub > > Library names usually come from pkg-config. > MOZ_GIO_LIBS and GLIB_LIBS would cover most of these. Done for those. Still hardcoded for atk and gdk-pixbuf. I could add a PKG_CONFIG_CHECK() for those, or perhaps strip the -lgdk and -lgtk parts from $(TK_LIBS) directly, if you think that would be preferred. > >+CXXFLAGS += -include gtkdefine.h $(MOZ_GTK2_CFLAGS) > > I guess -DMOZ_WIDGET_GTK=2 would fail to achieve this because -include > ../../mozilla-config.h is processed after command line defines? Exactly. > I wonder whether it would be easier to just assume that MOZ_WIDGET_GTK means > GTK2 in the plugin ipc child code. Do you mean dropping the #if (MOZ_WIDGET_GTK == 3) code? The above include hack (or something similar) is still needed so that indirect includes see the right MOZ_WIDGET_GTK define and include the right gtk+ headers. Otherwise both gtk+2 and gtk+3 headers get included and the compilation explodes. > >- GDK_THREADS_ENTER (); > >+ gdk_threads_enter (); > > What is the issue being fixed here? gdk_threads_enter is a function, but GDK_THREADS_ENTER is a define. When building against gtk+3, we get: gdkthreads.h:#define GDK_THREADS_ENTER() gdk_threads_enter() So libxul calls into gdk_threads_enter, which is also present in gtk+2, and so loading gtk+2 works just fine. However when building this code against gtk+2 (as we do with the -include hack from above) we get from gtk+ 2: # define GDK_THREADS_ENTER() G_STMT_START { \ if (gdk_threads_lock) \ (*gdk_threads_lock) (); \ } G_STMT_END gdk_threads_lock is not part of the ABI for gtk+3, thus the linking phase fails with: 2:09.39 /home/emilio/src/mozilla/mozilla-central/widget/gtkxtbin/gtk2xtbin.c:86: error: undefined reference to 'gdk_threads_lock' Using gdk_threads_enter solves this. > If these are causing a problem then we can remove them. > Gecko doesn't try to use these consistently so there is not point in having a > few of these calls. Fair enough, removed now. FWIW, when I was building this code against gtk+3, I didn't see any problems. In that case the gtkxtbin code wasn't built though (so possibly no Xt plugins). (In reply to Mike Hommey [:glandium] from comment #67) > Comment on attachment 8406797 [details] [diff] [review] > gtkwrapper-v7.patch > > Review of attachment 8406797 [details] [diff] [review]: > ----------------------------------------------------------------- > > Not repeating Karl's comments > > ::: dom/plugins/ipc/gtkdefine.h > @@ +1,2 @@ > > +#undef MOZ_WIDGET_GTK > > +#define MOZ_WIDGET_GTK 2 > > I think we should switch to using GTK_MAJOR_VERSION in many parts of the C++ > codebase. We could start with the plugins code. Should we do that in a separate bug? That #define seemed like the cleanest way to solve the errors I got when trying to build the plugin code against gtk+2, e.g.: 0:23.19 In file included from /usr/include/gtk-2.0/gdk/gdktestutils.h:28:0, 0:23.19 from /usr/include/gtk-2.0/gdk/gdk.h:56, 0:23.19 from ../../../dist/system_wrappers/gdk/gdk.h:3, 0:23.19 from ../../../dist/include/mozilla/X11Util.h:14, 0:23.19 from ../../../dist/include/mozilla/plugins/NPEventUnix.h:13, 0:23.19 from ../../../dist/include/mozilla/plugins/PluginMessageUtils.h:910, 0:23.19 from /home/emilio/src/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/plugins/PBrowserStreamParent.h:17, 0:23.19 from /home/emilio/src/mozilla/mozilla-central/dom/plugins/ipc/BrowserStreamParent.h:9, 0:23.19 from /home/emilio/src/mozilla/mozilla-central/dom/plugins/ipc/BrowserStreamParent.cpp:7: 0:23.19 /home/emilio/src/mozilla/mozilla-central/widget/gtk/compat/gdk/gdkwindow.h: In function ‘GdkScreen* gdk_window_get_screen(GdkWindow*)’: 0:23.19 /home/emilio/src/mozilla/mozilla-central/widget/gtk/compat/gdk/gdkwindow.h:23:41: error: cannot convert ‘GdkWindow* {aka _GdkWindow*}’ to ‘GdkDrawable* {aka _GdkDrawable*}’ for argument ‘1’ to ‘GdkScreen* gdk_drawable_get_screen(GdkDrawable*)’ 0:23.19 return gdk_drawable_get_screen (window); 0:23.19 ^ In some cases this is easy to solve, e.g. in widget/gtkxtbin/gtk2xtbin.h:12, changing #if (MOZ_WIDGET_GTK == 3) to GTK_MAJOR_VERSION or GTK_CHECK_VERSION or similar. But perhaps we can investigate this separately. What do you think? > ::: widget/gtk/moz.build > @@ +4,4 @@ > > # License, v. 2.0. If a copy of the MPL was not distributed with this > > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > > > +DIRS += ['mozgtk'] > > Seems to me this should be conditional to gtk3, and the condition in > mozgtk/moz.build lifted. Done. > > ::: widget/gtk/mozgtk/gtk2/Makefile.in > @@ +6,5 @@ > > + > > +include $(topsrcdir)/config/rules.mk > > + > > +CFLAGS += $(MOZ_GTK2_CFLAGS) > > +LDFLAGS += -Wl,-soname=libmozgtk.so > > This should be derived from DLL_PREFIX/DLL_SUFFIX (openbsd likes to change > DLL_SUFFIX) Done. > Ideally, we would avoid the original -Wl,-h to be passed on the command line > from MKSHLIB. Ideally, we'd define a SONAME here (or moz.build), that > defaults to $(notdir $@) in config/rules.mk, and change the various MKSHLIB > definitions in configure.in. That could probably be done in a followup bug. Sounds good. Different bug sounds fine to keep this simple where possible. > ::: widget/gtk/mozgtk/stub/mozgtk.c > @@ +1,3 @@ > > +#include "mozilla/Types.h" > > + > > +MOZ_EXPORT void gdk_atom_intern (void) { } > > How about defining a macro for the boilerplate and using MACRO(symbol) ? the > macro could be called STUB or something. Agreed, that's much cleaner. Done. > ::: widget/gtkxtbin/gtk2xtbin.c > @@ +83,4 @@ > > { > > int mask; > > > > + gdk_threads_enter(); > > AIUI, the GDK_THREADS_ENTER/LEAVE macros have been deprecated in gtk3. Since > you're forcing the code to build with gtk2 anyways, why care to change them? See the reason above. I have dropped them now as Karl suggested. BTW they are deprecated because calls should be made from the main thread (with g_idle_add or similar if necessary). Thank you both for your comments!

(In reply to Emilio Pozuelo Monfort from comment #68) > > Ideally, we would avoid the original -Wl,-h to be passed on the command line > > from MKSHLIB. Ideally, we'd define a SONAME here (or moz.build), that > > defaults to $(notdir $@) in config/rules.mk, and change the various MKSHLIB > > definitions in configure.in. That could probably be done in a followup bug. > > Sounds good. Different bug sounds fine to keep this simple where possible. I have opened bug #1000994 for this.

Comment on attachment 8411743 [details] [diff] [review] gtkwrapper-v8.patch Review of attachment 8411743 [details] [diff] [review]: ----------------------------------------------------------------- Please put the changes to nsNPAPIPlugin.cpp and gtk2xtbin.c in a separate patch, I think it would be better to keep the build bits separate. The GeckoChildProcessHost.cpp are borderline-related, I guess it's fine to keep them together, although it might make sense to separate them too (and aiui, without them we'd just have a gtk3 plugin-container). You're missing package manifest changes to install libmozgtk2.so and libmozgtk.so. (search for package-manifest.in files in the tree) ::: dom/plugins/base/nsNPAPIPlugin.cpp @@ +260,5 @@ > > +#if (MOZ_WIDGET_GTK == 3) > + // We force OOP on Linux/GTK3 because some plugins use GTK2 and both GTK > + // libraries can't be loaded in the same process. > + return true; Considering this comment, I'd say MOZ_DISABLE_OOP_PLUGINS should be made ineffective a few lines above. ::: ipc/glue/GeckoChildProcessHost.cpp @@ +582,5 @@ > + const char *ld_preload = PR_GetEnv("LD_PRELOAD"); > + nsCString new_ld_preload; > + > + new_ld_preload.Assign(path.get()); > + new_ld_preload.AppendLiteral("/libmozgtk2.so"); This needs to be derived from DLL_PREFIX/DLL_SUFFIX here too. ::: toolkit/library/libxul.mk @@ +168,5 @@ > endif > > ifdef MOZ_WIDGET_GTK > +ifdef MOZ_ENABLE_GTK3 > +EXTRA_DSO_LDOPTS += $(GLIB_LIBS) $(MOZ_GIO_LIBS) -latk-1.0 -lgdk_pixbuf-2.0 -lmozgtk_stub I'd rather do something like $(filter-out -lgtk-3 -lgdk-3,$(TK_LIBS)) ::: widget/gtk/mozgtk/gtk2/Makefile.in @@ +5,5 @@ > +EXTRA_DSO_LDOPTS += $(MOZ_GTK2_LIBS) > + > +include $(topsrcdir)/config/rules.mk > + > +CFLAGS += $(MOZ_GTK2_CFLAGS) There is no source to apply these CFLAGS to. ::: widget/gtk/mozgtk/gtk3/Makefile.in @@ +1,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this file, > +# You can obtain one at http://mozilla.org/MPL/2.0/. > + > +EXTRA_DSO_LDOPTS += $(TK_LIBS) This should be $(MOZ_GTK3_LIBS). Or even probably just -lgtk-3.0 -lgdk-3.0 (MOZ_GTK3_LIBS contains way more than that) The same is probably true for the gtk2 version. It should probably just use -lgtk-2.0 -lgdk-2.0. @@ +5,5 @@ > +EXTRA_DSO_LDOPTS += $(TK_LIBS) > + > +include $(topsrcdir)/config/rules.mk > + > +CFLAGS += $(TK_CFLAGS) There is no source to apply these CFLAGS to. That said, here and in gtk2, there *should* be a source, for the symbols that aren't in the opposite library. For instance, this is what "LD_LIBRARY_PATH=. ldd -r -d libxul.so" says: undefined symbol: gdk_window_lookup (./libxul.so) undefined symbol: gdk_x11_drawable_get_xdisplay (./libxul.so) undefined symbol: gdk_x11_colormap_foreign_new (./libxul.so) undefined symbol: gdk_drawable_get_screen (./libxul.so) undefined symbol: gdkx_visual_get (./libxul.so) undefined symbol: gdk_rgb_get_visual (./libxul.so) undefined symbol: gdk_x11_drawable_get_xid (./libxul.so) undefined symbol: gdk_x11_window_get_drawable_impl (./libxul.so) undefined symbol: gtk_object_get_type (./libxul.so) undefined symbol: gdk_window_set_back_pixmap (./libxul.so) undefined symbol: gdk_rgb_get_colormap (./libxul.so) undefined symbol: gdk_x11_colormap_get_xcolormap (./libxul.so) And what "LD_PRELOAD=libmozgtk2.so LD_LIBRARY_PATH=. ldd -r -d libxul.so" says: undefined symbol: gtk_cairo_transform_to_window (./libxul.so) undefined symbol: gtk_style_context_get_border_color (./libxul.so) undefined symbol: gtk_style_context_add_region (./libxul.so) undefined symbol: gtk_render_frame (./libxul.so) undefined symbol: gtk_get_major_version (./libxul.so) undefined symbol: gtk_get_minor_version (./libxul.so) undefined symbol: gtk_render_focus (./libxul.so) undefined symbol: gtk_style_context_new (./libxul.so) undefined symbol: gtk_style_context_save (./libxul.so) undefined symbol: gtk_render_line (./libxul.so) undefined symbol: gtk_combo_box_text_append (./libxul.so) undefined symbol: gtk_render_frame_gap (./libxul.so) undefined symbol: gtk_render_arrow (./libxul.so) undefined symbol: gtk_render_handle (./libxul.so) undefined symbol: gtk_menu_button_new (./libxul.so) undefined symbol: gtk_style_context_set_path (./libxul.so) undefined symbol: gtk_tree_view_column_get_button (./libxul.so) undefined symbol: gtk_render_background (./libxul.so) undefined symbol: gtk_style_context_get_padding (./libxul.so) undefined symbol: gdk_display_get_device_manager (./libxul.so) undefined symbol: gtk_style_context_get_color (./libxul.so) undefined symbol: gtk_style_context_remove_region (./libxul.so) undefined symbol: gtk_render_option (./libxul.so) undefined symbol: gtk_get_micro_version (./libxul.so) undefined symbol: gdk_window_get_type (./libxul.so) undefined symbol: gtk_render_extension (./libxul.so) undefined symbol: gdk_x11_window_get_xid (./libxul.so) undefined symbol: gtk_style_context_get_margin (./libxul.so) undefined symbol: gtk_style_context_get_border (./libxul.so) undefined symbol: gdk_device_manager_get_client_pointer (./libxul.so) undefined symbol: gtk_cairo_should_draw_window (./libxul.so) undefined symbol: gtk_style_context_restore (./libxul.so) undefined symbol: gtk_render_slider (./libxul.so) undefined symbol: gtk_widget_get_style_context (./libxul.so) undefined symbol: gtk_style_context_set_state (./libxul.so) undefined symbol: gtk_widget_path_new (./libxul.so) undefined symbol: gtk_render_check (./libxul.so) undefined symbol: gtk_render_activity (./libxul.so) undefined symbol: gtk_widget_set_visual (./libxul.so) undefined symbol: gtk_style_context_add_class (./libxul.so) undefined symbol: gtk_style_context_get_background_color (./libxul.so) undefined symbol: gtk_widget_path_append_type (./libxul.so) undefined symbol: gtk_render_expander (./libxul.so) That also makes e.g. webrtc signaling test fail to build because the static linker tries to resolve all symbols. Those are the symbols Karl was telling should MOZ_CRASH() or something along those lines. ::: widget/gtk/mozgtk/stub/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +UNIFIED_SOURCES += [ Doesn't need to be unified. ::: widget/gtkxtbin/gtkdefine.h @@ +1,2 @@ > +#undef MOZ_WIDGET_GTK > +#define MOZ_WIDGET_GTK 2 I must insist that this is too hackish. And in fact, looking at it more throroughly, it appears it's actually dangerous, due to what effect changing MOZ_WIDGET_GTK has on some headers and on some code. Any code that is #if (MOZ_WIDGET_GTK == 2) or #if (MOZ_WIDGET_GTK == 3) needs to be thoroughly inspected, and at the very least I suspect issues with code under dom/plugins/base. In fact, there are various #if (MOZ_WIDGET_GTK == 2) and #if (MOZ_WIDGET_GTK == 3) sections that make no sense now that some code is meant to always be built with gtk2. And there is one specific portion that is #if (MOZ_WIDGET_GTK == 2) that seems dangerous in PluginInstanceChild.h. Sure, it's for private members of the class, so it only really matters for the implementation of the class, but who knows how the code is going to evolve? I wouldn't be surprised with some inline function accessing a member added at the end of the class in the future. Which would lead to code using that function to access the wrong offset if used in code built against gtk3 when the implementation for the most of the class is built against gtk2. There are things I'm concerned with in dom/plugins/base, too, which I suspect at least some portions to be for the plugin container. So all in all, I think a prerequisite to this bug is to clear out all uses of the MOZ_WIDGET_GTK macro as a number, and carefully identifying if we mean to use the gtk2 or the gtk3 api/abi for each, depending whether they're meant to run in the parent process, or in the plugin process. Code that's exclusively for the parent process, but with support for both gtk2 and gtk3, GTK_MAJOR_VERSION should be used. Code that's exclusively for the child process should use the gtk2 code, and code that ends up used in both, well, probably gtk2 too. I'm tempted to say we should clearly separate code for parent process and code for plugin process, but that's probably daunting.

For what it's worth, with this patch on top of yours, I get through an entire build (without --disable-tests ;) ), and Firefox runs fine, including plugins. Except it crashes when stopping the plugin instance. Note this doesn't address all my review comments. Only part of the MOZ_WIDGET_GTK problem, but addresses the missing symbols part.

(In reply to Mike Hommey [:glandium] from comment #71) > Except it crashes when stopping the plugin instance. Well, so does it without my patch (only with the widget/gtk/ changes to pass linkage)

(In reply to Mike Hommey [:glandium] from comment #72) > (In reply to Mike Hommey [:glandium] from comment #71) > > Except it crashes when stopping the plugin instance. > > Well, so does it without my patch (only with the widget/gtk/ changes to pass > linkage) That would be https://bugzilla.mozilla.org/show_bug.cgi?id=968196

(In reply to Mike Hommey [:glandium] from comment #70) > Comment on attachment 8411743 [details] [diff] [review] > gtkwrapper-v8.patch > > Review of attachment 8411743 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please put the changes to nsNPAPIPlugin.cpp and gtk2xtbin.c in a separate > patch, I think it would be better to keep the build bits separate. The > GeckoChildProcessHost.cpp are borderline-related, I guess it's fine to keep > them together, although it might make sense to separate them too (and aiui, > without them we'd just have a gtk3 plugin-container). Ack. > You're missing package manifest changes to install libmozgtk2.so and > libmozgtk.so. (search for package-manifest.in files in the tree) I was basically doing ./mach build; ./mach run. Those changes done and now ./mach install installs libmozgtk{,2}.so > ::: dom/plugins/base/nsNPAPIPlugin.cpp > @@ +260,5 @@ > > > > +#if (MOZ_WIDGET_GTK == 3) > > + // We force OOP on Linux/GTK3 because some plugins use GTK2 and both GTK > > + // libraries can't be loaded in the same process. > > + return true; > > Considering this comment, I'd say MOZ_DISABLE_OOP_PLUGINS should be made > ineffective a few lines above. I didn't do that because I thought MOZ_DISABLE_OOP_PLUGINS would only be used by developers / testers when really necessary, and not all plugins will fail (just the ones that link to gtk+), so I thought I would let people override the behaviour with that env var. But I can change if you really think we shouldn't allow that. > ::: ipc/glue/GeckoChildProcessHost.cpp > @@ +582,5 @@ > > + const char *ld_preload = PR_GetEnv("LD_PRELOAD"); > > + nsCString new_ld_preload; > > + > > + new_ld_preload.Assign(path.get()); > > + new_ld_preload.AppendLiteral("/libmozgtk2.so"); > > This needs to be derived from DLL_PREFIX/DLL_SUFFIX here too. Done. > ::: toolkit/library/libxul.mk > @@ +168,5 @@ > > endif > > > > ifdef MOZ_WIDGET_GTK > > +ifdef MOZ_ENABLE_GTK3 > > +EXTRA_DSO_LDOPTS += $(GLIB_LIBS) $(MOZ_GIO_LIBS) -latk-1.0 -lgdk_pixbuf-2.0 -lmozgtk_stub > > I'd rather do something like $(filter-out -lgtk-3 -lgdk-3,$(TK_LIBS)) Yeah, that's what I said in a reply to Karl above. Done now. > ::: widget/gtk/mozgtk/gtk2/Makefile.in > @@ +5,5 @@ > > +EXTRA_DSO_LDOPTS += $(MOZ_GTK2_LIBS) > > + > > +include $(topsrcdir)/config/rules.mk > > + > > +CFLAGS += $(MOZ_GTK2_CFLAGS) > > There is no source to apply these CFLAGS to. > > ::: widget/gtk/mozgtk/gtk3/Makefile.in > @@ +1,5 @@ > > +# This Source Code Form is subject to the terms of the Mozilla Public > > +# License, v. 2.0. If a copy of the MPL was not distributed with this file, > > +# You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > +EXTRA_DSO_LDOPTS += $(TK_LIBS) > > This should be $(MOZ_GTK3_LIBS). Or even probably just -lgtk-3.0 -lgdk-3.0 > (MOZ_GTK3_LIBS contains way more than that) > The same is probably true for the gtk2 version. It should probably just use > -lgtk-2.0 -lgdk-2.0. Done. > > @@ +5,5 @@ > > +EXTRA_DSO_LDOPTS += $(TK_LIBS) > > + > > +include $(topsrcdir)/config/rules.mk > > + > > +CFLAGS += $(TK_CFLAGS) > > There is no source to apply these CFLAGS to. That said, here and in gtk2, > there *should* be a source, for the symbols that aren't in the opposite > library. > For instance, this is what "LD_LIBRARY_PATH=. ldd -r -d libxul.so" says: > > undefined symbol: gdk_window_lookup (./libxul.so) > undefined symbol: gdk_x11_drawable_get_xdisplay (./libxul.so) > undefined symbol: gdk_x11_colormap_foreign_new (./libxul.so) > undefined symbol: gdk_drawable_get_screen (./libxul.so) > undefined symbol: gdkx_visual_get (./libxul.so) > undefined symbol: gdk_rgb_get_visual (./libxul.so) > undefined symbol: gdk_x11_drawable_get_xid (./libxul.so) > undefined symbol: gdk_x11_window_get_drawable_impl (./libxul.so) > undefined symbol: gtk_object_get_type (./libxul.so) > undefined symbol: gdk_window_set_back_pixmap (./libxul.so) > undefined symbol: gdk_rgb_get_colormap (./libxul.so) > undefined symbol: gdk_x11_colormap_get_xcolormap (./libxul.so) > > And what "LD_PRELOAD=libmozgtk2.so LD_LIBRARY_PATH=. ldd -r -d libxul.so" > says: > > undefined symbol: gtk_cairo_transform_to_window (./libxul.so) > undefined symbol: gtk_style_context_get_border_color (./libxul.so) > undefined symbol: gtk_style_context_add_region (./libxul.so) > undefined symbol: gtk_render_frame (./libxul.so) > undefined symbol: gtk_get_major_version (./libxul.so) > undefined symbol: gtk_get_minor_version (./libxul.so) > undefined symbol: gtk_render_focus (./libxul.so) > undefined symbol: gtk_style_context_new (./libxul.so) > undefined symbol: gtk_style_context_save (./libxul.so) > undefined symbol: gtk_render_line (./libxul.so) > undefined symbol: gtk_combo_box_text_append (./libxul.so) > undefined symbol: gtk_render_frame_gap (./libxul.so) > undefined symbol: gtk_render_arrow (./libxul.so) > undefined symbol: gtk_render_handle (./libxul.so) > undefined symbol: gtk_menu_button_new (./libxul.so) > undefined symbol: gtk_style_context_set_path (./libxul.so) > undefined symbol: gtk_tree_view_column_get_button (./libxul.so) > undefined symbol: gtk_render_background (./libxul.so) > undefined symbol: gtk_style_context_get_padding (./libxul.so) > undefined symbol: gdk_display_get_device_manager (./libxul.so) > undefined symbol: gtk_style_context_get_color (./libxul.so) > undefined symbol: gtk_style_context_remove_region (./libxul.so) > undefined symbol: gtk_render_option (./libxul.so) > undefined symbol: gtk_get_micro_version (./libxul.so) > undefined symbol: gdk_window_get_type (./libxul.so) > undefined symbol: gtk_render_extension (./libxul.so) > undefined symbol: gdk_x11_window_get_xid (./libxul.so) > undefined symbol: gtk_style_context_get_margin (./libxul.so) > undefined symbol: gtk_style_context_get_border (./libxul.so) > undefined symbol: gdk_device_manager_get_client_pointer (./libxul.so) > undefined symbol: gtk_cairo_should_draw_window (./libxul.so) > undefined symbol: gtk_style_context_restore (./libxul.so) > undefined symbol: gtk_render_slider (./libxul.so) > undefined symbol: gtk_widget_get_style_context (./libxul.so) > undefined symbol: gtk_style_context_set_state (./libxul.so) > undefined symbol: gtk_widget_path_new (./libxul.so) > undefined symbol: gtk_render_check (./libxul.so) > undefined symbol: gtk_render_activity (./libxul.so) > undefined symbol: gtk_widget_set_visual (./libxul.so) > undefined symbol: gtk_style_context_add_class (./libxul.so) > undefined symbol: gtk_style_context_get_background_color (./libxul.so) > undefined symbol: gtk_widget_path_append_type (./libxul.so) > undefined symbol: gtk_render_expander (./libxul.so) I see. I get that here too. I wonder why things work for me though: I don't get any errors when starting firefox or when firefox starts plugin-container. But I can certainly fix those. > That also makes e.g. webrtc signaling test fail to build because the static > linker tries to resolve all symbols. How can I build/run the tests? It'd be good to verify that I don't break stuff. > Those are the symbols Karl was telling should MOZ_CRASH() or something along > those lines. Ack. I had something along those lines in a previous version of the patch, but now with the stub lib approach, I only added them to the stub library but not to the others and things were working here. But I'll add them back to the gtk2 and gtk3 libs. > ::: widget/gtk/mozgtk/stub/moz.build > @@ +3,5 @@ > > +# This Source Code Form is subject to the terms of the Mozilla Public > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > +UNIFIED_SOURCES += [ > > Doesn't need to be unified. Ok, changed to SOURCES.

(In reply to Emilio Pozuelo Monfort from comment #74) > I didn't do that because I thought MOZ_DISABLE_OOP_PLUGINS would only be > used by developers / testers when really necessary, and not all plugins will > fail (just the ones that link to gtk+), so I thought I would let people > override the behaviour with that env var. But I can change if you really > think we shouldn't allow that. I'll leave this to Karl. > I see. I get that here too. I wonder why things work for me though: I don't > get any errors when starting firefox or when firefox starts > plugin-container. But I can certainly fix those. Probably you haven't tried running firefox with LD_BIND_NOW=1. > > That also makes e.g. webrtc signaling test fail to build because the static > > linker tries to resolve all symbols. > > How can I build/run the tests? It'd be good to verify that I don't break > stuff. Tests should be built by default. Maybe your linker has lax defaults. To run them, I think it's mach cppunittest and/or mach gtest. We should start looking at how this can be built and run on our try servers.

(In reply to Mike Hommey [:glandium] from comment #75) > (In reply to Emilio Pozuelo Monfort from comment #74) > > I didn't do that because I thought MOZ_DISABLE_OOP_PLUGINS would only be > > used by developers / testers when really necessary, and not all plugins will > > fail (just the ones that link to gtk+), so I thought I would let people > > override the behaviour with that env var. But I can change if you really > > think we shouldn't allow that. > > I'll leave this to Karl. Karl, any comments on this patch?

(In reply to Mike Hommey [:glandium] from comment #70) > ::: widget/gtkxtbin/gtkdefine.h > @@ +1,2 @@ > > +#undef MOZ_WIDGET_GTK > > +#define MOZ_WIDGET_GTK 2 > > I must insist that this is too hackish. Agreed. I have removed that from the last patches, as things still build and work fine without them. Though as you say we need to clean up all the MOZ_WIDGET_GTK uses. I'll attach another patch shortly for that. (In reply to Mike Hommey [:glandium] from comment #75) > (In reply to Emilio Pozuelo Monfort from comment #74) > > I see. I get that here too. I wonder why things work for me though: I don't > > get any errors when starting firefox or when firefox starts > > plugin-container. But I can certainly fix those. > > Probably you haven't tried running firefox with LD_BIND_NOW=1. > > > > That also makes e.g. webrtc signaling test fail to build because the static > > > linker tries to resolve all symbols. > > > > How can I build/run the tests? It'd be good to verify that I don't break > > stuff. > > Tests should be built by default. Maybe your linker has lax defaults. To run > them, I think it's mach cppunittest and/or mach gtest. For cppunittest: cppunittests INFO | Result summary: cppunittests INFO | Passed: 87 cppunittests INFO | Failed: 0 gtest fails to link libxul. It uses gold to link whereas ./mach build links it with ld AFAICS. No idea why. > We should start looking at how this can be built and run on our try servers. I don't have access yet, so I'm going to request it. BTW is there a try server building with gtk+ 3?

(In reply to Emilio Pozuelo Monfort from comment #80) > gtest fails to link libxul. It uses gold to link whereas ./mach build links > it with ld AFAICS. No idea why. Scratch that. It looks like gold is used all the time, so it's got to be something else.

(In reply to Emilio Pozuelo Monfort from comment #80) > > We should start looking at how this can be built and run on our try servers. > > I don't have access yet, so I'm going to request it. BTW is there a try > server building with gtk+ 3? There isn't at the moment, so pushing to try at the moment is not going to achieve much. Thus my initial suggestion.

(And by we I don't mean us, the people in this bug, but we, collectively, Mozilla)

Comment on attachment 8416408 [details] [diff] [review] Force OOP plugins on GTK3 Review of attachment 8416408 [details] [diff] [review]: ----------------------------------------------------------------- Leaving this one to Karl

Comment on attachment 8416409 [details] [diff] [review] patch #2: Remove GDK_THREADS_ENTER/LEAVE calls Review of attachment 8416409 [details] [diff] [review]: ----------------------------------------------------------------- Leaving this to Karl, too.

Comment on attachment 8416410 [details] [diff] [review] Add libmozgtk libraries so that libxul can use GTK+ 2 and 3 Review of attachment 8416410 [details] [diff] [review]: ----------------------------------------------------------------- You're saying this fails to build libxul for gtest. How does it fail? ::: browser/installer/package-manifest.in @@ +123,5 @@ > #endif > +#ifdef MOZ_GTK3 > +@BINPATH@/@DLL_PREFIX@mozgtk@DLL_SUFFIX@ > +@BINPATH@/@DLL_PREFIX@mozgtk2@DLL_SUFFIX@ > +#endif You'll need the same thing in b2g/installer/package-manifest.in ::: widget/gtk/mozgtk/gtk2/Makefile.in @@ +5,5 @@ > +EXTRA_DSO_LDOPTS += -lgtk-x11-2.0 -lgdk-x11-2.0 > + > +include $(topsrcdir)/config/rules.mk > + > +CFLAGS += $(MOZ_GTK2_CFLAGS) Can leave the CFLAGS out. The sole c file doesn't need the gtk headers. ::: widget/gtk/mozgtk/gtk2/mozgtk.c @@ +1,4 @@ > +#include "mozilla/Types.h" > +#include "mozilla/Assertions.h" > + > +#define STUB(symbol) MOZ_EXPORT void symbol (void) { MOZ_CRASH("can't call __func__ from a GTK+2 process!"); } I doubt __func__ works inside a literal string. You'd have to cut it in pieces. ::: widget/gtk/mozgtk/gtk3/Makefile.in @@ +5,5 @@ > +EXTRA_DSO_LDOPTS += -lgtk-3 -lgdk-3 > + > +include $(topsrcdir)/config/rules.mk > + > +CFLAGS += $(MOZ_GTK3_CFLAGS) Same as CFLAGS for gtk2. ::: widget/gtk/mozgtk/gtk3/mozgtk.c @@ +1,4 @@ > +#include "mozilla/Types.h" > +#include "mozilla/Assertions.h" > + > +#define STUB(symbol) MOZ_EXPORT void symbol (void) { MOZ_CRASH("can't call __func__ from a GTK+3 process!"); } Same as gtk2. ::: widget/gtk/mozgtk/stub/mozgtk.c @@ +525,5 @@ > +STUB(gtk_window_set_wmclass) > +STUB(gtk_window_unfullscreen) > +STUB(gtk_window_unmaximize) > + > +// used by MOZ_WIDGET_GTK 2 code in dom/plugins/ipc/ and widget/gtkxtbin/ Come to think of it, it might be nice to have a single include file that does the STUB(foo) stuff, with sections for gtk2, gtk3 and both, and have the other c files include it with the right defines to get the set of stubs it needs.

Comment on attachment 8416411 [details] [diff] [review] Let plugin-container load libmozgtk2 Review of attachment 8416411 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the following fixed. ::: ipc/glue/GeckoChildProcessHost.cpp @@ +578,5 @@ > newEnvVars["LD_LIBRARY_PATH"] = path.get(); > } > + > +# if (MOZ_WIDGET_GTK == 3) > + const char *ld_preload = PR_GetEnv("LD_PRELOAD"); I think you have indentation problems (tab vs space?) ::: ipc/glue/moz.build @@ +141,5 @@ > > for var in ('MOZ_CHILD_PROCESS_NAME', 'MOZ_CHILD_PROCESS_BUNDLE'): > DEFINES[var] = '"%s"' % CONFIG[var] > > +for var in ('DLL_PREFIX', 'DLL_SUFFIX'): You can add both to the for above.

(In reply to Mike Hommey [:glandium] from comment #86) > You're saying this fails to build libxul for gtest. How does it fail? I have just verified that it happens in latest mozilla-central without my patches, so I have gone and opened bug 1005892.

(In reply to Mike Hommey [:glandium] from comment #86) > ::: browser/installer/package-manifest.in > @@ +123,5 @@ > > #endif > > +#ifdef MOZ_GTK3 > > +@BINPATH@/@DLL_PREFIX@mozgtk@DLL_SUFFIX@ > > +@BINPATH@/@DLL_PREFIX@mozgtk2@DLL_SUFFIX@ > > +#endif > > You'll need the same thing in b2g/installer/package-manifest.in I didn't know b2g could be built with gtk. I have added that, plus the necessary DEFINES (note that MOZ_GTK was missing there). > ::: widget/gtk/mozgtk/gtk2/Makefile.in > @@ +5,5 @@ > > +EXTRA_DSO_LDOPTS += -lgtk-x11-2.0 -lgdk-x11-2.0 > > + > > +include $(topsrcdir)/config/rules.mk > > + > > +CFLAGS += $(MOZ_GTK2_CFLAGS) > > Can leave the CFLAGS out. The sole c file doesn't need the gtk headers. Dropped. > ::: widget/gtk/mozgtk/gtk2/mozgtk.c > @@ +1,4 @@ > > +#include "mozilla/Types.h" > > +#include "mozilla/Assertions.h" > > + > > +#define STUB(symbol) MOZ_EXPORT void symbol (void) { MOZ_CRASH("can't call __func__ from a GTK+2 process!"); } > > I doubt __func__ works inside a literal string. You'd have to cut it in > pieces. You're right. And making that: "Can't call " __func__ " from ..." doesn't work because of the macro expansion. so I've just made it MOZ_CRASH() as the assertion will print the file and line number and possibly the function. > ::: widget/gtk/mozgtk/gtk3/Makefile.in > @@ +5,5 @@ > > +EXTRA_DSO_LDOPTS += -lgtk-3 -lgdk-3 > > + > > +include $(topsrcdir)/config/rules.mk > > + > > +CFLAGS += $(MOZ_GTK3_CFLAGS) > > Same as CFLAGS for gtk2. Dropped. > ::: widget/gtk/mozgtk/gtk3/mozgtk.c > @@ +1,4 @@ > > +#include "mozilla/Types.h" > > +#include "mozilla/Assertions.h" > > + > > +#define STUB(symbol) MOZ_EXPORT void symbol (void) { MOZ_CRASH("can't call __func__ from a GTK+3 process!"); } > > Same as gtk2. Ack. > ::: widget/gtk/mozgtk/stub/mozgtk.c > @@ +525,5 @@ > > +STUB(gtk_window_set_wmclass) > > +STUB(gtk_window_unfullscreen) > > +STUB(gtk_window_unmaximize) > > + > > +// used by MOZ_WIDGET_GTK 2 code in dom/plugins/ipc/ and widget/gtkxtbin/ > > Come to think of it, it might be nice to have a single include file that > does the STUB(foo) stuff, with sections for gtk2, gtk3 and both, and have > the other c files include it with the right defines to get the set of stubs > it needs. Sounds good. Done.

(In reply to Mike Hommey [:glandium] from comment #87) > Comment on attachment 8416411 [details] [diff] [review] > Let plugin-container load libmozgtk2 > > Review of attachment 8416411 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ with the following fixed. > > ::: ipc/glue/GeckoChildProcessHost.cpp > @@ +578,5 @@ > > newEnvVars["LD_LIBRARY_PATH"] = path.get(); > > } > > + > > +# if (MOZ_WIDGET_GTK == 3) > > + const char *ld_preload = PR_GetEnv("LD_PRELOAD"); > > I think you have indentation problems (tab vs space?) Right. Wrong number of spaces. Fixed. > ::: ipc/glue/moz.build > @@ +141,5 @@ > > > > for var in ('MOZ_CHILD_PROCESS_NAME', 'MOZ_CHILD_PROCESS_BUNDLE'): > > DEFINES[var] = '"%s"' % CONFIG[var] > > > > +for var in ('DLL_PREFIX', 'DLL_SUFFIX'): > > You can add both to the for above. Done.

(In reply to Mike Hommey [:glandium] from comment #75) > We should start looking at how this can be built and run on our try servers. I would say there should be a try server building for gtk+3 with --enable-default-toolkit=cairo-gtk3 --enable-system-cairo, and the gtk+2 try server should be kept around for the time where both gtk+ 2 and 3 configurations are supported, and dropped only after --enable-default-toolkit=cairo-gtk2 support is dropped. There is https://wiki.mozilla.org/Platform/GFX/GTK3#Builds_and_Tests but it doesn't say much.

(In reply to Emilio Pozuelo Monfort from comment #91) > I would say there should be a try server building for gtk+3 with > --enable-default-toolkit=cairo-gtk3 --enable-system-cairo, and the gtk+2 try No need to specify --enable-system-cairo - it's chosen by default in cairo-gtk3 target.

Comment on attachment 8417384 [details] [diff] [review] Add libmozgtk libraries so that libxul can use GTK+ 2 and 3 Review of attachment 8417384 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/installer/Makefile.in @@ +65,5 @@ > DEFINES += -DGKMEDIAS_SHARED_LIBRARY > endif > > +ifdef MOZ_WIDGET_GTK > +DEFINES += -DMOZ_GTK=1 If you're adding that, you might as well fix the MOZ_GTK2 that is in package-manifest.in. Because otherwise, it's just doing nothing.

I think there's a patch missing for the MOZ_WIDGET_GTK stuff.

Comment on attachment 8416408 [details] [diff] [review] Force OOP plugins on GTK3 (In reply to Emilio Pozuelo Monfort from comment #74) > > Considering this comment, I'd say MOZ_DISABLE_OOP_PLUGINS should be made > > ineffective a few lines above. > > I didn't do that because I thought MOZ_DISABLE_OOP_PLUGINS would only be > used by developers / testers when really necessary, and not all plugins will > fail (just the ones that link to gtk+), so I thought I would let people > override the behaviour with that env var. But I can change if you really > think we shouldn't allow that. I didn't know or had forgotten about this environment variable. I actually don't think this is going to be useful for debugging, because we want all plugins OOP, and I've seen too many crash reports where people didn't know that they had forced plugins in process. (I don't know whether they used the pref or environment variable.) Please move the MOZ_WIDGET_GTK == 3 conditional to the top of the function. The !aPluginTag block is not relevant as that was just an additional null check for safety when using the tag below mozilla-central changeset 6bef1538745f, and none of the callers can pass a null aPluginTag anyway. Please also use an #else to explicitly disable compilation of the rest of the function. I think this would be clearer and I'm a little worried about unreachable code warnings.

(In reply to Karl Tomlinson (:karlt, back May 5) from comment #95) > Please move the MOZ_WIDGET_GTK == 3 conditional to the top of the function. I mean the whole conditional block, not just the test.

(In reply to Emilio Pozuelo Monfort from comment #68) > Do you mean dropping the #if (MOZ_WIDGET_GTK == 3) code? Yes. That's what I meant. As in the dom/plugins/ipc parts of attachment 8412399 [details] [diff] [review].

(In reply to Mike Hommey [:glandium] from comment #93) > Comment on attachment 8417384 [details] [diff] [review] > Add libmozgtk libraries so that libxul can use GTK+ 2 and 3 > > Review of attachment 8417384 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: b2g/installer/Makefile.in > @@ +65,5 @@ > > DEFINES += -DGKMEDIAS_SHARED_LIBRARY > > endif > > > > +ifdef MOZ_WIDGET_GTK > > +DEFINES += -DMOZ_GTK=1 > > If you're adding that, you might as well fix the MOZ_GTK2 that is in > package-manifest.in. Because otherwise, it's just doing nothing. Aye, done (I thought the manifest file was using MOZ_GTK, not MOZ_GTK2).

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #95) > Comment on attachment 8416408 [details] [diff] [review] > Force OOP plugins on GTK3 > > (In reply to Emilio Pozuelo Monfort from comment #74) > > > Considering this comment, I'd say MOZ_DISABLE_OOP_PLUGINS should be made > > > ineffective a few lines above. > > > > I didn't do that because I thought MOZ_DISABLE_OOP_PLUGINS would only be > > used by developers / testers when really necessary, and not all plugins will > > fail (just the ones that link to gtk+), so I thought I would let people > > override the behaviour with that env var. But I can change if you really > > think we shouldn't allow that. > > I didn't know or had forgotten about this environment variable. > > I actually don't think this is going to be useful for debugging, because we > want all plugins OOP, and I've seen too many crash reports where people > didn't > know that they had forced plugins in process. (I don't know whether they > used > the pref or environment variable.) > > Please move the MOZ_WIDGET_GTK == 3 conditional to the top of the function. > The !aPluginTag block is not relevant as that was just an additional null > check for safety when using the tag below mozilla-central changeset > 6bef1538745f, and none of the callers can pass a null aPluginTag anyway. > > Please also use an #else to explicitly disable compilation of the rest of the > function. I think this would be clearer and I'm a little worried about > unreachable code warnings. Done.

(In reply to Mike Hommey [:glandium] from comment #70) > ::: widget/gtkxtbin/gtkdefine.h > @@ +1,2 @@ > > +#undef MOZ_WIDGET_GTK > > +#define MOZ_WIDGET_GTK 2 > > I must insist that this is too hackish. And in fact, looking at it more > throroughly, it appears it's actually dangerous, due to what effect changing > MOZ_WIDGET_GTK has on some headers and on some code. Any code that is #if > (MOZ_WIDGET_GTK == 2) or #if (MOZ_WIDGET_GTK == 3) needs to be thoroughly > inspected, and at the very least I suspect issues with code under > dom/plugins/base. Ack. > In fact, there are various #if (MOZ_WIDGET_GTK == 2) and #if (MOZ_WIDGET_GTK > == 3) sections that make no sense now that some code is meant to always be > built with gtk2. And there is one specific portion that is #if > (MOZ_WIDGET_GTK == 2) that seems dangerous in PluginInstanceChild.h. Sure, > it's for private members of the class, so it only really matters for the > implementation of the class, but who knows how the code is going to evolve? > I wouldn't be surprised with some inline function accessing a member added > at the end of the class in the future. Which would lead to code using that > function to access the wrong offset if used in code built against gtk3 when > the implementation for the most of the class is built against gtk2. There > are things I'm concerned with in dom/plugins/base, too, which I suspect at > least some portions to be for the plugin container. > > So all in all, I think a prerequisite to this bug is to clear out all uses > of the MOZ_WIDGET_GTK macro as a number, and carefully identifying if we > mean to use the gtk2 or the gtk3 api/abi for each, depending whether they're > meant to run in the parent process, or in the plugin process. Code that's > exclusively for the parent process, but with support for both gtk2 and gtk3, > GTK_MAJOR_VERSION should be used. Code that's exclusively for the child > process should use the gtk2 code, and code that ends up used in both, well, > probably gtk2 too. > > I'm tempted to say we should clearly separate code for parent process and > code for plugin process, but that's probably daunting. So, widget/ gfx/ toolkit/ -> those are mostly for gtk+2 or gtk+3 support, we want to keep supporting both so should be fine. dom/plugins/ipc/ -> no more conditional uses with this patch. we build with gtk+2 cflags unconditionally for the *Child classes, but there are *Parent classes as well. The only g[dt]k call there is gdk_pointer_ungrab() in PluginInstanceParent.cpp which isn't a problem (no g[dt]k struct access, just a function call) but if they were to grow some gtk usage, we could run into problems. But right now I think we're good. dom/plugins/base/ -> There are various unsorted conditional usages here: - nsNPAPIPlugin.cpp has the Xt plugin stuff, only for gtk+2 atm. - nsPluginsDirUnix.cpp seems to load libXt in a hacky way for the Xt plugins, again for gtk+2 only. - nsPluginStreamListenerPeer.cpp has a conditional for gtk+2 or qt, but it's not clear to me why. - nsPluginNativeWindowGtk.cpp: Xt again. So I think we need to figure out what to do with Xt, and we're basically done. Unfortunately I had issues with getting acroread to work because it's 32 bits, and the vlc plugin no longer seems to use Xt... Thoughts?

(In reply to Emilio Pozuelo Monfort from comment #100) > Created attachment 8425823 [details] [diff] [review] > Clean up MOZ_WIDGET_GTK usages FWIW I've tested this patch with both gtk+ 2 and gtk+ 3, and both versions still work fine and load plugins (flash).

(In reply to Emilio Pozuelo Monfort from comment #101) > (In reply to Emilio Pozuelo Monfort from comment #100) > > Created attachment 8425823 [details] [diff] [review] > > Clean up MOZ_WIDGET_GTK usages > > FWIW I've tested this patch with both gtk+ 2 and gtk+ 3, and both versions > still work fine and load plugins (flash). Sorry,i not have widget/gtk/mozgtk/ folder needed for this patch, other files was correctly patched. Gtk3 I receive this: http://pastebin.com/1PvfqNmQ Without patch i compiled correctly (flash missing)

Comment on attachment 8425823 [details] [diff] [review] Clean up MOZ_WIDGET_GTK usages Review of attachment 8425823 [details] [diff] [review]: ----------------------------------------------------------------- Looks a lot like my patch :) ::: dom/plugins/base/nsPluginNativeWindowGtk.cpp @@ +23,1 @@ > #include "gtk2xtbin.h" cf. https://bugzilla.mozilla.org/attachment.cgi?id=8412399 gtk2xtbin.h is useless here. ::: dom/plugins/ipc/PluginInstanceChild.cpp @@ -1036,5 @@ > xt_client_create(&mXtClient, browserSocket, mWindow.width, mWindow.height); > mWindow.window = (void *)XtWindow(mXtClient.child_widget); > } > -#else > - mWindow.window = reinterpret_cast<void*>(aWindow.window); This #else might be used by the Qt port.

(In reply to Emilio Pozuelo Monfort from comment #100) > Created attachment 8425823 [details] [diff] [review] > Clean up MOZ_WIDGET_GTK usages > > (In reply to Mike Hommey [:glandium] from comment #70) > > ::: widget/gtkxtbin/gtkdefine.h > > @@ +1,2 @@ > > > +#undef MOZ_WIDGET_GTK > > > +#define MOZ_WIDGET_GTK 2 > > > > I must insist that this is too hackish. And in fact, looking at it more > > throroughly, it appears it's actually dangerous, due to what effect changing > > MOZ_WIDGET_GTK has on some headers and on some code. Any code that is #if > > (MOZ_WIDGET_GTK == 2) or #if (MOZ_WIDGET_GTK == 3) needs to be thoroughly > > inspected, and at the very least I suspect issues with code under > > dom/plugins/base. > > Ack. > > > In fact, there are various #if (MOZ_WIDGET_GTK == 2) and #if (MOZ_WIDGET_GTK > > == 3) sections that make no sense now that some code is meant to always be > > built with gtk2. And there is one specific portion that is #if > > (MOZ_WIDGET_GTK == 2) that seems dangerous in PluginInstanceChild.h. Sure, > > it's for private members of the class, so it only really matters for the > > implementation of the class, but who knows how the code is going to evolve? > > I wouldn't be surprised with some inline function accessing a member added > > at the end of the class in the future. Which would lead to code using that > > function to access the wrong offset if used in code built against gtk3 when > > the implementation for the most of the class is built against gtk2. There > > are things I'm concerned with in dom/plugins/base, too, which I suspect at > > least some portions to be for the plugin container. > > > > So all in all, I think a prerequisite to this bug is to clear out all uses > > of the MOZ_WIDGET_GTK macro as a number, and carefully identifying if we > > mean to use the gtk2 or the gtk3 api/abi for each, depending whether they're > > meant to run in the parent process, or in the plugin process. Code that's > > exclusively for the parent process, but with support for both gtk2 and gtk3, > > GTK_MAJOR_VERSION should be used. Code that's exclusively for the child > > process should use the gtk2 code, and code that ends up used in both, well, > > probably gtk2 too. > > > > I'm tempted to say we should clearly separate code for parent process and > > code for plugin process, but that's probably daunting. > > > So, > > widget/ > gfx/ > toolkit/ > -> those are mostly for gtk+2 or gtk+3 support, we want to keep supporting > both so should be fine. Those should switch from MOZ_WIDGET_GTK to GTK_MAJOR_VERSION, but that can be done in a followup bug. > dom/plugins/ipc/ > -> no more conditional uses with this patch. we build with gtk+2 cflags > unconditionally for the *Child classes, but there are *Parent classes as > well. The only g[dt]k call there is gdk_pointer_ungrab() in > PluginInstanceParent.cpp which isn't a problem (no g[dt]k struct access, > just a function call) but if they were to grow some gtk usage, we could run > into problems. But right now I think we're good. > > dom/plugins/base/ > -> There are various unsorted conditional usages here: > - nsNPAPIPlugin.cpp has the Xt plugin stuff, only for gtk+2 atm. > - nsPluginsDirUnix.cpp seems to load libXt in a hacky way for the Xt > plugins, again for gtk+2 only. > - nsPluginStreamListenerPeer.cpp has a conditional for gtk+2 or qt, but it's > not clear to me why. > - nsPluginNativeWindowGtk.cpp: Xt again. > > So I think we need to figure out what to do with Xt, and we're basically > done. Unfortunately I had issues with getting acroread to work because it's > 32 bits, and the vlc plugin no longer seems to use Xt... > > Thoughts? The bothering thing in those directories is that there are files that are meant to run in the plugin container and files meant to run in the main process. And they may each require a different gtk version.

(In reply to Mike Hommey [:glandium] from comment #103) > Comment on attachment 8425823 [details] [diff] [review] > Clean up MOZ_WIDGET_GTK usages > > Review of attachment 8425823 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks a lot like my patch :) Heh, yeah. I based it on yours. I've given proper credit now > ::: dom/plugins/base/nsPluginNativeWindowGtk.cpp > @@ +23,1 @@ > > #include "gtk2xtbin.h" > > cf. https://bugzilla.mozilla.org/attachment.cgi?id=8412399 gtk2xtbin.h is > useless here. I added it back as removing it broke the gtk2 build, as there are some gtk2xtbin usages, e.g. #if (MOZ_WIDGET_GTK == 2) if (GTK_IS_XTBIN(mSocketWidget)) { > ::: dom/plugins/ipc/PluginInstanceChild.cpp > @@ -1036,5 @@ > > xt_client_create(&mXtClient, browserSocket, mWindow.width, mWindow.height); > > mWindow.window = (void *)XtWindow(mXtClient.child_widget); > > } > > -#else > > - mWindow.window = reinterpret_cast<void*>(aWindow.window); > > This #else might be used by the Qt port. Ah, good point. Restored.

(In reply to Emilio Pozuelo Monfort from comment #105) > Created attachment 8426962 [details] [diff] [review] > Clean up MOZ_WIDGET_GTK usages Ping? Does this new version look good? And if so, can we land this series now, or do you think we should block on something else?

Comment on attachment 8426962 [details] [diff] [review] Clean up MOZ_WIDGET_GTK usages Review of attachment 8426962 [details] [diff] [review]: ----------------------------------------------------------------- LGTM but I'd rather Karl looked at it too.

Updated patch for latest mozilla-central (just solved minor conflicts on the manifest files)

Updated patch for latest mozilla-central (solved minor conflicts)

Comment on attachment 8426962 [details] [diff] [review] Clean up MOZ_WIDGET_GTK usages >-#if (MOZ_WIDGET_GTK == 3) >+#if (GTK_MAJOR_VERSION == 3) > #include <gtk/gtkx.h> > #endif Can you see whether this block (and this include) can be removed, please? That might help protect from accidentally using this with GTK3.

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #110) > Comment on attachment 8426962 [details] [diff] [review] > Clean up MOZ_WIDGET_GTK usages > > >-#if (MOZ_WIDGET_GTK == 3) > >+#if (GTK_MAJOR_VERSION == 3) > > #include <gtk/gtkx.h> > > #endif > > Can you see whether this block (and this include) can be removed, please? > That might help protect from accidentally using this with GTK3. It could be removed as gtkxtbin itself is built against GTK+2. However gtk2xtbin.h gets indirectly included when building some other files against GTK+3, making the build fail. E.g. when building dom/ipc/CrashReporterChild.cpp: 0:18.09 In file included from ../../dist/include/mozilla/plugins/PluginInstanceChild.h:37:0, 0:18.09 from ../../dist/include/mozilla/plugins/PluginModuleChild.h:32, 0:18.09 from /home/emilio/src/mozilla/mozilla-central/dom/ipc/CrashReporterChild.cpp:6: 0:18.09 ../../dist/include/gtk2xtbin.h:56:3: error: ‘GtkSocket’ does not name a type 0:18.09 In file included from ../../dist/include/mozilla/plugins/PluginInstanceChild.h:37:0, 0:18.09 from ../../dist/include/mozilla/plugins/PluginModuleChild.h:32, 0:18.09 from /home/emilio/src/mozilla/mozilla-central/dom/ipc/CrashReporterChild.cpp:6: 0:18.09 ../../dist/include/gtk2xtbin.h:56:3: error: ‘GtkSocket’ does not name a type PluginInstanceChild.h includes gtk2xtbin.h for the definition of XtClient. FWIW as I have said before, I haven't been able to test the xtbin codepath as I couldn't install a plugin that used Xt. So although things build fine now, I don't know if Xt plugins actually work on GTK3.

(In reply to Emilio Pozuelo Monfort from comment #111) > PluginInstanceChild.h includes gtk2xtbin.h for the definition of XtClient. Ah, thanks. I see what Mike was talking about now. One option might be to wrap this block with "#if (GTK_MAJOR_VERSION == 2)": http://mxr.mozilla.org/mozilla-central/source/widget/gtkxtbin/gtk2xtbin.h?rev=94c819a423ff&mark=57-73#57 (Then the <gtk/gtkx.h> include won't be necessary.) But if that doesn't work out, then don't worry about it. > FWIW as I have said before, I haven't been able to test the xtbin codepath > as I couldn't install a plugin that used Xt. So although things build fine > now, I don't know if Xt plugins actually work on GTK3. That's OK.

My patch was splitting gtk2xtbin.h in two headers to make it work.

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #112) > (In reply to Emilio Pozuelo Monfort from comment #111) > > PluginInstanceChild.h includes gtk2xtbin.h for the definition of XtClient. > > Ah, thanks. I see what Mike was talking about now. > > One option might be to wrap this block with "#if (GTK_MAJOR_VERSION == 2)": > > http://mxr.mozilla.org/mozilla-central/source/widget/gtkxtbin/gtk2xtbin. > h?rev=94c819a423ff&mark=57-73#57 > > (Then the <gtk/gtkx.h> include won't be necessary.) > > But if that doesn't work out, then don't worry about it. Ah yes, that works. I have also included the typedefs and the defines inside the #if block. (In reply to Mike Hommey [:glandium] from comment #113) > My patch was splitting gtk2xtbin.h in two headers to make it work. Oh. I guess I missed that. I don't see it in attachment 8412399 [details] [diff] [review], but maybe it was in another patch or you just mentioned it... How does this new version look?

Comment on attachment 8429990 [details] [diff] [review] patch #5: Clean up MOZ_WIDGET_GTK usages Looks good, thanks. (In MOZ_WIDGET_GTK == 3 builds, only XtClient is used and none of the GtkXtBin code, even in the GTK2 plugin container, but that can be cleaned out when MOZ_WIDGET_GTK == 2 builds are no longer required.)

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #115) > Comment on attachment 8429990 [details] [diff] [review] > Clean up MOZ_WIDGET_GTK usages > > Looks good, thanks. Great! Can this series be committed now?

Are you able to push the changes to the try server [1], please, with syntax for a linux build, to confirm our build machines will still build with GTK2. https://wiki.mozilla.org/ReleaseEngineering/TryServer Then post the link here, check that the patches are attached in appropriate order for applying, or otherwise label the attachments to make the order clear, and add the checkin-needed keyword.

Comment on attachment 8429990 [details] [diff] [review] patch #5: Clean up MOZ_WIDGET_GTK usages No need for my review here.

Try: https://tbpl.mozilla.org/?tree=Try&rev=04f71848e61b

Comment on attachment 8429363 [details] [diff] [review] patch #3: Add libmozgtk libraries so that libxul can use GTK+ 2 and 3 This version applies on latest mozilla-central. No changes to the old one, just solved a conflict.

Comment on attachment 8429364 [details] [diff] [review] patch #4: Let plugin-container load libmozgtk2 Again, this is just to solve a conflict. No code changes wrt the previous version.

(In reply to Emilio Pozuelo Monfort from comment #119) > Try: https://tbpl.mozilla.org/?tree=Try&rev=04f71848e61b Looks like mochitests bc1 failed, but I see those failing a lot in previous builds, including in mozilla-central at https://tbpl.mozilla.org/, so those don't look like a regression from these patches. mochitests 5 failed as well but those look like intermitent failures, e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=950636

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4fe0b4e76d https://hg.mozilla.org/integration/mozilla-inbound/rev/b5c10596daae https://hg.mozilla.org/integration/mozilla-inbound/rev/8b27e157b268 https://hg.mozilla.org/integration/mozilla-inbound/rev/7d49f8f7d3e3 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d69b8234aac

https://hg.mozilla.org/mozilla-central/rev/4c4fe0b4e76d https://hg.mozilla.org/mozilla-central/rev/b5c10596daae https://hg.mozilla.org/mozilla-central/rev/8b27e157b268 https://hg.mozilla.org/mozilla-central/rev/7d49f8f7d3e3 https://hg.mozilla.org/mozilla-central/rev/8d69b8234aac

Wow! Works great, tested on Fedora 20/x86_64/latest trunk and patch from Bug 968196. I see only small performance lags on youtube when video starts...maybe some cache/initial link delay or caused by debug build? It may be also good to have fixed Bug 959224 too - but I haven't see any complains about it yet.

(In reply to Martin Stránský from comment #125) > Wow! Works great, tested on Fedora 20/x86_64/latest trunk and patch from Bug > 968196. Great! I'm glad it's working well. > I see only small performance lags on youtube when video > starts...maybe some cache/initial link delay or caused by debug build? When youtube is loading, flash shows a spinner, which stops spinning (as if the UI was freezing) for a little bit (less than a second) a few times. Once loading is finishes, those lags seem to be gone, video playback seems to be smooth. I have tried on mozilla-central building against gtk2, and I see the same "lags" while youtube videos load. Since my patches are unused in the gtk2 builds, I don't think this is a regression from this bug, but it was probably introduced a while ago. Running Firefox 29.0.1 (Iceweasel from Debian) which uses gtk2, I don't see those lags, so this is probably a regression between Firefox 29 and latest trunk. It may be possible to bisect it, but let's track this in a separate bug as it doesn't look like a regression from this bug. I've opened bug #1019025 for it.