1159273 - GTK3 toolkit build requires system Cairo

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Lee Salzman [:lsalzman]]

Attachment: Allow loading of tree Cairo and system Cairo at the same time

When --enable-default-toolkit=cairo-gtk3 is required, it requires --enable-system-cairo to also be used so that Cairo structs can be passed back and forth between our code and GTK3. This is because our own tree Cairo, despite some renaming code, is bit-rotten and not properly isolating all symbols anymore. Recent usage of Harfbuzz is also exporting conflicting symbols even with GTK2 builds as well (and in GTK3 builds).

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: Allow loading of tree Cairo and system Cairo at the same time

fix missing system-cairo.h file

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: Allow loading of tree Cairo and system Cairo at the same time

simplified the idea to preserve header locations

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: Allow loading of tree Cairo and system Cairo at the same time

Fix symbol visibility issue for Cairo headers

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8599580 [details] [diff] [review]
Allow loading of tree Cairo and system Cairo at the same time

Review of attachment 8599580 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me, but needs build peer review and probably someone from harfbuzz too.

Comment 5

[Jonathan Kew [:jfkthame]]

This looks like bit of a maintenance headache; is there some way we can avoid touching hb.h (which will get overwritten whenever we take a new harfbuzz version)? Maybe by using the -include option during compilation?

And whenever harfbuzz adds new API (e.g. a new public API was just landed upstream today!), we'll need to remember to add corresponding entries to the renaming file. Ugh.

If we have to, we have to. But if there's any way to avoid this, I'd really appreciate it!

Comment 6

[Lee Salzman [:lsalzman]]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> This looks like bit of a maintenance headache; is there some way we can
> avoid touching hb.h (which will get overwritten whenever we take a new
> harfbuzz version)? Maybe by using the -include option during compilation?
> 
> And whenever harfbuzz adds new API (e.g. a new public API was just landed
> upstream today!), we'll need to remember to add corresponding entries to the
> renaming file. Ugh.
> 
> If we have to, we have to. But if there's any way to avoid this, I'd really
> appreciate it!

The linker was reporting that despite the symbols being hidden other external libraries (that were linked against system harfbuzz) were pulling symbols from tree harfbuzz. If there was a good solution to this issue that did not involve renaming, would we not have been able to avoid the renaming solution in both cairo and pixman too?

Comment 7

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman from comment #6)
> (In reply to Jonathan Kew (:jfkthame) from comment #5)
> > This looks like bit of a maintenance headache; is there some way we can
> > avoid touching hb.h (which will get overwritten whenever we take a new
> > harfbuzz version)? Maybe by using the -include option during compilation?
> > 
> > And whenever harfbuzz adds new API (e.g. a new public API was just landed
> > upstream today!), we'll need to remember to add corresponding entries to the
> > renaming file. Ugh.
> > 
> > If we have to, we have to. But if there's any way to avoid this, I'd really
> > appreciate it!
> 
> The linker was reporting that despite the symbols being hidden other
> external libraries (that were linked against system harfbuzz) were pulling
> symbols from tree harfbuzz. If there was a good solution to this issue that
> did not involve renaming, would we not have been able to avoid the renaming
> solution in both cairo and pixman too?

The warnings were of the following form, a bunch of different harfbuzz symbols:

 0:29.08 ../../build/unix/gold/ld: warning: hidden symbol 'hb_font_destroy' in /home/lee/central/obj-gtk3/toolkit/library/../../gfx/harfbuzz/src/Unified_cpp_gfx_harfbuzz_src0.o is referenced by DSO /usr/lib/gcc/x86_64-linux-gnu/4.9/../../../x86_64-linux-gnu/libpangoft2-1.0.so

The main offender here is libpangoft2. I tried explicitly adding harfbuzz to the linker args via -lharfbuzz but no luck, it still spews the same warnings.

On the flip side, why do we need a tree harfbuzz in the first place? Couldn't we just use the system harfbuzz in such cases and avoid the whole issue?

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to Lee Salzman from comment #7)

> On the flip side, why do we need a tree harfbuzz in the first place?
> Couldn't we just use the system harfbuzz in such cases and avoid the whole
> issue?

See bug 847568. That's a possibility, though we do tend to update in-tree harfbuzz pretty aggressively (for any upstream changes that are relevant to our usage), and it's likely that some users' systems will have older versions. E.g., we're about to take an update to fix bug 1157758, and if we run on a system with an older harfbuzz, that bug will still manifest itself for those users. But maybe that's an acceptable situation.

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: Allow loading of tree Cairo and system Cairo at the same time

I separated out all harfbuzz changes from this patch so only the existing non-controversial Cairo changes remain

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8600208 [details] [diff] [review]
Allow loading of tree Cairo and system Cairo at the same time

Review of attachment 8600208 [details] [diff] [review]:
-----------------------------------------------------------------

> This is because our own tree Cairo, despite some renaming code, is bit-rotten and not properly isolating all symbols anymore.

Is that really all there is to it? I thought the problem was that we would pass cairo objects from the system cairo through gtk to the in-tree cairo, or the other way around, and /that/ wouldn't work. But maybe that changed, and we can, in fact, now build with in-tree cairo, I don't know.

Also, technically, the symbol renaming is not strictly necessary, as long as the symbols we *do* need from system cairo are renamed (and there are only 12 of them, all of which were already in the list). That would lead to the linker complaining, like it does for harfbuzz, but in practice, it doesn't change the fact that the (internal) hidden symbols are what's used internally. So I really doubt your claim not being able to use in-tree cairo with gtk3 was due to symbols isolation is true.

IOW, if this does work, I'm skeptical it is because of the changes to *-rename.h.

In any case, please split this patch between clean-ups that are unrelated and things to make building with in-tree cairo work, and rerequest review. In the meanwhile, I'll do a bunch of try pushes to see if this does any good to the test suite.

::: gfx/thebes/gfxFontTest.h
@@ -9,4 @@
>  #include "nsString.h"
>  #include "nsTArray.h"
>  
> -#include "cairo/cairo.h"

Seems to me the changes in gfxFontTest.h should be separate.

::: layout/media/symbols.def.in
@@ -539,5 @@
> -cairo_win32_get_system_text_quality
> -cairo_win32_surface_create_with_alpha
> -cairo_win32_surface_get_height
> -cairo_win32_surface_get_width
> -cairo_win32_surface_set_can_convert_to_dib

Seems to me the changes in symbols.def.in could be separate (and they are a no-op in practice).

That said, does building with GKMEDIAS_SHARED_LIBRARY still actually works on windows? It might be worth removing it if it doesn't. (it's not been the default for a while, and I don't know if people are using it locally)

::: widget/gtk/moz.build
@@ +98,5 @@
>  
>  CFLAGS += CONFIG['MOZ_STARTUP_NOTIFICATION_CFLAGS']
> +
> +if not CONFIG['MOZ_ENABLE_GTK3'] or not CONFIG['MOZ_TREE_CAIRO']:
> +    CXXFLAGS += CONFIG['MOZ_CAIRO_CFLAGS']

This really needs a comment saying that for in-tree cairo gtk3 builds, the goal is to *not* use the in-tree headers, but use the system headers instead.

::: widget/gtk/nsWindow.cpp
@@ -102,4 @@
>  #include "nsAutoPtr.h"
>  #include "ClientLayerManager.h"
>  
> -extern "C" {

Seems to me the changes in nsWindow* should be separate.

Comment 11

[Mike Hommey [:glandium]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a40f8ffb3dc8

This makes things significantly worse than they are on elm:
https://treeherder.mozilla.org/#/jobs?repo=elm

Comment 12

[Lee Salzman [:lsalzman]]

(In reply to Mike Hommey [:glandium] from comment #10)
> Comment on attachment 8600208 [details] [diff] [review]
> Allow loading of tree Cairo and system Cairo at the same time
> 
> Review of attachment 8600208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > This is because our own tree Cairo, despite some renaming code, is bit-rotten and not properly isolating all symbols anymore.
> 
> Is that really all there is to it? I thought the problem was that we would
> pass cairo objects from the system cairo through gtk to the in-tree cairo,
> or the other way around, and /that/ wouldn't work. But maybe that changed,
> and we can, in fact, now build with in-tree cairo, I don't know.

That is the problem. But to properly ensure that they two Cairos are isolated
requires that symbols don't leak. Any symbols that we DON'T rename are picked up
by external libraries linking against Cairo, and if we only rename symbols
piece-meal, then external libraries using external Cairo will use some of their
symbols and some of ours. 

If the private/internal structs of various Cairos are assumed different in these
various functions. A struct allocated by one Cairo that is used by the other is not
safe, and the result with Cairo when we tried to pass our Cairo objects to system
Cairo was crashing.

That is the reason why I originally tried to isolate our harfbuzz as well, because
that is still happening with our harfbuzz, which is just not safe either.

> 
> Also, technically, the symbol renaming is not strictly necessary, as long as
> the symbols we *do* need from system cairo are renamed (and there are only
> 12 of them, all of which were already in the list). That would lead to the
> linker complaining, like it does for harfbuzz, but in practice, it doesn't
> change the fact that the (internal) hidden symbols are what's used
> internally. So I really doubt your claim not being able to use in-tree cairo
> with gtk3 was due to symbols isolation is true.

See above comment. The problem is if the external library that was linked against
system Cairo pulls 12 of our symbols, but the rest from the system Cairo, all hell
breaks loose. The renaming solves this so our symbols don't leak, so external libraries
that were linked against system Cairo always use system Cairo's symbols. It covers
our asses.

> IOW, if this does work, I'm skeptical it is because of the changes to
> *-rename.h.

In the interest of ensuring the breakage does not happen spontaneously in the future,
as well as fixing the crashes that happen now, these were bundled up.

> In any case, please split this patch between clean-ups that are unrelated
> and things to make building with in-tree cairo work, and rerequest review.
> In the meanwhile, I'll do a bunch of try pushes to see if this does any good
> to the test suite.

I can/will debundle them, but all of these changes I would encourage are technically
needed to keep the full solution working.

> ::: gfx/thebes/gfxFontTest.h
> @@ -9,4 @@
> >  #include "nsString.h"
> >  #include "nsTArray.h"
> >  
> > -#include "cairo/cairo.h"
> 
> Seems to me the changes in gfxFontTest.h should be separate.

Okay, but if when using system Cairo, there will be no "cairo/cairo.h",
because no system wrapper header will be generated in the dist/include/cairo
directly which that file accidentally pulls from, and the build will fail
unless stale system wrappers are left in the dist/include dir by accident.
The actual pkg-config --cflags cairo does not use -I/usr/include but rather
-I/usr/include/cairo. 

> ::: layout/media/symbols.def.in
> @@ -539,5 @@
> > -cairo_win32_get_system_text_quality
> > -cairo_win32_surface_create_with_alpha
> > -cairo_win32_surface_get_height
> > -cairo_win32_surface_get_width
> > -cairo_win32_surface_set_can_convert_to_dib
> 
> Seems to me the changes in symbols.def.in could be separate (and they are a
> no-op in practice).
> 
> That said, does building with GKMEDIAS_SHARED_LIBRARY still actually works
> on windows? It might be worth removing it if it doesn't. (it's not been the
> default for a while, and I don't know if people are using it locally)

No idea if it is needed anymore, but it's in-tree, and it was using the symbols,
so in the interest of thoroughness, I updated the symbols to track with the
renaming so as not to break that in case it was needed.
 
> ::: widget/gtk/moz.build
> @@ +98,5 @@
> >  
> >  CFLAGS += CONFIG['MOZ_STARTUP_NOTIFICATION_CFLAGS']
> > +
> > +if not CONFIG['MOZ_ENABLE_GTK3'] or not CONFIG['MOZ_TREE_CAIRO']:
> > +    CXXFLAGS += CONFIG['MOZ_CAIRO_CFLAGS']
> 
> This really needs a comment saying that for in-tree cairo gtk3 builds, the
> goal is to *not* use the in-tree headers, but use the system headers instead.

Okay.

> ::: widget/gtk/nsWindow.cpp
> @@ -102,4 @@
> >  #include "nsAutoPtr.h"
> >  #include "ClientLayerManager.h"
> >  
> > -extern "C" {
> 
> Seems to me the changes in nsWindow* should be separate.

Okay.

Comment 13

[Lee Salzman [:lsalzman]]

Attachment: Update build system to allow using in-tree Cairo with GTK3 enabled

Comment 14

[Lee Salzman [:lsalzman]]

Attachment: Update Cairo/Pixman renaming to not leak the symbols we use and silence linker warnings

Comment 15

[Lee Salzman [:lsalzman]]

Attachment: Update gkmedias symbol defs to catch up with the Cairo/Pixman renames

Comment 16

[Lee Salzman [:lsalzman]]

Attachment: Fix out-dated Cairo usage that may fail to build or work properly when system Cairo is involved

Comment 17

[Lee Salzman [:lsalzman]]

(In reply to Mike Hommey [:glandium] from comment #11)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a40f8ffb3dc8
> 
> This makes things significantly worse than they are on elm
> https://treeherder.mozilla.org/#/jobs?repo=elm

The reason the builds crash with just these changes is because of bug 1127752, which includes a fix for the separate issue of the GTK3 widget painting being buggy. Those fixes were also originally bundled with these changes that as one, so that the whole solution worked, but I was encouraged to split those off into a separate issue. I would recommend that the patches from that bug are applied after these patches to encompass the desired effect.

Comment 18

[Mike Hommey [:glandium]]

(In reply to Lee Salzman from comment #12)
> (In reply to Mike Hommey [:glandium] from comment #10)
> > Comment on attachment 8600208 [details] [diff] [review]
> > Allow loading of tree Cairo and system Cairo at the same time
> > 
> > Review of attachment 8600208 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > > This is because our own tree Cairo, despite some renaming code, is bit-rotten and not properly isolating all symbols anymore.
> > 
> > Is that really all there is to it? I thought the problem was that we would
> > pass cairo objects from the system cairo through gtk to the in-tree cairo,
> > or the other way around, and /that/ wouldn't work. But maybe that changed,
> > and we can, in fact, now build with in-tree cairo, I don't know.
> 
> That is the problem. But to properly ensure that they two Cairos are isolated
> requires that symbols don't leak. Any symbols that we DON'T rename are
> picked up
> by external libraries linking against Cairo, and if we only rename symbols
> piece-meal, then external libraries using external Cairo will use some of
> their
> symbols and some of ours. 

No, because ours are hidden and thus not exported. System libraries can *not* pick our symbols even if they aren't renamed. That is true for cairo, and that is true for harfbuzz.

Comment 19

[Lee Salzman [:lsalzman]]

(In reply to Mike Hommey [:glandium] from comment #18)
> (In reply to Lee Salzman from comment #12)
> > (In reply to Mike Hommey [:glandium] from comment #10)
> > > Comment on attachment 8600208 [details] [diff] [review]
> > > Allow loading of tree Cairo and system Cairo at the same time
> > > 
> > > Review of attachment 8600208 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > > This is because our own tree Cairo, despite some renaming code, is bit-rotten and not properly isolating all symbols anymore.
> > > 
> > > Is that really all there is to it? I thought the problem was that we would
> > > pass cairo objects from the system cairo through gtk to the in-tree cairo,
> > > or the other way around, and /that/ wouldn't work. But maybe that changed,
> > > and we can, in fact, now build with in-tree cairo, I don't know.
> > 
> > That is the problem. But to properly ensure that they two Cairos are isolated
> > requires that symbols don't leak. Any symbols that we DON'T rename are
> > picked up
> > by external libraries linking against Cairo, and if we only rename symbols
> > piece-meal, then external libraries using external Cairo will use some of
> > their
> > symbols and some of ours. 
> 
> No, because ours are hidden and thus not exported. System libraries can
> *not* pick our symbols even if they aren't renamed. That is true for cairo,
> and that is true for harfbuzz.

Okay, but then why was the linker warning otherwise until I did the renames? i.e.

0:29.08 ../../build/unix/gold/ld: warning: hidden symbol 'hb_font_destroy' in /home/lee/central/obj-gtk3/toolkit/library/../../gfx/harfbuzz/src/Unified_cpp_gfx_harfbuzz_src0.o is referenced by DSO /usr/lib/gcc/x86_64-linux-gnu/4.9/../../../x86_64-linux-gnu/libpangoft2-1.0.so

Still looking into the mochitest failures as some are indeed failing.

Comment 20

[Mike Hommey [:glandium]]

(In reply to Lee Salzman from comment #19)
> (In reply to Mike Hommey [:glandium] from comment #18)
> > (In reply to Lee Salzman from comment #12)
> > > (In reply to Mike Hommey [:glandium] from comment #10)
> > > > Comment on attachment 8600208 [details] [diff] [review]
> > > > Allow loading of tree Cairo and system Cairo at the same time
> > > > 
> > > > Review of attachment 8600208 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > > This is because our own tree Cairo, despite some renaming code, is bit-rotten and not properly isolating all symbols anymore.
> > > > 
> > > > Is that really all there is to it? I thought the problem was that we would
> > > > pass cairo objects from the system cairo through gtk to the in-tree cairo,
> > > > or the other way around, and /that/ wouldn't work. But maybe that changed,
> > > > and we can, in fact, now build with in-tree cairo, I don't know.
> > > 
> > > That is the problem. But to properly ensure that they two Cairos are isolated
> > > requires that symbols don't leak. Any symbols that we DON'T rename are
> > > picked up
> > > by external libraries linking against Cairo, and if we only rename symbols
> > > piece-meal, then external libraries using external Cairo will use some of
> > > their
> > > symbols and some of ours. 
> > 
> > No, because ours are hidden and thus not exported. System libraries can
> > *not* pick our symbols even if they aren't renamed. That is true for cairo,
> > and that is true for harfbuzz.
> 
> Okay, but then why was the linker warning otherwise until I did the renames?
> i.e.
> 
> 0:29.08 ../../build/unix/gold/ld: warning: hidden symbol 'hb_font_destroy'
> in
> /home/lee/central/obj-gtk3/toolkit/library/../../gfx/harfbuzz/src/
> Unified_cpp_gfx_harfbuzz_src0.o is referenced by DSO
> /usr/lib/gcc/x86_64-linux-gnu/4.9/../../../x86_64-linux-gnu/libpangoft2-1.0.
> so

I don't know why gold emits such a warning. bfd.ld doesn't. I'd consider it a bug in gold. Or it being overzealous.
 
> Still looking into the mochitest failures as some are indeed failing.

The build oranges are also different from elm.

Comment 21

[Lee Salzman [:lsalzman]]

(In reply to Mike Hommey [:glandium] from comment #20)
> (In reply to Lee Salzman from comment #19)
> > (In reply to Mike Hommey [:glandium] from comment #18)
> > > (In reply to Lee Salzman from comment #12)
> > > > (In reply to Mike Hommey [:glandium] from comment #10)
> > > > > Comment on attachment 8600208 [details] [diff] [review]
> > > > > Allow loading of tree Cairo and system Cairo at the same time
> > > > > 
> > > > > Review of attachment 8600208 [details] [diff] [review]:
> > > > > -----------------------------------------------------------------
> > > > > 
> > > > > > This is because our own tree Cairo, despite some renaming code, is bit-rotten and not properly isolating all symbols anymore.
> > > > > 
> > > > > Is that really all there is to it? I thought the problem was that we would
> > > > > pass cairo objects from the system cairo through gtk to the in-tree cairo,
> > > > > or the other way around, and /that/ wouldn't work. But maybe that changed,
> > > > > and we can, in fact, now build with in-tree cairo, I don't know.
> > > > 
> > > > That is the problem. But to properly ensure that they two Cairos are isolated
> > > > requires that symbols don't leak. Any symbols that we DON'T rename are
> > > > picked up
> > > > by external libraries linking against Cairo, and if we only rename symbols
> > > > piece-meal, then external libraries using external Cairo will use some of
> > > > their
> > > > symbols and some of ours. 
> > > 
> > > No, because ours are hidden and thus not exported. System libraries can
> > > *not* pick our symbols even if they aren't renamed. That is true for cairo,
> > > and that is true for harfbuzz.
> > 
> > Okay, but then why was the linker warning otherwise until I did the renames?
> > i.e.
> > 
> > 0:29.08 ../../build/unix/gold/ld: warning: hidden symbol 'hb_font_destroy'
> > in
> > /home/lee/central/obj-gtk3/toolkit/library/../../gfx/harfbuzz/src/
> > Unified_cpp_gfx_harfbuzz_src0.o is referenced by DSO
> > /usr/lib/gcc/x86_64-linux-gnu/4.9/../../../x86_64-linux-gnu/libpangoft2-1.0.
> > so
> 
> I don't know why gold emits such a warning. bfd.ld doesn't. I'd consider it
> a bug in gold. Or it being overzealous.
>

Found an explanation for the warnings: https://sourceware.org/bugzilla/show_bug.cgi?id=10471
It will complain loudly about the reference, but it will still refuse to link it in the end. So regardless of it being noisy, the behavior is safe for us in the end. So you are correct about it being overzealous.

> > Still looking into the mochitest failures as some are indeed failing.
> 
> The build oranges are also different from elm.

I accidentally confirmed that it was a result of failure to apply the patch from bug 1127752. When I tried running mochitests locally without that patch, everything was crashing as expected like in the bug report. When I applied that patch on top of these patches. everything was happy again and the mochitests passed.

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8600293 [details] [diff] [review]
Update build system to allow using in-tree Cairo with GTK3 enabled

Review of attachment 8600293 [details] [diff] [review]:
-----------------------------------------------------------------

From a build system perspective, this is fine, but it's still not clear to me this is actually putting us in a better situation than the current status-quo, cf. all the new crashes on https://treeherder.mozilla.org/#/jobs?repo=try&revision=6832f85f55b1 (like those happening during make check in the build jobs)

::: widget/gtk/moz.build
@@ +97,5 @@
>  DEFINES['MOZ_APP_NAME'] = '"%s"' % CONFIG['MOZ_APP_NAME']
>  
>  CFLAGS += CONFIG['MOZ_STARTUP_NOTIFICATION_CFLAGS']
> +
> +# When building with GTK3, we need to use system Cairo headers, so

"we always need to use system Cairo headers for the widget code, independently of whether we're using in-tree Cairo for graphics code" or something like that?

Comment 23

[Lee Salzman [:lsalzman]]

Attachment: Initialize g-type before gdk is used in components to avoid crashing in older glib versions (< 2.36) that don't automatically initialize g-type

So I diagnosed the cause of the crashes in make check (and also xpcshell tests which were what make check was crashing on). The cause was because of the bundled Glib version we're using (2.34). In 2.36 they made a change to automatically initialize g_type_init so that it doesn't need to be explicitly called anymore, which is why it was working fine locally. But in older versions when this is not initialized, and in calls like gdk_display_get_default, it's not so okay, hence crashes... This was showing up in xpcshell tests even in current repos before, but some other error in make check was hiding it from showing it up there till I applied these other patches.

The build and xpcshell tests now show up green on try with this patch combined with my other ones: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c90a75d0c511

Comment 24

[Lee Salzman [:lsalzman]]

Attachment: Allow loading of tree Cairo and system Cairo at the same time

Edit moz.build comment to be more descriptive

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8603841 [details] [diff] [review]
Initialize g-type before gdk is used in components to avoid crashing in older glib versions (< 2.36) that don't automatically initialize g-type

Review of attachment 8603841 [details] [diff] [review]:
-----------------------------------------------------------------

I'd rather go with a backout of bug 978147, and making the g_type_init calls conditional to !GLIB_CHECK_VERSION (2, 36, 0). Although... I'm not sure why there were g_type_init calls in the download code.

Comment 26

[Mike Hommey [:glandium]]

Err, this is all confusing... bug 978147 was backed out and the g_type_init calls are still there. So I guess this comes from assumptions of the gtk libraries themselves in gtk 3.x. So, I think that would be better in moz_gtk_init in gtk3drawing.c, if that's early enough.

Comment 27

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #26)
> Err, this is all confusing... bug 978147 was backed out and the g_type_init
> calls are still there. So I guess this comes from assumptions of the gtk
> libraries themselves in gtk 3.x. So, I think that would be better in
> moz_gtk_init in gtk3drawing.c, if that's early enough.

It's not early enough. Looking into this further, nsAppShell::Init would be a better place than NS_InitXPCOM2.

Comment 28

[Lee Salzman [:lsalzman]]

Attachment: Ensure g_type_init is explicitly called in nsAppShell::Init before other Glib/Gtk usage

I simplified that g_type_init call to be in nsAppShell::Init as requested, and I verified it works on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad6d63de3ffd

In that try build I also included the patch for gdk_display_close from bugzilla 884831. Sum total of all these patches seems we pass a lot more unit tests now.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1663d6ff4ae0
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c35d4c15798
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9ce69afe624
https://hg.mozilla.org/integration/mozilla-inbound/rev/d495587bc0d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/642ea5f65b81

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1663d6ff4ae0
https://hg.mozilla.org/mozilla-central/rev/0c35d4c15798
https://hg.mozilla.org/mozilla-central/rev/e9ce69afe624
https://hg.mozilla.org/mozilla-central/rev/d495587bc0d2
https://hg.mozilla.org/mozilla-central/rev/642ea5f65b81