Move shutdown of GTK's fontmap from gfxPlatformGtk to nsAppRunner

RESOLVED FIXED in mozilla1.9.1b1

Status

()

Toolkit
Startup and Profile System
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

Trunk
mozilla1.9.1b1
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

9 years ago
GTK's fontmap should not be shutdown until after we have finished with GTK.
Thebes will use a different fontmap after bug 385263.
(Assignee)

Comment 1

9 years ago
Created attachment 338030 [details] [diff] [review]
patch [pushed]

This also removes our dependencies on pangocairo and Xft (which we haven't
used for some time).
Attachment #338030 - Flags: superreview?
Attachment #338030 - Flags: review?(roc)
(Assignee)

Updated

9 years ago
Attachment #338030 - Flags: superreview? → superreview?(benjamin)
Attachment #338030 - Flags: review?(roc) → review+
Comment on attachment 338030 [details] [diff] [review]
patch [pushed]

I really don't feel comfortable having this kind of cleanup code only run debug-or-memory-logging... what's the reason we can't just turn it on in general?
(Assignee)

Comment 3

9 years ago
(In reply to comment #2)
> what's the reason we can't just turn it on in general?

cairo_debug_reset_static_data() hits a fatal assertion if there are active
objects remaining.

http://www.cairographics.org/manual/cairo-Error-Handling.html#cairo-debug-reset-static-data

pango_fc_font_map_shutdown can cause a different fatal assertion to be hit in
cairo with some cairo/Pango versions if there are active Pango objects
remaining (bug 399556).

gtk loads different modules according to user/distro settings and so it is
hard or impossible for us to know which ones may leak cairo or Pango objects.

In debug builds this cleanup is useful because it asserts when there are
objects leaked.  In NS_TRACE_MALLOC builds it is useful for cleaning up
shutdown leaks.  I'm not so confident of the usefulness for
NS_BUILD_REFCNT_LOGGING-only builds.  Pango and cairo objects can have
shutdown functions that might release refcnted objects, but we've already shut
down XPCOM here.  However it would test that we'd removed our shutdown functions before/when shutting down XPCOM.

In release builds I see little advantage in having these enabled.
The one thing that they could achieve is crashes on shutdown and restart.

(I think closing the display is different, as I think perhaps the display
should be synchronously closed before calling exec and opening another
connection, but I still don't completely understand  Bug 246313.)

When Thebes uses its own fontmap (bug 385263), we should always shutdown the
fontmap as we have control over cairo and the fontmap and it is only
accessible through Thebes.  I'll change my patch in that bug accordingly.
Attachment #338030 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Comment 4

9 years ago
http://hg.mozilla.org/mozilla-central/rev/71004cbe3d8a
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Target Milestone: --- → mozilla1.9.1b1
This broke debug build on linux (fedora-7 based)
nsAppRunner.cpp
In file included from ../../dist/include/system_wrappers/ft2build.h:3,
                 from /usr/include/pango-1.0/pango/pangofc-font.h:25,
                 from ../../dist/include/system_wrappers/pango/pangofc-font.h:3,
                 from /usr/include/pango-1.0/pango/pangofc-decoder.h:25,
                 from ../../dist/include/system_wrappers/pango/pangofc-decoder.h:3,
                 from /usr/include/pango-1.0/pango/pangofc-fontmap.h:27,
                 from ../../dist/include/system_wrappers/pango/pangofc-fontmap.h:3,
                 from /home/smaug/mozilla/mozilla_cvs/hg/mozilla/toolkit/xre/nsAppRunner.cpp:280:
/usr/include/ft2build.h:56:38: error: freetype/config/ftheader.h: No such file or directory
In file included from ../../dist/include/system_wrappers/pango/pangofc-font.h:3,
                 from /usr/include/pango-1.0/pango/pangofc-decoder.h:25,
                 from ../../dist/include/system_wrappers/pango/pangofc-decoder.h:3,
                 from /usr/include/pango-1.0/pango/pangofc-fontmap.h:27,
                 from ../../dist/include/system_wrappers/pango/pangofc-fontmap.h:3,
                 from /home/smaug/mozilla/mozilla_cvs/hg/mozilla/toolkit/xre/nsAppRunner.cpp:280:
/usr/include/pango-1.0/pango/pangofc-font.h:26:10: error: #include expects "FILENAME" or <FILENAME>
/usr/include/pango-1.0/pango/pangofc-font.h:112: error: expected identifier before ‘*’ token
/usr/include/pango-1.0/pango/pangofc-font.h:112: error: ‘FT_Face’ declared as function returning a function
/usr/include/pango-1.0/pango/pangofc-font.h:147: error: ‘FT_Face’ does not name a type
(Assignee)

Comment 6

9 years ago
Created attachment 340539 [details] [diff] [review]
add MOZ_PANGO_CFLAGS [pushed]

to address comment 5.
Attachment #340539 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #340539 - Flags: review? → review?(benjamin)
Comment on attachment 340539 [details] [diff] [review]
add MOZ_PANGO_CFLAGS [pushed]

This works for me
Attachment #340539 - Flags: review?(benjamin) → review+
(Assignee)

Updated

9 years ago
Attachment #338030 - Attachment description: patch → patch [pushed]
(Assignee)

Updated

9 years ago
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Whiteboard: [attachment 340539 needs checkin]
(Assignee)

Updated

9 years ago
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [attachment 340539 needs checkin]
(Assignee)

Comment 8

9 years ago
Comment on attachment 340539 [details] [diff] [review]
add MOZ_PANGO_CFLAGS [pushed]

http://hg.mozilla.org/mozilla-central/rev/03e2e5e8dad7
Attachment #340539 - Attachment description: add MOZ_PANGO_CFLAGS → add MOZ_PANGO_CFLAGS [pushed]
You need to log in before you can comment on or make changes to this bug.