407761 - crash [@ gfxAtsuiFont::SetupCairoFont]

Status: VERIFIED FIXED

(Core :: Graphics, defect, P3)

Description

[:Gavin Sharp [email: [e-mail]]]

aContext is null. I see:

WARNING: Could not get glyph extents: file /Users/gavin/moz/mozilla/gfx/thebes/src/gfxFont.cpp, line 538

before crashing.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: testcase (crashes on load)

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: stack

Comment 3

[Mike Schroepfer]

JDaggett this going to get fixed by your font work?

Comment 4

[John Daggett (:jtd)]

No, I don't think this will be fixed by any changes in the font matching code.  This one started showing up around 11/9 in crash reports so I'm guessing it's tied to some of the changes for by 385417 by roc/reed.  The crash reports only show it occurring on 10.5.

http://crash-stats.mozilla.com/report/list?range_unit=months&query_search=signature&query_type=contains&platform=mac&signature=gfxAtsuiFont%3A%3ASetupCairoFont%28gfxContext%2A%29&range_value=2

Comment 5

[John Daggett (:jtd)]

Confirmed works-for-me on 10.4.11 with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2007121204 Minefield/3.0b3pre

Will test this under 10.5 tomorrow.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I can't get this to crash in the latest Minefield nightly or my CmTrunk build from tonight on 10.5.1.

After a slew of the warnings from comment 0, I also see this:
 
<Error>: ATSFontGetPostScriptName failed to return valid ASCII PostScript name

Comment 8

[Snala]

(In reply to comment #7)
> *** Bug 408701 has been marked as a duplicate of this bug. ***
> 

Good test sites to reproduce (From Bug 408701)
http://-coey-.deviantart.com
or
http://www.deviantart.com/deviation/70742546/
This bug appears for me on the latest nightly. :/

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I do crash with attachment https://bugzilla.mozilla.org/attachment.cgi?id=291259 with this stack and Gavin's warning on 10.5.1 ;)

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Marcia says over in bug 396137 this is the #3 crasher for b2.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This is crashing because http://mxr.mozilla.org/seamonkey/source/layout/generic/nsTextFrameThebes.cpp#5690 calls MeasureText with a nsnull refContext -- however, it also specifies PR_FALSE to tight metrics, and the refContext is only supposed to be used when tight metrics are requested.

The problem is, one of the glyphs is not a simple glyph, so in http://mxr.mozilla.org/seamonkey/source/gfx/thebes/src/gfxFont.cpp#404 we fail that test, and we drop into the else clause which unconditionally does GetTightGlyphExtents... and then boom.

roc, what's supposed to happen here?  I'm guessing we're seeing this a lot more because we dropped the min font size for using tight glyph extents a good amount.  Are we supposed to just not request the tight glyph metrics and use the bounding box like we do in the simple glyph case?

Comment 13

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

There should have been a FetchGlyphExtents call on that textrun which should have preloaded all necessary glyph extents, right here:
http://mxr.mozilla.org/seamonkey/source/gfx/thebes/src/gfxFont.cpp#1948

Comment 14

[Marcia Knous [:marcia]]

Using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007122004 Minefield/3.0b3pre, I do not crash with Gavin's test case, but I crash instantly when I visit http://www.deviantart.com/deviation/70742546/.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: testcase that triggers

Ok, so what's happening is that there's a bogus glyph (0x3232) in here that, for some reason, we're assinging to the same text run as the other normal ascii glyphs.  That glyph doesn't exist in the font, so the initial caching of metrics fails (this is the assertion).  But later since there's no entry for it, we end up blowing up because we go through the same path expecting that everything was cached.

I'm inclined to leave this be until John's patch lands -- John, could you test this out if you get a chance with your patch?  The deviantart link crashes every time for me, but here's a testcase for at least the assertion (it doesn't cause a crash, though, but the assertion should also go away).

(Note that the first testcase only sporadically crashes for me.)

Comment 16

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

Taking.

The "bogus glyph" is actually the character code; this is a missing-glyph situation and we shouldn't be trying get the extents for such "glyphs". I'll fix.

Comment 17

[Snala]

As of the current nightly, this no longer triggers with either of testcases..

But the deviantart link I provided still does.  I am curious if Bug 408701 is really a duplicate, or is this a related but different problem?

Thoughts?

Comment 18

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

Attachment: fix

1) Add better comments to FetchGlyphExtents and elsewhere
2) FetchGlyphExtents should not try to fetch extents for missing glyphs
3) gfxFont::Measure should not try to get extents for missing glyphs; the rectangle given by the glyph advance and the font ascent and descent covers the missing-glyph box
4) gfxFont::Measure should gracefully handle the cases where glyph extents are not available. They might not be available due to OOM when we were setting up the textrun.

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 294151 [details] [diff] [review]
fix

Looks good to me!

Comment 20

[John Daggett (:jtd)]

Do we understand why this only occurs on 10.5?  With Vlad's testcase on 10.5 I see warnings "Could not get glyph extents" which I don't see on 10.4.  In both cases, there are no fonts for the missing character U+0CA0 (unless you have fonts for Kannada installed), so the missing extents should be the same.  Are we just unlucky on 10.5 where something failed benignly on 10.4?

Comment 21

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

checked in, with crashtest

Comment 22

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

(In reply to comment #20)
> Do we understand why this only occurs on 10.5?

To be honest, I don't. It's possible ATSUGlyphGetScreenMetrics always returns noErr on 10.4 for an unknown glyph ID and just gives us bogus metrics, that would hide this bug.

A similar explanation probably holds for Windows and Pango: the default gfxFont::SetupGlyphExtents calls cairo_glyph_extents without checking for errors, so we always appear to get some extents even for bogus glyph IDs, also hiding this bug.

Comment 23

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007123104 Minefield/3.0b3pre and the testcase from this bug - no crash on testcase

-> Verified fixed