Last Comment Bug 458169 - [@font-face] implement downloadable font support on Linux
: [@font-face] implement downloadable font support on Linux
Status: RESOLVED FIXED
: fixed1.9.1
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Linux
: P2 major with 8 votes (vote)
: mozilla1.9.1b3
Assigned To: Karl Tomlinson (:karlt)
:
:
Mentors:
Depends on: 449356 467874 480098
Blocks: 70132 acid3
  Show dependency treegraph
 
Reported: 2008-10-02 00:20 PDT by John Daggett (:jtd)
Modified: 2010-03-05 08:57 PST (History)
38 users (show)
vladimir: blocking1.9.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proof of concept for src:url() (56.22 KB, patch)
2008-11-17 17:16 PST, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review
src:url() v1.0 (80.94 KB, patch)
2008-11-23 21:47 PST, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review
src:url() v1.1 (81.79 KB, patch)
2008-11-28 15:42 PST, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review
src:url() v1.1 - use cairo's FT_Library [pushed to m-c and 1.9.1] (81.04 KB, patch)
2008-12-03 03:04 PST, Karl Tomlinson (:karlt)
roc: review+
Details | Diff | Splinter Review

Description John Daggett (:jtd) 2008-10-02 00:20:55 PDT
As part of the work for bug 441473, support for downloable fonts was implemented on Mac and Windows.  For Linux, there's additional work required in our Pango font code to hook into this mechanism, this is being done under bug 449356.

Once that bug is complete, the work for this includes:

* within font selection code, lookup fonts within the user font object when appropriate
* load/unload fonts using downloaded data [src: url()]
* lookup specific font faces using the full name of a given face [src: local()]
Comment 1 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-11-08 15:16:07 PST
When this lands, you'll need to adjust the fails-if and skip-if markers in http://hg.mozilla.org/mozilla-central/file/tip/layout/reftests/font-face/reftest.list
Comment 2 Karl Tomlinson (:karlt) 2008-11-17 17:16:00 PST
Created attachment 348692 [details] [diff] [review]
proof of concept for src:url()

This provides src:url() web fonts.  It hasn't had much testing but works on
http://www.w3.org/International/tests/test-webfonts/test-georgian-ucnobi.html

Currently FcFreeTypeQueryFace is used and so fontconfig-2.4.2 or newer is
required.  I'll need to add support for older versions.  (Our CentOS 5.0 build
and test machines have fontconfig-2.4.1.)

To avoid temporary files (sometimes?) being created by nsIDownloader in system
directories and not-necessarily cleaned up, fonts are loaded into memory
through nsIStreamLoader in order to access them.

This misses an opportunity for sharing glyph caches etc across pages, but that
can be something that is sorted out if/when we have a downloaded-font cache.

A reference to the nsIStreamLoader is held to keep the font data in memory
until the font entry is destroyed.

This makes possible future optimization of nsIStreamLoader (or a new loader
type) such that, when appropriate, the font data is pinned to the disk cache
and the logical address of the data is provided through mmap rather than a
memory copy of the file data.  (This I think would benefit all platforms, even
those on which platform libraries make their own copy of all the data.)
Comment 3 Karl Tomlinson (:karlt) 2008-11-23 21:47:11 PST
Created attachment 349707 [details] [diff] [review]
src:url() v1.0

This version works even with older versions of fontconfig.
A build is available here:

https://build.mozilla.org/tryserver-builds/2008-11-23_20:06-ktomlinson@mozilla.com-try-afd5aedcbcf/

Note that this doesn't support src:local(), which I intend to do using a
different implementation of gfxFcFontEntry.

(In reply to comment #2)
> A reference to the nsIStreamLoader is held to keep the font data in memory
> until the font entry is destroyed.

The nsIStreamLoader* arg could be an nsISupports*, I assume.
Let me know if you prefer that.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-11-24 00:57:56 PST
Basically looks great to me.

+     * Ownership of the returned gfxFontEntry is passed to the caller,
+     * who must either AddRef() or delete.

Should these functions be changed (in a separate patch) to return already_AddRefed?

+    // Helper function to be called from InitPattern() to change the pattern
+    // so that it matches the CSS style descriptors and so gets properly
+    // sorted in font selection.  This also avoids synthetic style effects
+    // being added by the renderer when the style of the font itself does not
+    // match the descriptor provided by the author.
+    void ConformPattern();

I'd like a better name here. Perhaps "AdjustPatternToCSS"?

+ * gfxWebFcFontEntry:

I think gfxDownloadedFcFontEntry would be a better name.

+    FcPatternGetInteger(mPattern, FC_WEIGHT, 0, &fontWeight);
+    int cssWeight = gfxFontconfigUtils::FcWeightForBaseWeight(mWeight);
+    if (cssWeight != fontWeight) {
+        FcPatternDel(mPattern, FC_WEIGHT);
+        FcPatternAddInteger(mPattern, FC_WEIGHT, cssWeight);
+    }

Is there a reason not to do Del/Add unconditionally here? Ditto for FC_SLANT and setting FC_FULLNAME.

+        PRUint8 savedStyle = aStyle.style;
+        aStyle.style = FONT_STYLE_NORMAL;
+        fontEntry = static_cast<gfxFcFontEntry*>
+            (mUserFontSet->FindFontEntry(utf16Family, aStyle, needsBold));
+        aStyle.style = savedStyle;

This is yuck. Can we make aStyle a const reference and just use a temporary copy here?

+            // User fonts are already filtered by slant (but not size) in
+            // mUserFontSet->FindFontEntry().

Aren't you working around that by retrying FindFontEntry with FONT_STYLE_NORMAL, in FindFontPattern?

+    if (!(list->Contains(fontName))) {

Lose unnecessary parens.

+PRBool
+gfxPlatformGtk::IsFontFormatSupported(nsIURI *aFontURI, PRUint32 aFormatFlags)
+{
+    // reject based on format flags
+    if (aFormatFlags & (gfxUserFontSet::FLAG_FORMAT_EOT | gfxUserFontSet::FLAG_FORMAT_SVG)) {
+        return PR_FALSE;
+    }

Can we avoid blacklisting and write this code to just return true for the formats we know we can support?

jdaggett, do you want to review this too?
Comment 5 Karl Tomlinson (:karlt) 2008-11-24 13:35:53 PST
(In reply to comment #4)
> +     * Ownership of the returned gfxFontEntry is passed to the caller,
> +     * who must either AddRef() or delete.
> 
> Should these functions be changed (in a separate patch) to return
> already_AddRefed?

I think so.  I find it confusing to pass around references to objects that
have a reference count of zero.

> 
> +    // Helper function to be called from InitPattern() to change the pattern
> +    // so that it matches the CSS style descriptors and so gets properly
> +    // sorted in font selection.  This also avoids synthetic style effects
> +    // being added by the renderer when the style of the font itself does not
> +    // match the descriptor provided by the author.
> +    void ConformPattern();
> 
> I'd like a better name here. Perhaps "AdjustPatternToCSS"?

That sounds fine.
 
> + * gfxWebFcFontEntry:
> 
> I think gfxDownloadedFcFontEntry would be a better name.

gfxDownloadedFcFontEntry has the advantage of indicating that the font has
already been downloaded.  And I guess src:local() faces would really be web
fonts too (but not downloaded fonts) because the family and style properties are obtained from the web (even if the font data is not).

> 
> +    FcPatternGetInteger(mPattern, FC_WEIGHT, 0, &fontWeight);
> +    int cssWeight = gfxFontconfigUtils::FcWeightForBaseWeight(mWeight);
> +    if (cssWeight != fontWeight) {
> +        FcPatternDel(mPattern, FC_WEIGHT);
> +        FcPatternAddInteger(mPattern, FC_WEIGHT, cssWeight);
> +    }
> 
> Is there a reason not to do Del/Add unconditionally here? Ditto for FC_SLANT
> and setting FC_FULLNAME.

I'd like to leave these conditional.

For FC_WEIGHT, the only reason is to avoid the reallocation of memory for the
property value and the memmove back and forth of all the trailing properties
and pointers to their values.  The weight is expected to often (maybe usually)
already have the right value.

For FC_SLANT, there is the additional benefit of retaining the distinction
between italic and oblique where possible.

For FC_FULLNAME, if there is an existing value, then that is the best value as
it comes from the OpenType name table.  Appending style to family should only
be a fallback.

> +        PRUint8 savedStyle = aStyle.style;
> +        aStyle.style = FONT_STYLE_NORMAL;
> +        fontEntry = static_cast<gfxFcFontEntry*>
> +            (mUserFontSet->FindFontEntry(utf16Family, aStyle, needsBold));
> +        aStyle.style = savedStyle;
> 
> This is yuck. Can we make aStyle a const reference and just use a temporary
> copy here?

Yes, this is yuck.

Constructing a gfxFontStyle always requires a memory allocation because it has
an nsCString member, which is always forced to be non-empty, even though it
doesn't get used here.

What I think would look nicest here would be to change the FindFontPattern()
gfxFontStyle argument to thebes style and weight arguments.  That would avoid
the new gfxFontStyle allocation in SortPreferredFonts, and confine all the
gfxFontStyle yuck to within FindFontPattern.

Then modifying gfxUserFontSet::FindFontEntry arguments so that only the
information actually used needs to be provided, and/or modifying gfxFontStyle
so that the nsCString member can be empty, can be considered as future
improvements.

> 
> +            // User fonts are already filtered by slant (but not size) in
> +            // mUserFontSet->FindFontEntry().
> 
> Aren't you working around that by retrying FindFontEntry with
> FONT_STYLE_NORMAL, in FindFontPattern?

SlantIsAcceptable() also accepts faces with FONT_STYLE_NORMAL/FC_SLANT_ROMAN
when the requested style is not normal/roman (as an oblique can be synthesized
from normal/roman).

> +PRBool
> +gfxPlatformGtk::IsFontFormatSupported(nsIURI *aFontURI, PRUint32 aFormatFlags)
> +{
> +    // reject based on format flags
> +    if (aFormatFlags & (gfxUserFontSet::FLAG_FORMAT_EOT |
> gfxUserFontSet::FLAG_FORMAT_SVG)) {
> +        return PR_FALSE;
> +    }
> 
> Can we avoid blacklisting and write this code to just return true for the
> formats we know we can support?

Maybe.

The editor's draft http://dev.w3.org/csswg/css3-fonts/#font-reference and the
2002 working draft http://www.w3.org/TR/css3-webfonts/#src currently suggests
returning true only for formats we know we can support: "The user agent will
recognize the name of font formats that it supports, and will avoid
downloading fonts in formats that it does not recognize."

This code was copied from the code for Mac and Windows, so I suggest
considering making that change to all platforms in a separate bug, probably
bug 465452.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-11-24 13:40:16 PST
(In reply to comment #5)
> > Should these functions be changed (in a separate patch) to return
> > already_AddRefed?
> 
> I think so.  I find it confusing to pass around references to objects that
> have a reference count of zero.

Please file a bug on that.

> I'd like to leave these conditional.
> 
> For FC_WEIGHT, the only reason is to avoid the reallocation of memory for the
> property value and the memmove back and forth of all the trailing properties
> and pointers to their values.  The weight is expected to often (maybe usually)
> already have the right value.
> 
> For FC_SLANT, there is the additional benefit of retaining the distinction
> between italic and oblique where possible.
> 
> For FC_FULLNAME, if there is an existing value, then that is the best value as
> it comes from the OpenType name table.  Appending style to family should only
> be a fallback.

OK.

> > +        PRUint8 savedStyle = aStyle.style;
> > +        aStyle.style = FONT_STYLE_NORMAL;
> > +        fontEntry = static_cast<gfxFcFontEntry*>
> > +            (mUserFontSet->FindFontEntry(utf16Family, aStyle, needsBold));
> > +        aStyle.style = savedStyle;
> > 
> > This is yuck. Can we make aStyle a const reference and just use a temporary
> > copy here?
> 
> Yes, this is yuck.
> 
> Constructing a gfxFontStyle always requires a memory allocation because it has
> an nsCString member, which is always forced to be non-empty, even though it
> doesn't get used here.
> 
> What I think would look nicest here would be to change the FindFontPattern()
> gfxFontStyle argument to thebes style and weight arguments.  That would avoid
> the new gfxFontStyle allocation in SortPreferredFonts, and confine all the
> gfxFontStyle yuck to within FindFontPattern.
> 
> Then modifying gfxUserFontSet::FindFontEntry arguments so that only the
> information actually used needs to be provided, and/or modifying gfxFontStyle
> so that the nsCString member can be empty, can be considered as future
> improvements.

OK

> > +            // User fonts are already filtered by slant (but not size) in
> > +            // mUserFontSet->FindFontEntry().
> > 
> > Aren't you working around that by retrying FindFontEntry with
> > FONT_STYLE_NORMAL, in FindFontPattern?
> 
> SlantIsAcceptable() also accepts faces with FONT_STYLE_NORMAL/FC_SLANT_ROMAN
> when the requested style is not normal/roman (as an oblique can be synthesized
> from normal/roman).

OK

> This code was copied from the code for Mac and Windows, so I suggest
> considering making that change to all platforms in a separate bug, probably
> bug 465452.

OK
Comment 7 John Daggett (:jtd) 2008-11-24 16:53:43 PST
(In reply to comment #3)
> > A reference to the nsIStreamLoader is held to keep the font data in memory
> > until the font entry is destroyed.
> 
> The nsIStreamLoader* arg could be an nsISupports*, I assume.
> Let me know if you prefer that.

I think that seems better, it would eliminate the need for gfx to link against necko, right?
Comment 8 Karl Tomlinson (:karlt) 2008-11-24 17:18:31 PST
(In reply to comment #7)
> I think that seems better, it would eliminate the need for gfx to link against
> necko, right?

Thanks to XPCOM (or virtual functions) gfx doesn't need to link against necko.

It would just save including nsIStreamLoader.h in gfxPangoFonts.h and adding necko to the include path for gfx.
Comment 9 Karl Tomlinson (:karlt) 2008-11-24 17:19:07 PST
(In reply to comment #8)
> It would just save including nsIStreamLoader.h in gfxPangoFonts.h and

Sorry, in gfxPangoFonts.cpp.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-11-26 11:44:57 PST
There's a tiny bit of updating needed after bug 457821 landed:
 * you'll want to adjust the reftest.list for additional reftests that landed
 * you'll want to add the assertion to your UpdateFontList
Comment 11 Bill Gianopoulos [:WG9s] 2008-11-28 07:27:03 PST
(In reply to comment #3)
> Created an attachment (id=349707) [details]

With this patch installed, There is a one row remnant of the magenta background visible in the upper right corner of the Acid3 test.

Additionally, this testcase from bug 457194,
https://bugzilla.mozilla.org/attachment.cgi?id=340544, fails.  If I install the Ahem font on my system and modify the test to
use the installed font rather than @font-face, the test passes.
Comment 12 Bill Gianopoulos [:WG9s] 2008-11-28 09:57:49 PST
(In reply to comment #11)

> Additionally, this testcase from bug 457194,
> https://bugzilla.mozilla.org/attachment.cgi?id=340544, fails.  If I install the
> Ahem font on my system and modify the test to
> use the installed font rather than @font-face, the test passes.

Oddly, in investigating this issue I found that this testcase fails under Windows as well, even though Windows does not show the issue under Acid3.  I filed bug 467084 on that issue.
Comment 13 Bill Gianopoulos [:WG9s] 2008-11-28 12:12:10 PST
It appears that the issue in bug 467084 is likely a regression from bug 457821.  So, that could very well be the cause of the issue here as well.
Comment 14 Karl Tomlinson (:karlt) 2008-11-28 13:32:34 PST
(In reply to comment #11)
> > Created an attachment (id=349707)
> 
> With this patch installed, There is a one row remnant of the magenta background
> visible in the upper right corner of the Acid3 test.

Yes.  I see that too, but that wasn't there when I last checked (when I think the patch was against 26f6c6c90a43 - Nov 12/13).
Comment 15 Karl Tomlinson (:karlt) 2008-11-28 15:42:58 PST
Created attachment 350535 [details] [diff] [review]
src:url() v1.1

* Addressed review comments.

* Changed nsIStreamLoader* arg to nsISupports* as the font entry doesn't care
what type of loader it is.

* Lazy FT_Init_FreeType to avoid a Ts regression of 10-20 ms.
(It looks like there is a Tp gain of 0.5-1% probably due to keeping a
reference to the PangoCoverage on the PangoFont.)

There is still an occasional shutdown crash that needs to be addressed.  This
happens due to cairo caching its font structures that hold a reference to the
FT_Face.  During _cairo_font_face_reset_static_data these font
structures are destroyed, during which our callback calls FT_Done_Face, but
FT_Done_FreeType has already been called from ~gfxPlatformGtk.
(FT_Done_FreeType destroys any faces not already Done).

I assume the same problem exists with the gfxFT2Fonts backend.

#4  <signal handler called>
#5  0x00007f42d4d791ad in FT_Done_Face () from /usr/lib64/libfreetype.so.6
#6  0x00007f42cc679889 in ~gfxDownloadedFcFontEntry (this=0x38eba70)
    at /home/karl/moz/dev/gfx/thebes/src/gfxPangoFonts.cpp:345
#7  0x00007f42cc66177e in gfxFontEntry::Release (this=0x38eba70)
    at ../../../dist/include/thebes/gfxFont.h:145
#8  0x00007f42cc672f69 in ReleaseDownloadedFontEntry (data=0x38eba70)
    at /home/karl/moz/dev/gfx/thebes/src/gfxPangoFonts.cpp:2669
#9  0x00007f42cc6891b4 in _cairo_user_data_array_fini (array=0x3a1a0a0)
    at /home/karl/moz/dev/gfx/cairo/cairo/src/cairo-array.c:392
#10 0x00007f42cc68b202 in *INT__moz_cairo_font_face_destroy (
    font_face=0x3a1a090)
    at /home/karl/moz/dev/gfx/cairo/cairo/src/cairo-font-face.c:206
#11 0x00007f42cc6cec11 in _cairo_ft_unscaled_font_destroy (
    abstract_font=0x3a9b1b0)
    at /home/karl/moz/dev/gfx/cairo/cairo/src/cairo-ft-font.c:519
#12 0x00007f42cc68bba6 in _cairo_unscaled_font_destroy (
    unscaled_font=0x3a9b1b0)
    at /home/karl/moz/dev/gfx/cairo/cairo/src/cairo-font-face.c:759
#13 0x00007f42cc6d14e9 in _cairo_ft_scaled_font_fini (abstract_font=0x3a15860)
    at /home/karl/moz/dev/gfx/cairo/cairo/src/cairo-ft-font.c:1739
#14 0x00007f42cc6a17b2 in _cairo_scaled_font_fini_internal (
    scaled_font=0x3a15860)
    at /home/karl/moz/dev/gfx/cairo/cairo/src/cairo-scaled-font.c:733
#15 0x00007f42cc6a17d6 in _cairo_scaled_font_fini (scaled_font=0x3a15860)
    at /home/karl/moz/dev/gfx/cairo/cairo/src/cairo-scaled-font.c:744
#16 0x00007f42cc6a0cc7 in _cairo_scaled_font_map_destroy ()
    at /home/karl/moz/dev/gfx/cairo/cairo/src/cairo-scaled-font.c:387
#17 0x00007f42cc68bbba in _cairo_font_face_reset_static_data ()
    at /home/karl/moz/dev/gfx/cairo/cairo/src/cairo-font-face.c:767
#18 0x00007f42cc68b075 in _moz_cairo_debug_reset_static_data ()
    at /home/karl/moz/dev/gfx/cairo/cairo/src/cairo-debug.c:64
#19 0x00007f42cc66901f in ~gfxPlatform (this=0x28269d0)
    at /home/karl/moz/dev/gfx/thebes/src/gfxPlatform.cpp:262
#20 0x00007f42cc6808fc in ~gfxPlatformGtk (this=0x28269d0)
    at /home/karl/moz/dev/gfx/thebes/src/gfxPlatformGtk.cpp:148
#21 0x00007f42cc6691da in gfxPlatform::Shutdown ()
    at /home/karl/moz/dev/gfx/thebes/src/gfxPlatform.cpp:249
#22 0x00007f42c8d5b5c2 in nsThebesGfxModuleDtor (self=0x2826970)
    at /home/karl/moz/dev/gfx/src/thebes/nsThebesGfxFactory.cpp:150
#23 0x00007f42d8dd5e2b in nsGenericModule::Shutdown (this=0x2826970)
    at nsGenericFactory.cpp:340
#24 0x00007f42d8dd5e55 in ~nsGenericModule (this=0x2826970)
    at nsGenericFactory.cpp:237
#25 0x00007f42d8dd5fe7 in nsGenericModule::Release (this=0x2826970)
    at nsGenericFactory.cpp:245
#26 0x00007f42d8e3c8bf in nsCOMPtr<nsIModule>::assign_assuming_AddRef (
    this=0x28755d0, newPtr=0x0) at ../../dist/include/xpcom/nsCOMPtr.h:495
#27 0x00007f42d8e3c954 in nsCOMPtr<nsIModule>::assign_with_AddRef (
    this=0x28755d0, rawPtr=0x0) at ../../dist/include/xpcom/nsCOMPtr.h:1171
#28 0x00007f42d8e3f4d1 in nsCOMPtr<nsIModule>::operator= (this=0x28755d0,
    rhs=0x0) at ../../dist/include/xpcom/nsCOMPtr.h:640
#29 0x00007f42d8e3eb0a in nsNativeModuleLoader::ReleaserFunc (
    aHashedFile=0x2824528, aLoadData=@0x28755d0)
    at /home/karl/moz/dev/xpcom/components/nsNativeComponentLoader.cpp:219
#30 0x00007f42d8e3f320 in nsBaseHashtable<nsHashableHashKey, nsNativeModuleLoader::NativeLoadData, nsNativeModuleLoader::NativeLoadData>::s_EnumStub (
    table=0x239c5f0, hdr=0x28755c0, number=4, arg=0x7fffe21b6300)
    at ../../dist/include/xpcom/nsBaseHashtable.h:346
#31 0x00007f42d8dc4961 in PL_DHashTableEnumerate (table=0x239c5f0,
    etor=0x7f42d8e3f2c8 <nsBaseHashtable<nsHashableHashKey, nsNativeModuleLoader::NativeLoadData, nsNativeModuleLoader::NativeLoadData>::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*)>, arg=0x7fffe21b6300)
    at pldhash.c:735
#32 0x00007f42d8e3f3a4 in nsBaseHashtable<nsHashableHashKey, nsNativeModuleLoader::NativeLoadData, nsNativeModuleLoader::NativeLoadData>::Enumerate (
    this=0x239c5f0,
    enumFunc=0x7f42d8e3eae8 <nsNativeModuleLoader::ReleaserFunc(nsIHashable*, nsNativeModuleLoader::NativeLoadData&, void*)>, userArg=0x0)
    at ../../dist/include/xpcom/nsBaseHashtable.h:221
#33 0x00007f42d8e3eacd in nsNativeModuleLoader::UnloadLibraries (
    this=0x239c5e8)
    at /home/karl/moz/dev/xpcom/components/nsNativeComponentLoader.cpp:258
#34 0x00007f42d8e38484 in nsComponentManagerImpl::Shutdown (this=0x239c530)
    at /home/karl/moz/dev/xpcom/components/nsComponentManager.cpp:747
#35 0x00007f42d8ddac3c in NS_ShutdownXPCOM_P (servMgr=0x0)
    at /home/karl/moz/dev/xpcom/build/nsXPComInit.cpp:843

Possible solutions are

1) Check whether FT_Done_FreeType has been called before calling FT_Done_Face.
   This may be fairly simple, but its ugly leaving the reference to the
   FT_Face in cairo's caches.

2) Be more careful about shutdown order.  Some things will need to be shutdown
   before _cairo_font_face_reset_static_data and some after.

3) Use a refcounted object count references to the FT_Library (whenever an
   FT_Face is created or destroyed) and call FT_Done_FreeType when these reach
   zero.
Comment 16 Karl Tomlinson (:karlt) 2008-11-29 23:39:48 PST
>(In reply to comment #15)
> 2) Be more careful about shutdown order.  Some things will need to be shutdown
>    before _cairo_font_face_reset_static_data and some after.

This isn't a practical option with system-cairo.  cairo's cached font structures can be released when other fonts are added through other users of the library after gfxPlatform has shut down.

Another options:

4) Find out the FT_Library that cairo is using from the FT_Face from
   a cairo_ft_scaled_font_lock_face, and use that instead of using a
   separate FT_Library.

   There is some appeal in using the same FT_Library as cairo rather than
   having two identical FT_Librarys.  This approach would be making the
   assumption that cairo won't "Done" its FT_Library until
   cairo_debug_reset_static_data, but I can't think of a good reason why
   cairo might change its behavior to do this.

All these approaches except (4) are assuming that our callback function pointer on the cairo font face will point to a function in a library that is still in memory.

I was surprised to see that NS_ShutdownXPCOM_P() [including nsNativeModuleLoader::UnloadLibraries()] doesn't actually seem to be unloading XPCOM module libraries.  

If thebes (or necko) may be unloaded from memory, some other options are:

5) Force cairo to empty its holdover fonts (unreferenced fonts).  This would
   require creation and destruction of 256 cairo_scaled_fonts.

6) Modifying cairo to not holdover unreferenced fonts with external
   destroy_hooks.
Comment 17 Karl Tomlinson (:karlt) 2008-11-29 23:51:57 PST
(In reply to comment #16)
> All these approaches except (4) are assuming that our callback function pointer
> on the cairo font face will point to a function in a library that is still in
> memory.

Please ignore the "except (4)".  At the time I wrote that, (4) was something else.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-11-30 12:58:06 PST
It seems like option 6 is the way to go. Or perhaps

7) add a cairo API to flush unreferenced fonts (or unreferenced fonts with external destroy_hooks)

Basically, it's an API bug for cairo to require external destroy_hooks to work indefinitely far in the future.
Comment 19 Karl Tomlinson (:karlt) 2008-12-01 18:04:38 PST
(In reply to comment #6)
> (In reply to comment #5)
> > > Should these functions be changed (in a separate patch) to return
> > > already_AddRefed?
> > 
> > I think so.  I find it confusing to pass around references to objects that
> > have a reference count of zero.
> 
> Please file a bug on that.

Bug 467465
Comment 20 Karl Tomlinson (:karlt) 2008-12-02 19:56:52 PST
(In reply to comment #16)
> 6) Modifying cairo to not holdover unreferenced fonts with external
>    destroy_hooks.

(In reply to comment #18)
> It seems like option 6 is the way to go. Or perhaps
> 
> 7) add a cairo API to flush unreferenced fonts (or unreferenced fonts with
> external destroy_hooks)
> 
> Basically, it's an API bug for cairo to require external destroy_hooks to work
> indefinitely far in the future.

I think there are also performance reasons to only holdover fonts that are likely to be used again, and if these fonts have no external references they are not likely to be used again.  So cairo won't need an API to flush unreferenced fonts, if the holdover mechanism takes this into consideration.

I think modifying cairo's holdover logic is probably the best solution in the long term, but will require quite a reshuffle of data structures.  I've filed

https://bugs.freedesktop.org/show_bug.cgi?id=18857

(In reply to comment #16)
> 4) Find out the FT_Library that cairo is using from the FT_Face from
>    a cairo_ft_scaled_font_lock_face, and use that instead of using a
>    separate FT_Library.
> 
>    There is some appeal in using the same FT_Library as cairo rather than
>    having two identical FT_Librarys.  This approach would be making the
>    assumption that cairo won't "Done" its FT_Library until
>    cairo_debug_reset_static_data, but I can't think of a good reason why
>    cairo might change its behavior to do this.

I think this is going to be the best solution in the short term, as we currently don't need to concern ourselves with libraries being unloaded.

Behdad suggested this same solution as I was about to mention it to him, and said that the assumption here is safe.

Selected discussion on this from #cairo, with some edits to group threads more closely:

<karlt> i using cairo-ft-font for fonts created from an FT_Face
        for font data in memory
<karlt> this means registering a destroy_func on the font_face
<karlt> the bit where this gets inconvenient is that the destroy_func
        may be called at any time
<karlt> due to the holdovers in the scaled font map
<karlt> this is particularly inconvenient for users of cairo-ft-fonts
        that are dynamic libraries (or modules) that want to be unloaded
<karlt> such libraries would need to keep a reference to themselves, and (often)
        call dlclose on themselves when the last cairo_font_face is destroyed
<karlt> calling dlclose on oneself either involves relying on tail-call optimization,
<karlt> or making dlclose a destroy func on the last cairo_font_face
        (during execution of its first destroy_func)

<Company> can't you put the destory func into something that i not a module?

<otaylor> karlt: I think you just have to choose a) make a copy of the data
          b) don't unload your module

<karlt> Company: that's what making dlclose a destroy func on the last
        cairo_font_face would be doing

<Company> kinda, yeah
<Company> if you assume the dlclose really is called as the later one of the
          destroy funcs

<karlt> Company: yeah, it would be added only after the first destroy func has
        already been called

<Company> can you add destroy funcs from destroy funcs?

<karlt> Company: i haven't checked

<Company> i'd guess you can't, but not sure
<Company> unloading libraries is a PITA anyway, because it invites lots of bugs

<karlt> otaylor: i'm not seeing how to release the copy for (a)

<otaylor> karlt: might be tricky, you could have a tiny library that stays resident

<karlt> otaylor: possible
<karlt> but i'm wondering whether this complexity is necessary:
<karlt> the holdover scaled fonts are only useful if they might be picked up again
<karlt> and if there are no other references to the font_face then they won't
        be referenced again
<karlt> s/font_face/FT_Face/ maybe

<karlt> i guess i'm imagining a possible implementation something like where the
        unscaled_font (or maybe font_face) decides how many scaled_fonts to holdover
<karlt> that way the unscaled font can release scaled_fonts that won't be needed

<karlt> on a related note, the user currently has to keep a reference count to
        it's FT_Library so that it doesn't call FT_Done_FreeType before cairo
        has called the destroy_func on the last FT_Face
<karlt> is it worth considering added an api to cairo-ft-fonts so that the
        user can use cairo's FT_Library?
<karlt> and does anyone know if there is significant memory associated with
        have extra FT_Librarys in existence

<behdad> messy stuff
<behdad> karlt: actually
<behdad> I don't know how come no one added ref counting to freetype

<karlt> yes, that would be nice

<behdad> as for getting to cairo's FT lib, just create a toy scaled font
         or something and get the lib out of that

<karlt> cairo's FT_Library is kind of available through
        cairo_ft_scaled_font_lock_face, and face->glyph->library

<behdad> yeah

<karlt> is it safe to assume that modifications to cairo won't choose to Done
        the FT_Library before reset_static_data?

<behdad> karlt: yes
Comment 21 Karl Tomlinson (:karlt) 2008-12-03 03:04:21 PST
Created attachment 351146 [details] [diff] [review]
src:url() v1.1 - use cairo's FT_Library [pushed to m-c and 1.9.1]

This avoids the crash in comment 15.

We'll also want to fix https://bugs.freedesktop.org/show_bug.cgi?id=18862 to avoid crashes in FT_Done_Face or FT_Set_Transform that occur at a random time after browsing pages with @font-face { src: url() }.
Comment 22 Karl Tomlinson (:karlt) 2008-12-05 23:20:22 PST
Comment on attachment 351146 [details] [diff] [review]
src:url() v1.1 - use cairo's FT_Library [pushed to m-c and 1.9.1]

I pushed this to m-c with one small change to the font used to find the
FT_Library

-            new gfxPangoFontGroup(NS_LITERAL_STRING("\"sans-serif\""),
+            new gfxPangoFontGroup(NS_LITERAL_STRING("sans-serif"),

so as to use mozilla default font (which is often the system default) instead
of just the system default.

http://hg.mozilla.org/mozilla-central/rev/e6c31f12b879
http://hg.mozilla.org/mozilla-central/rev/b09d5673573e
Comment 23 Karl Tomlinson (:karlt) 2008-12-05 23:21:51 PST
Fixed on mozilla-central.  Needs to land on 1.9.1.
Comment 24 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-12-06 00:23:19 PST
(In reply to comment #22)
> I pushed this to m-c with one small change to the font used to find the
> FT_Library
> 
> -            new gfxPangoFontGroup(NS_LITERAL_STRING("\"sans-serif\""),
> +            new gfxPangoFontGroup(NS_LITERAL_STRING("sans-serif"),
> 
> so as to use mozilla default font (which is often the system default) instead
> of just the system default.

Whether Mozilla's default is serif or sans-serif is controlled by the font.default.[langGroup] preferences (and maybe also the font.default preference).
Comment 25 Karl Tomlinson (:karlt) 2008-12-06 01:40:17 PST
(In reply to comment #24)
> (In reply to comment #22)
> > I pushed this to m-c with one small change to the font used to find the
> > FT_Library
> > 
> > -            new gfxPangoFontGroup(NS_LITERAL_STRING("\"sans-serif\""),
> > +            new gfxPangoFontGroup(NS_LITERAL_STRING("sans-serif"),
> > 
> > so as to use mozilla default font (which is often the system default) instead
> > of just the system default.

I had intended to explain with an indefinite article here: "use _a_ mozilla default font", but managed to leave that out of the explanation.

> Whether Mozilla's default is serif or sans-serif is controlled by the
> font.default.[langGroup] preferences (and maybe also the font.default
> preference).

Yes.

In this particular situation, the precise font is of little importantance, as all fonts will give us the same FT_Library.  Using a default (generic) font is an attempt to avoid another font being opened through FreeType.

You've made me aware that the default langGroup on the gfxFontStyle of "x-unicode" will cause the pref "font.name.sans-serif.x-unicode" to be used.  The default value for this pref is "\"sans-serif\"", which would resolve to a font suitable for the language of the user's locale, but if the pref has been modified, it becomes more likely that it could be a font quite different from what would normally be used for their language from font.name.[generic].[langGroup from locale].

However, the issues here are not worth any extra code to look up preferences.
Comment 26 Nicolas Mailhot 2008-12-06 02:27:48 PST
Please take a look at bug #414427 on font defaults.

Any program (firefox included) that does not use the common system defaults but hardcodes its own preferences is broken by design.
Comment 27 Karl Tomlinson (:karlt) 2008-12-10 22:04:03 PST
Comment on attachment 351146 [details] [diff] [review]
src:url() v1.1 - use cairo's FT_Library [pushed to m-c and 1.9.1]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6510330ddad7

Note You need to log in before you can comment on or make changes to this bug.