Closed Bug 1442669 Opened 4 years ago Closed 4 years ago

font-optical-sizing reftest hits a fatal assertion "assert(SkTFitsIn<D>(s))" when run on Linux64-debug

Categories

(Core :: Graphics: Text, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jfkthame, Assigned: lsalzman)

References

(Blocks 1 open bug)

Details

(Whiteboard: gfx-noted)

Attachments

(1 file)

In bug 1435692, we're about to land a reftest that uses Google's demo variation font "Voto Serif" to test the font-optical-sizing property.

Although this font seems to render as expected across all platforms when we have variation font support, when run on Linux (debug) it hits a fatal assertion within Skia:

[task 2018-03-02T16:19:18.794Z] 16:19:18     INFO - /builds/worker/workspace/build/src/gfx/skia/skia/include/core/SkTypes.h:212: fatal error: "assert(SkTFitsIn<D>(s))"
[task 2018-03-02T16:19:18.794Z] 16:19:18     INFO - Abort from sk_abort
[task 2018-03-02T16:19:18.794Z] 16:19:18     INFO - Hit MOZ_CRASH() at /builds/worker/workspace/build/src/memory/mozalloc/mozalloc_abort.cpp:33

See the reftest-2 oranges on https://treeherder.mozilla.org/#/jobs?repo=try&revision=125f417f3ac1b270a311a0f7ad74702aee1f2509.

I'm intending to mark this test as skipped on Linux/debug for now, so that we can get bug 1435692 landed, but this needs further investigation. It's not clear to me at the moment whether this indicates a fault in the test font, or an internal bug within Skia, or....?
The assertion points to the found that the bounding box of glyph outlines can't be stored in an int16_t here: https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/ports/SkFontHost_cairo.cpp#666

That in itself seems strange, like either the glyph has a gigantic bounding box inherently, or was never the less scaled up to such a gigantic size that it did overflow the int16_t?
Odd; the glyphs don't look out of the ordinary at all when I've viewed the axis-praxis sample using that font.

I was intending to see if I could reproduce the assertion locally, but haven't had time yet. (Feel free to give it a try if you have a few spare cycles...)
Whiteboard: gfx-noted
It looks like every other Skia font host (DWrite and Mac) both check that the bounds fit within the S16/U16 square, which is why they seemingly pass, while the Cairo font host does not, and thus asserts.

Now that this is actually checked against, the mentioned reftest seems to pass just fine.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8961484 - Flags: review?(jfkthame)
I still don't understand why this test should apparently involve a glyph with such a gigantic bbox. It uses font-size:96px, and the default instance has max bounds (according to the 'head' table) of:

    <xMin value="-748"/>
    <yMin value="-389"/>
    <xMax value="3474"/>
    <yMax value="1032"/>

(with a UPEM value of 1000), so the maximal x-extent is about 4.2em, or a little over 400px at font-size:96px.

Even allowing for variation deltas to be applied, that's a long way short of overflowing 16 bits. So where does the problematic bounding box come from? Might this indicate a FreeType bug, maybe?
Can we at least get this patch in to deal with the symptom and prevent the overflowing of the glyph width/height values for now?
Yes, let's do that. Sorry, I was hoping to look into this a bit more but haven't had time to get to it. Avoiding the overflowing bounds seems like a good precaution anyway.
Comment on attachment 8961484 [details] [diff] [review]
guard against excessively large glyphs in SkFontHost_cairo

Review of attachment 8961484 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/skia/skia/src/ports/SkFontHost_cairo.cpp
@@ +724,5 @@
>          SkDEBUGFAIL("unknown glyph format");
>          return;
>      }
>  
> +    if (SkIRect::MakeXYWH(SHRT_MIN, SHRT_MIN, USHRT_MAX, USHRT_MAX).contains(bounds)) {

If this condition fails, should we maybe set the glyph-f{Width,Height,Left,Top} fields to the max 16-bit values, rather than leaving them untouched? I.e. clamp rather than ignore?
Attachment #8961484 - Flags: review?(jfkthame) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #7)
> Comment on attachment 8961484 [details] [diff] [review]
> guard against excessively large glyphs in SkFontHost_cairo
> 
> Review of attachment 8961484 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/skia/skia/src/ports/SkFontHost_cairo.cpp
> @@ +724,5 @@
> >          SkDEBUGFAIL("unknown glyph format");
> >          return;
> >      }
> >  
> > +    if (SkIRect::MakeXYWH(SHRT_MIN, SHRT_MIN, USHRT_MAX, USHRT_MAX).contains(bounds)) {
> 
> If this condition fails, should we maybe set the
> glyph-f{Width,Height,Left,Top} fields to the max 16-bit values, rather than
> leaving them untouched? I.e. clamp rather than ignore?

I think that would be potentially dangerous in that then we risk allocating a USHRT_MAX * USHRT_MAX (= 4GB) chunk of memory for the glyph in the worst case. Skipping this glyph seems safer, since it is how the other platforms (Windows/Mac) behave.
Fair enough, let's go with that.
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa0ba213a294
guard against excessively large glyphs in SkFontHost_cairo. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/aa0ba213a294
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.