Closed Bug 1159273 Opened 5 years ago Closed 5 years ago

GTK3 toolkit build requires system Cairo

Categories

(Core :: Graphics, defect, major)

All
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: lsalzman, Assigned: lsalzman)

References

Details

Attachments

(5 files, 7 obsolete files)

10.33 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.09 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
5.59 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
3.00 KB, patch
glandium
: review+
Details | Diff | Splinter Review
895 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
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).
Attachment #8598688 - Flags: review?(jmuizelaar)
Blocks: 1127752
fix missing system-cairo.h file
Attachment #8598688 - Attachment is obsolete: true
Attachment #8598688 - Flags: review?(jmuizelaar)
Attachment #8598698 - Flags: review?(jmuizelaar)
simplified the idea to preserve header locations
Attachment #8598698 - Attachment is obsolete: true
Attachment #8598698 - Flags: review?(jmuizelaar)
Attachment #8599414 - Flags: review?(jmuizelaar)
Fix symbol visibility issue for Cairo headers
Attachment #8599414 - Attachment is obsolete: true
Attachment #8599414 - Flags: review?(jmuizelaar)
Attachment #8599580 - Flags: review?(jmuizelaar)
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.
Attachment #8599580 - Flags: review?(jmuizelaar)
Attachment #8599580 - Flags: review?(jfkthame)
Attachment #8599580 - Flags: review+
Attachment #8599580 - Flags: review?(mh+mozilla)
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!
(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?
(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?
(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.
I separated out all harfbuzz changes from this patch so only the existing non-controversial Cairo changes remain
Attachment #8599580 - Attachment is obsolete: true
Attachment #8599580 - Flags: review?(mh+mozilla)
Attachment #8599580 - Flags: review?(jfkthame)
Attachment #8600208 - Flags: review?(mh+mozilla)
Attachment #8600208 - Flags: review?(jfkthame)
Blocks: 1160420
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.
Attachment #8600208 - Flags: review?(mh+mozilla)
Attachment #8600208 - Flags: review?(jfkthame)
(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.
Attachment #8600208 - Attachment is obsolete: true
Attachment #8600293 - Flags: review?(mh+mozilla)
(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.
(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.
(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.
(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.
(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.
Attachment #8600298 - Flags: review?(jmuizelaar) → review+
Attachment #8600297 - Flags: review?(jmuizelaar) → review+
Attachment #8600295 - Flags: review?(jmuizelaar) → review+
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?
Attachment #8600293 - Flags: review?(mh+mozilla) → feedback+
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
Attachment #8603841 - Flags: review?(mh+mozilla)
Edit moz.build comment to be more descriptive
Attachment #8603843 - Flags: review?(mh+mozilla)
Attachment #8600293 - Attachment is obsolete: true
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.
Attachment #8603841 - Flags: review?(mh+mozilla) → review-
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.
Attachment #8603843 - Flags: review?(mh+mozilla) → review+
(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.
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.
Attachment #8603841 - Attachment is obsolete: true
Attachment #8604656 - Flags: review?(mh+mozilla)
Attachment #8604656 - Flags: review?(mh+mozilla) → review+
You need to log in before you can comment on or make changes to this bug.