458861 - downloading fonts periodically fails under Mac OS X

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[John Daggett (:jtd)]

With a testpage that uses script to load the same font 50 times, the ATS server periodically generates a bum font.  With debug builds, this generates a warning:

WARNING: ATSFontGetPostScriptName err = -984: file /builds/mozcentral/gfx/thebes/src/gfxQuartzFontCache.mm, line 160
W

Steps:

1. Save attached testcase in layout/reftests/fonts
     - or -
   Save the attached testcase into a folder and copy in Ahem.ttf

2. Load in the page using the latest nightly

Result: See a bunch of red lines with "loadtestXX" showing up in white against a red background.

Expected result: Should see *only* red lines (this is actually another bug, bug xxx).

Comment 1

[John Daggett (:jtd)]

Attachment: testcase, load the same font 50 times

Once the Acid 3 bug 457194 is fixed, the testcase should only display a blank page.

Comment 2

[John Daggett (:jtd)]

Same problem does not occur under Windows.

Comment 3

[John Daggett (:jtd)]

Attachment: testcase, simpler version using data url version of Ahem

To reproduce, load the attached testcase, no need to save or set up fonts.

Comment 4

[John Daggett (:jtd)]

Attachment: code that causes ATS cache corruption

I isolated this as a problem caused by ATS (Apple Type Server) cache corruption.  One of the reftests attached to bug 452870 tests out using a html page URL within a font face rule, something like this:

  @font-face {
    font-family: test;
    src: url(http://example.com/blah.html);
  }

Attempting to activate a font using the HTML source data, causes subsequent font loads to fail randomly, hence the -984 error.

The attached source simply cycles through loading and unloading font files specified in the arg list.  A -n option allows the number of load/unload cycles to be specified.

Comment 5

[John Daggett (:jtd)]

Attachment: test utility with fonts, disk image

Steps:

1. Open the attached disk image
2. Open a Terminal window and enter the command below:

    cd /Volumes/ATS\ Cache\ Bug/

3. Run the test program, this should run without any errors:

    ./atscachebug -n 10 font*

4. Run the test program again, but include a HTML source file in the list:

    ./atscachebug -n 10 html.ttf font*

   Error messages should appear:

    load font failed with err: -984 (/Users/jd/temp/fonts/html.ttf)

5. Run the test program with the original list of fonts:

    ./atscachebug -n 10 font*

Error messages will now appear even though these are all valid fonts.  Which fonts have problems is related to the order in which they are loaded, not the actual contents of the font data loaded.

So loading bad font data causes the ATS server to randomly fail on subsequent font loads, independent of whether or not the fonts are valid.  The workaround is to do some simple font validation before loading the font.

Comment 6

[John Daggett (:jtd)]

Logged as Apple bug 6283556.

Comment 7

[philippe (part-time)]

John, would that be the same underlying cause for WebKit ? I regularly -but not as frequently as with Minefield- see the most recent WebKit builds fail to use the downloaded fonts.
(that is all with minimal test cases, and during reload of those testcases)

Comment 8

[John Daggett (:jtd)]

Attachment: testcase, causes ATS font server corruption (FF/Webkit)

(In reply to comment #7)
> John, would that be the same underlying cause for WebKit ? I regularly -but not
> as frequently as with Minefield- see the most recent WebKit builds fail to use
> the downloaded fonts.

I tested this and yes, it will occur in Webkit.  It's harder to test Webkit because there's a caching mechanism in place to cache font resources, similar to an image cache.  The attached testcase will reproduce this.

Steps:

1. download the test utility folder, attached
2. save this testcase into that folder
3. open the testcase in Mozilla latest or Webkit latest

Result: some font instances will not load correctly.

After this, other pages using downloadable fonts that normally would have rendered just fine will suffer from intermittent font download failures.  There's some code in the Webkit font loading code that appears to be doing a sanity check on the font:

    // Workaround for <rdar://problem/5675504>.
    if (!CGFontGetNumberOfGlyphs(cgFontRef)) {
        CFRelease(cgFontRef);
        cgFontRef = 0;
    }

My guess is that this is probably to work around this problem.

Comment 9

[John Daggett (:jtd)]

Note: tested with Mac OS X 10.5.5 (9F33), Webkit nightly r37469

Comment 10

[John Daggett (:jtd)]

To clear up the problem, clean out ATS caches and reboot:

  sudo find /private/var/folders -name "*ATS*" -print | xargs rm -rf

For those with weak knees, you can also use Font Finagler:

  http://homepage.mac.com/mdouma46/fontfinagler/

Reboot after use.

Comment 11

[John Daggett (:jtd)]

Attachment: screenshot, desktop corruption after rebooting

Screenshot, right *after* restarting.

When ATS font cache corruption occurs, somehow the desktop also is affected.  Without explicitly deleting the font cache files in /private/var/folders (under 10.5), the desktop displays a bunch of garbage upon reboot.  Anything that causes a refresh cleans this out but it's a little scary that a bug like this could have this kind of impact...

Comment 12

[philippe (part-time)]

(In reply to comment #11)

I suspect there is something really wrong with the 10.5.5 start up / restarting routines... Bug 459238 had a similar corruption (in Fx), I had a freaky one (in Preview.app) after upgrading to 10.5.5 on one machine, and I've seen a number of posts on the Apple forums with similar 'artwork'.

Comment 13

[John Daggett (:jtd)]

Yeah, we should be able to avoid such journeys into abstract art land by doing some simple validations on the TrueType header data, so that trivial mistakes by web authors don't cause font server cache/screen corruption like this.  I should have a patch for this tomorrow.

Comment 14

[John Daggett (:jtd)]

Attachment: patch, v.0.2b, validate font headers before activating

Added a simple font table header validation routine to gfxFontUtils.  This will verify that at least the font data looks like TrueType/OpenType data.

  * switch to loading into memory instead of files (nsStreamLoader)
  * validate font data headers and checksum
  * make sure the font data has basic required tables
  * include in load group so that onload handler fires after font loads

Need to do some more testing before review.

Comment 15

[John Daggett (:jtd)]

Simpler way of fixing the font cache corruption:

1. Open up a Terminal window

2. Enter these commands

  sudo atsutil databases -removeUser 
  sudo atsutil server -shutdown
  atsutil server -ping

A message indicating the ATS server is running should appear.  No reboot required.

Comment 16

[John Daggett (:jtd)]

Comment on attachment 343206 [details] [diff] [review]
patch, v.0.2b, validate font headers before activating

Tested both on Mac/Win, validation is done on both platforms even though underlying problem is Mac-specific.

Comment 17

[John Daggett (:jtd)]

Note: as part of the work for 458160, support for .otf fonts on Windows, I'm going to dump the EOT header creation code and just munge the name table for all fonts.  So there's a certain amount of "double validation" here that will go away as part of that work.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Basically looks great.

I'm tempted to use an nsTArray<PRUint8> as the type for the downloaded font data, so we don't have two parameters (pointer and length) being passed everywhere. How does that sound?

+IsValidSFNTVersion(PRUint32 version)
+{
+    // normally 0x00010000, CFF-style OT fonts == 'OTTO' and Apple TT fonts = 'true'
+    return version == 0x10000 || version == 'OTTO' || version == 'true';
+}

http://www.microsoft.com/typography/otspec/otff.htm says that "typ1" is also allowed here --- but neither 'true' nor 'typ1' should be used for fonts that contain Opentype tables. Is there a reason we have 'true' but not 'typ1' here?

+    const HeadTable  *headData = reinterpret_cast<const HeadTable*>(aFontData + headOffset);

extra space between HeadTable and *

+        if (PRUint64(nameOffset) + PRUint64(PRUint32(nameHeader->stringOffset)) + PRUint64(nameoff) + PRUint64(namelen) > dataLength)

Why do you need the nested cast?

+    eotHeader->italic = (PRUint16) os2Data->fsSelection & 0x01;

Do we need this cast here? I don't see why.

Comment 19

[John Daggett (:jtd)]

(In reply to comment #18)

> I'm tempted to use an nsTArray<PRUint8> as the type for the downloaded font
> data, so we don't have two parameters (pointer and length) being passed
> everywhere. How does that sound?

I think that makes more sense but right now it's determined by the 
Right now, that's dictated by the parameter to OnStreamComplete.  As part of the work to support .otf fonts on windows we need to tweak this interface so I'll do that as a part of that patch.

> http://www.microsoft.com/typography/otspec/otff.htm says that "typ1" is also
> allowed here --- but neither 'true' nor 'typ1' should be used for fonts that
> contain Opentype tables. Is there a reason we have 'true' but not 'typ1' here?

Old Apple TT fonts can have this set to 'true'.  We could special case this to Mac-only but whether the font has a 'OS/2' table or not is more important.  The 'typ1' version indicates a Type 1 font wrapped in a SFNT form, so I'm excluding that explicitly. I'll add a comment to note these two explicitly.

http://developer.apple.com/textfonts/TTRefMan/RM06/Chap6.html#ScalerTypeNote

> +    eotHeader->italic = (PRUint16) os2Data->fsSelection & 0x01;
> 
> Do we need this cast here? I don't see why.

The cast is needed for the compiler to choose the right cast operator for AutoSwap_PRUint16.

Comment 20

[John Daggett (:jtd)]

Attachment: patch, v.0.3, changes based on review comments

Comment 21

[John Daggett (:jtd)]

checked in