605009 - Linux debug build with --disable-pango crashes during shutdown

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Kew [:jfkthame]]

Bug 600102 (re-)enabled desktop Linux builds with the --disable-pango flag, to use the FT2Fonts backend (like mobile). However, debug builds using this option crash during gfxPlatform shutdown because the gfxPlatformGtk destructor closes down the Freetype library (see http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatformGtk.cpp#149). Then the gfxPlatform destructor calls Cairo's cleanup function (see http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#353) which leads to a crash when cairo tries to deallocate the fonts, but their associated FTLibrary is already gone.

The gfxPangoFonts code avoids this by ensuring that the fonts are created using Cairo's FTLibrary, not a separately-initialized one; see http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPangoFonts.cpp#2138. But in the --disable-pango case, the platform initializes its own FTLibrary (see http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatformGtk.cpp#119), and then closes it down too early.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: patch, prevent shutdown crash by cleaning up cairo earlier

This fixes the (debug-only) crash by calling cairo's cleanup function earlier. This means we actually call it twice (from the gfxPlatformGtk destructor, and again from the gfxPlatform destructor), but that's harmless. This is the same solution other gfxPlatform subclasses like Android and Qt are using.

Comment 2

[Karl Tomlinson (:karlt)]

(In reply to comment #0)
> The gfxPangoFonts code avoids this by ensuring that the fonts are created using
> Cairo's FTLibrary, not a separately-initialized one; see
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPangoFonts.cpp#2138.
> But in the --disable-pango case, the platform initializes its own FTLibrary

and, without fontconfig, there is no way to create a font with cairo to extract cairo's FT_Library.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 495883 [details] [diff] [review]
patch, prevent shutdown crash by cleaning up cairo earlier

(In reply to comment #1)
> This is the same
> solution other gfxPlatform subclasses like Android and Qt are using.

except that Android and Qt call cairo_debug_reset_static_data() unconditionally.
I'd be happier with that, because, if cairo_debug_reset_static_data() is not called, the references to dead FT_Faces will still exist, and cairo may choose to destroy our font_faces at some other time.

Another option would be to keep a reference count for gPlatformFTLibrary, FT_Init_FreeType when first needed, and FT_Done_FreeType when the reference count drops to zero, instead of from gfxPlatformGtk.

Comment 4

[Karl Tomlinson (:karlt)]

Or, alternatively, include FT_Done_FreeType in the same conditional as cairo_debug_reset_static_data.

NS_FREE_PERMANENT_DATA is probably the appropriate preprocessor tag to test.

Comment 5

[Karl Tomlinson (:karlt)]

I think I like the conditional FT_Done_FreeType option best.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch v2 - prevent shutdown crash in debug builds

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 496185 [details] [diff] [review]
patch v2 - prevent shutdown crash in debug builds

Thanks.  This should really be conditional also on MOZ_TREE_CAIRO as you had before.  Sorry if my comment was misleading.  Qt and Android don't need to worry about GTK using cairo.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Any plans to land this?  Not a big deal, but makes working with fennec-desktop builds somewhat annoying.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 496185 [details] [diff] [review]
patch v2 - prevent shutdown crash in debug builds

This doesn't affect release builds, but makes debugging desktop builds with --disable-pango easier; requesting approval-2.0 to get this into the tree.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 496185 [details] [diff] [review]
patch v2 - prevent shutdown crash in debug builds

a=beltzner

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 496185 [details] [diff] [review]
patch v2 - prevent shutdown crash in debug builds

Didn't land in time for RC1 - unapproved.

Comment 13

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/1aa5fcc054ad

Comment 14

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/1aa5fcc054ad

Comment 15

[Karl Tomlinson (:karlt)]

Review comment wasn't addressed.